CMake fixes #62

h-vetinari · 2022-07-31T20:48:18Z

Closes #58
Closes #61

conda-forge-linter · 2022-07-31T20:48:21Z

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 · 2022-07-31T21:55:35Z

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 · 2022-07-31T22:06:42Z

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 🤞

hmaarrfk · 2022-07-31T22:50:34Z

recipe/meta.yaml

+      run_constrained:
+        # do not co-install with shared-only lib;
+        # zstd-static contains the shared libs already anyway
+        - zstd <0.0a0


The issue is that the previous design was completely going against this.

The goal is:

Try to compile a package that depends on package a and zstd.

a links to zstd dynamically

The new package links to zstd statically.

The issue is that the previous design was completely going against this.

Respectfully, the previous design was a hack, and demonstrably broken (at least from the POV of cmake). I think it's a bad idea for the output containing the static+shared build to depend on the output for the shared-only build - it all but guarantees broken CMake metadata and/or clobbered files.

The goal is:

Try to compile a package that depends on package a and zstd.

a links to zstd dynamically

The new package links to zstd statically.

I don't understand your point here. Why should a package depending on the (already-now) shared zstd end up linking statically?

On the off chance that you're referring to "zstd-static contains the shared libs already anyway" - this was previously the case already. In fact, the build breaks when passing both:

ZSTD_BUILD_STATIC=ON ZSTD_BUILD_SHARED=OFF

The error was something like "the CLI cannot be built without the shared lib". I didn't investigate further how to disable building the CLI, as I wanted to keep the previous setup of having both shared & static libs within the zstd-static output - the only point I changed is that it doesn't depend on the zstd output anymore.

PS. It's obviously not great to have shared + static lib in the same output, but that's the way things are currently, not least because the upstream build doesn't make it easy to build only the static one.

The error was something like "the CLI cannot be built without the shared lib". I didn't investigate further how to disable building the CLI, as I wanted to keep the previous setup of having both shared & static libs within the zstd-static output - the only point I changed is that it doesn't depend on the zstd output anymore.

I did investigate this now, details in this comment resp. 22966cd

h-vetinari · 2022-08-01T09:26:49Z

@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

https://github.com/facebook/zstd/blob/v1.5.2/build/cmake/CMakeLists.txt#L144-L152

wait for conda-forge/zstd-feedstock#62

hmaarrfk · 2022-08-01T10:46:32Z

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 · 2022-08-01T11:34:21Z

"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.

wait for conda-forge/zstd-feedstock#62

h-vetinari · 2022-08-03T15:19:39Z

@hmaarrfk Any further thoughts on this?

h-vetinari · 2022-08-06T07:42:34Z

@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 · 2022-08-06T12:12:33Z

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:

zstdConfig.cmake
zstdConfigVersion.cmake
zstdTargets.cmake
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:

shared library installed, static library not installed.
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 · 2022-08-06T12:20:25Z

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 · 2022-08-06T14:51:10Z

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 · 2022-08-08T10:14:24Z

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 · 2022-08-08T11:21:39Z

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 · 2022-08-08T11:30:07Z

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 · 2022-08-08T11:32:17Z

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

h-vetinari · 2022-08-08T11:37:58Z

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¹, 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).

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 · 2022-08-08T11:41:01Z

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 · 2022-08-08T11:49:36Z

User builds package a, which depends on zstd dynamic. zstd exports a requirement of zstd.
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 · 2022-08-08T12:00:55Z

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):

working CMake integration
no clobbering of CMake files
no manual hacking (of CMake files resp. upstream CMake integration)
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 · 2022-08-08T12:02:32Z

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

hmaarrfk · 2022-08-08T12:03:34Z

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

hmaarrfk · 2022-08-08T12:04:50Z

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 · 2022-08-08T12:05:16Z

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 · 2022-08-08T12:06:44Z

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

cmake works with the dynamic libraries.
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 · 2022-08-08T12:13:57Z

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 · 2022-08-08T12:43:08Z

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 conda-forge/_libarchive_static_for_cph-feedstock#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 · 2022-08-08T14:40:14Z

Run exports will only be triggered for direct dependencies

h-vetinari · 2022-08-08T15:32:27Z

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 · 2022-08-08T18:21:09Z

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

h-vetinari force-pushed the cmake branch from fed4c44 to 130b244 Compare July 31, 2022 22:05

