CMake fixes

[h-vetinari]

Closes #58 Closes #61

[conda-forge-linter]

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

[hmaarrfk]

The issue, i believe. Is that you may now get clobbered files.

I would suggest we maybe look into patching the targets file to make the static targets optional.

[h-vetinari]

The issue, i believe. Is that you may now get clobbered files.

Not if we separate the builds completely. TBH I don't understand why zstd got co-installed with zstd-static in the last run, but I added a run_constrained now that should make this (and the clobbering) go away. Hopefully at least 🤞

[h-vetinari]

@conda-forge/zstd

The current status of this feedstock is not ideal as far as I can tell. Perhaps there's something I'm missing, but at least the CMake integration is broken (see #58). I also don't think it's particularly elegant to have shared and static libs in the same output (and the absence of static libs in the nominally shared-only zstd output is not true on windows).

Current status (before this PR):

output	contains shared lib	contains static lib	contains CLI	comment
zstd	✔️	➖ (unix) ✔️ (win)	✔️	broken CMake on unix ❌
zstd-static	✔️ (transitively)	✔️	✔️ (transitively)	run-depends on zstd output; both outputs build the shared lib & CLI, but conda manages to deduplicate this and avoids clobbering here

Status of this PR:

output	contains shared lib	contains static lib	contains CLI	comment
zstd	✔️	➖	✔️	working CMake ✔️
zstd-static	✔️	✔️	✔️	independent of zstd output

Future?

If I were to draw this feedstock up from scratch, I believe the cleanest approach would be the following, but I didn't want to make such invasive changes now in a PR that's only meant to unblock the CMake stuff and leave the rest working as-is.	output	contains shared lib	contains static lib	contains CLI	comment
libzstd	✔️	➖	➖
libzstd-static	➖	✔️	➖
zstd	✔️ (transitively)	➖	✔️	depends on libzstd
zstd-static				obsolete

[hmaarrfk]

I'm not sure I understand your statement in:

• "Current Status (Before this PR)"
• zstd-static clobbers cli of ztd

If i look at the ztd-static for linux, it only contains a single file libzstd.a and does not contain any other files.

"clobbering" for me means that a file exists from two different packages. It will cause one package to overwrite the file of the other.

The comment in the main branch indicates that conda-build will not include the shared libraries, cli, or cmake file, because they are included by ztd a package that is installed at build time of ztd-static.

[h-vetinari]

"clobbering" for me means that a file exists from two different packages. It will cause one package to overwrite the file of the other.

Yes, that's what I meant...

The comment in the main branch indicates that conda-build will not include the shared libraries, cli, or cmake file, because they are included by ztd a package that is installed at build time of ztd-static.

... both outputs currently build the shared lib & CLI, but I was not aware that conda successfully deduplicates this (in most cases I've seen this happen, it didn't work). I'll edit the overview comment to reflect this.

[h-vetinari]

@hmaarrfk Any further thoughts on this?

[h-vetinari]

@conda-forge/core

@hmaarrfk is quite busy ATM, and the rest of @conda-forge/zstd are pretty inactive too - could someone have a look here please? Summary here.

[hmaarrfk]

I unfortunately disagree with the approach, but do not have time to delve into it too much. Instead, I'm hoping you can help me understand who the "owner" of each of the 4 files is:

1. zstdConfig.cmake
2. zstdConfigVersion.cmake
3. zstdTargets.cmake
4. zstdTargets-release.cmake

The source of the problem, in my opinion, is that zstdTargets.cmake (in zstd 1.5.2 h8a70e8d_2 conda-forge) the recontains the following code:

foreach(_expectedTarget zstd::libzstd_shared zstd::libzstd_static)
  list(APPEND _expectedTargets ${_expectedTarget})
  if(NOT TARGET ${_expectedTarget})
    list(APPEND _targetsNotDefined ${_expectedTarget})
  endif()
  if(TARGET ${_expectedTarget})
    list(APPEND _targetsDefined ${_expectedTarget})
  endif()
endforeach()

[...]
# Loop over all imported files and verify that they actually exist
foreach(target ${_IMPORT_CHECK_TARGETS} )
  foreach(file ${_IMPORT_CHECK_FILES_FOR_${target}} )
    if(NOT EXISTS "${file}" )
      message(FATAL_ERROR "The imported target \"${target}\" references the file
   \"${file}\"
but this file does not exist.  Possible reasons include:
* The file was deleted, renamed, or moved to another location.
* An install or uninstall procedure did not complete successfully.
* The installation package was faulty and contained
   \"${CMAKE_CURRENT_LIST_FILE}\"
but not all the files it references.
")
    endif()
  endforeach()
  unset(_IMPORT_CHECK_FILES_FOR_${target})
endforeach()
unset(_IMPORT_CHECK_TARGETS)

This one file, is incompatible with the presently allowed two options:

1. shared library installed, static library not installed.
2. shared library installed, static library installed <---- works with this

Therefore, I would like to understand the specifics of the 4 aforementioned files. Which package, in the new structure, actually owns them.

[h-vetinari]

Thanks for the detailed response. Since the outputs do not depend on each other anymore, each output owns its own copy of those files, and they don't clobber each other because they are intentionally not co-installable.

It would be much cleaner still not to have an output that contains both shared & static lib in the first place, but I wanted to keep the amount of changes (in terms of content of the various outputs) to a minimum. If you like the proposal under "Future?" better, I'd be very happy to prepare a PR.

[ocefpaf]

I'm extremely busy this coming week but I can try to take a look at when I get back. I'd love Isuru's input here too but he is finishing grad school and I don't think it is fair to ask him to review it.

With all that said, if you two are in agreement go ahead and merge and we can pull the packages if something is amiss.

[h-vetinari]

I'll note that this has some degree of time sensitivity, because it's currently blocking progress on building the LLVM 15 rc's (in fact, from the POV of the llvmdev feedstock, #58 is a regression, because the same recipe worked at the time of LLVM 14).

I'm stuck at step 0 in a series of quite a few outputs necessary to do that, and not being able to progress mean we might miss the now-much-shorter rc window to get in fixes that are required/beneficial for us (which has the potential to substantially increase the integration work later on).

[hmaarrfk]

Ok. To be honest, i don't even know why the static library got added. I feel like we over cautiously added static libraries, and I'm not sure who requested this particular package.

I feel like there will be challenges with users needing to install bot the static and the dynamic library.

But let's try.

[h-vetinari]

Thanks!

i don't even know why the static library got added

Perhaps @wolfv (who did this in #43) still remembers?

I feel like there will be challenges with users needing to install bot the static and the dynamic library.

Note that the state of affairs is unchanged by this PR (i.e. also previously, zstd-static contained both the dynamic and the static build). I'm happy to further separate this as outlined here (but this PR was intended as a minimal fix to progress elsewhere, without opening up larger-than-necessary topics for discussion).

[hmaarrfk]

But packaged will depend on the package named zstd and if it isn't installed in their environment, the solver will complain.

[h-vetinari]

But packages will depend on the package named zstd and if it isn't installed in their environment, the solver will complain.

I don't understand this concern.

• If a feedstock currently depends on zstd, it only needs the SO[^1], which it will continue to get.
• If a feedstock currently depends on zstd-static, it may use both the dynamic & static artefacts, which it will continue to get (only now, this will not also bring in the zstd output anymore, but that's purely an organisational entity/wrapper in this regard).

[^1]: except perhaps on windows, where we previously also packaged the static lib into zlib; if a feedstock breaks due to the now-missing static lib, the immediate remedy is to change to zlib-static on windows.

[h-vetinari]

I feel like there will be challenges with users needing to install bot the static and the dynamic library.

Ah, perhaps you meant "users depending on both zlib and zlib-static"? I feel such mixing is a pretty clear anti-pattern (or at least strictly worse than using static libs in the first place, for which we have CFEP-18).

That indeed would not be possible anymore (though again, the use-case is de facto still supported - as previously - by only depending on zlib-static, which brings in both).

[hmaarrfk]

1. User builds package a, which depends on zstd dynamic. zstd exports a requirement of zstd.
2. User tries to build package b that depends on a and zstd-static. This user, can no longer build their package.

Package b cannot depend on just zstd-static because the dependency on zstd is controlled by package a.

[h-vetinari]

Package b cannot depend on just zstd-static because the dependency on zstd is controlled by package a.

Ah, yes, the run-export & transitivity... I hope this is not actually an issue in practice, but if yes, I'd agree to mark the current builds as broken.

However that just brings us back to the underlying problem here, which is that we can have only 3 out of the following 4 (AFAICT):

1. working CMake integration
2. no clobbering of CMake files
3. no manual hacking (of CMake files resp. upstream CMake integration)
4. zstd & zstd-static co-installable

Currently I've chosen to give up 4. - perhaps an argument can be made that giving up 2. is less harmful despite being against best practice.

[hmaarrfk]

Right, so honestly, i would almost favor deleting the static packages, and seeing if anybody complains.

[hmaarrfk]

However, stats show that they are in use, by somebody: https://anaconda.org/conda-forge/zstd-static/files

[hmaarrfk]

My hunch however, is that the little people that do try, will not be hitting the paradigm that I described, but rather be trying to build out a fuller statically linked stack.

[hmaarrfk]

For what its worth, I did look for quite some time on making a cmake target optionally installed. It doesn't seem like it is a common usecase.

[hmaarrfk]

The other alternative, which I don't have time to flesh out, is to return to the previous system, but, to ensure that:

1. cmake works with the dynamic libraries.
2. cmake fails to automatically report the static libraries.

This would favor the dynamic library usage, instead of forcing users of cmake+zstd to have the static libraries installed.

[h-vetinari]

2. cmake fails to automatically report the static libraries.

Wouldn't that defeat the point of zstd-static? If I use an output named like that, I'd certainly expect the static lib to be picked up, not the dynamic one (i.e. compiling against zstd-static would seem to work, but would actually do the same as compiling against zstd, i.e. create a runtime dependence on zstd)

[h-vetinari]

However, stats show that [zstd-static is] in use, by somebody: https://anaconda.org/conda-forge/zstd-static/files

I don't trust github's code-search, but the only uses that show up are micromamba and one other feedstock.

The micromamba case makes sense to me (no runtime dep for something "micro"), and is presumably why @wolfv added this in the first place. The good thing is that is will not be affected by the run-export problem (and actually, this would be better served by the kind of standalone libzstd-static I described above resp. below; interestingly, also up until now, this didn't inject a run-export, presumably because host-deps aren't transitive)

In the other feedstock, zstd-static was added by @beckermr in https://github.com/conda-forge/_libarchive_static_for_cph-feedstock/pull/14 without further comment - but the name of the feedstock implies that this should also only need the static lib (and similarly be unaffected by a transitive run-export clash).

In particular, both of these cases do not require (best as I can tell) on having zstd-static depend on zstd. All in all, I'd now prefer to follow through with the proposal I had further up:

output	contains shared lib	contains static lib	contains CLI	comment
libzstd	✔️	➖	➖
libzstd-static	➖	✔️	➖
zstd	✔️ (transitively)	➖	✔️	depends on libzstd
zstd-static				obsolete

The two known users of zstd-static could move to libzstd-static just fine, and for everything else it should be a clear improvement IMO.

[hmaarrfk]

Run exports will only be triggered for direct dependencies

[h-vetinari]

Run exports will only be triggered for direct dependencies

I know. I just added "presumably" to my equivalent statement because I'm not 100% sure on all the details behind the scenes... 😅

Any comment about the rest of what I wrote? Would you consider a split as outlined beneficial? Do you still prefer your own proposal (AIUI: do you think that zstd-static with disabled CMake detection for the static builds would make sense)?

[h-vetinari]

I ended up opening an issue for wider discussion about this, because I've encountered the same problem in libprotobuf: https://github.com/conda-forge/conda-forge.github.io/issues/1809