h-vetinari changed the title ~~WIP: CMake fixes~~ CMake fixes Jul 31, 2022

h-vetinari marked this pull request as ready for review July 31, 2022 22:11

h-vetinari requested review from hmaarrfk, msarahan, rmax and xhochy as code owners July 31, 2022 22:11

hmaarrfk reviewed Jul 31, 2022

View reviewed changes

h-vetinari force-pushed the cmake branch from 8c7ad74 to 9802462 Compare August 1, 2022 08:51

h-vetinari and others added 11 commits August 1, 2022 11:33

remove fake variable

f749b06

try to specify static links at compile time

87e308f

output cmake files for inspectjon

cf3b62a

uncouple outputs

f3cc8dc

sort & complete tests

8d075f5

cannot build CLI without shared libs

49e3cbc

run cmake --find-package as test

4011ff8

clean up bld.bat

f6fa361

don't build static lib on windows

246a0b2

disable cmake test on windows for now

7c294af

link CLI against shared builds on windows

22966cd

https://github.com/facebook/zstd/blob/v1.5.2/build/cmake/CMakeLists.txt#L144-L152

h-vetinari force-pushed the cmake branch from 2ece626 to 22966cd Compare August 1, 2022 09:33

h-vetinari added a commit to h-vetinari/llvmdev-feedstock that referenced this pull request Aug 1, 2022

work around broken zstd

0ae847d

wait for conda-forge/zstd-feedstock#62

h-vetinari added a commit to h-vetinari/llvmdev-feedstock that referenced this pull request Aug 1, 2022

work around broken zstd

d8aa104

wait for conda-forge/zstd-feedstock#62

hmaarrfk merged commit 9aca7b1 into conda-forge:main Aug 8, 2022

h-vetinari deleted the cmake branch August 8, 2022 11:30

h-vetinari mentioned this pull request Aug 8, 2022

General setup of static outputs vs. shared ones conda-forge/conda-forge.github.io#1809

Open

h-vetinari mentioned this pull request Aug 11, 2022

re-add output coupling #63

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake fixes #62

CMake fixes #62

h-vetinari commented Jul 31, 2022

conda-forge-linter commented Jul 31, 2022

hmaarrfk commented Jul 31, 2022

h-vetinari commented Jul 31, 2022

hmaarrfk Jul 31, 2022

h-vetinari Jul 31, 2022 •

edited

Loading

h-vetinari Jul 31, 2022 •

edited

Loading

h-vetinari Aug 1, 2022

h-vetinari commented Aug 1, 2022 •

edited

Loading

hmaarrfk commented Aug 1, 2022

h-vetinari commented Aug 1, 2022

h-vetinari commented Aug 3, 2022

h-vetinari commented Aug 6, 2022

hmaarrfk commented Aug 6, 2022

h-vetinari commented Aug 6, 2022

ocefpaf commented Aug 6, 2022

h-vetinari commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

h-vetinari commented Aug 8, 2022 •

edited

Loading

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

CMake fixes #62

CMake fixes #62

Conversation

h-vetinari commented Jul 31, 2022

conda-forge-linter commented Jul 31, 2022

hmaarrfk commented Jul 31, 2022

h-vetinari commented Jul 31, 2022

hmaarrfk Jul 31, 2022

Choose a reason for hiding this comment

h-vetinari Jul 31, 2022 • edited Loading

Choose a reason for hiding this comment

h-vetinari Jul 31, 2022 • edited Loading

Choose a reason for hiding this comment

h-vetinari Aug 1, 2022

Choose a reason for hiding this comment

h-vetinari commented Aug 1, 2022 • edited Loading

Current status (before this PR):

Status of this PR:

Future?

hmaarrfk commented Aug 1, 2022

h-vetinari commented Aug 1, 2022

h-vetinari commented Aug 3, 2022

h-vetinari commented Aug 6, 2022

hmaarrfk commented Aug 6, 2022

h-vetinari commented Aug 6, 2022

ocefpaf commented Aug 6, 2022

h-vetinari commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

Footnotes

h-vetinari commented Aug 8, 2022 • edited Loading

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

hmaarrfk commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

h-vetinari commented Aug 8, 2022

h-vetinari Jul 31, 2022 •

edited

Loading

h-vetinari Jul 31, 2022 •

edited

Loading

h-vetinari commented Aug 1, 2022 •

edited

Loading

h-vetinari commented Aug 8, 2022 •

edited

Loading