-
-
Notifications
You must be signed in to change notification settings - Fork 282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
General setup of static outputs vs. shared ones #1809
Comments
Libraries should use a separate target name for their static and dynamic libs so the CMake files don't clobber each and there is no ambiguity about what is being linked? https://stackoverflow.com/a/2152157 |
Could also create more outputs of the recipe to avoid clobbering. i.e. run_export only libs (no headers or CMake files). |
I agree with this, however, it doesn't match current widespread practice...
Could you sketch your idea w.r.t. to the most important aspects of a |
Having static and shared builds in the same environment has been the standard in Unix environments for decades. I don't know what you mean by iffy here. Please understand that conda is not special and we should do well to learn from the past. In the case of zstd, the way you fixed is not ideal. What you should do is first build the static build, install and then do the shared lib only. CMake configs will support only shared. In my experience, projects supporting both static and shared libraries at the same time with cmake is rare. Most projects can build only one of those at the same time. In the case of micromamba, there is no mistake about using static libraries because it explicitly searches for .a files. In other build systems, they usually add |
By iffy I mean:
I think a feedstock should make a specific choice of depending on the shared or static builds of a given dependency, and then only depend on that. Having both in the same environment makes it much harder to tell what's happening under the hood.
Thanks for the feedback. Happy to find a better way to do it, which is why I opened this issue. Already in that issue I had sketched my preferred solutionwhich would be:
instead of the previous (== state of other static builds in conda-forge):
and the current (after conda-forge/zstd-feedstock#62):
IIUC correctly, only the shared builds would have CMake metadata. I dislike this because we'd be "lying" to any consuming feedstock that requests
I had no doubt that it was being used correctly there, but IMO we should have more "guard rails" for using these static lib. Or if we decide not to, rename them to have a leading underscore so that they're clearly marked as "here be dragons". In short, IMO:
|
We are not lying. there would be no
You are going into hypotheticals. Please show one package where having only the shared build in cmake will fail when a project like zstd is built like how I said. Otherwise this conversation is moot. |
Having only the shared target is what worried me, because a feedstock using CMake & containing something like
I gave two examples in the OP, zstd & libprotobuf (the latter has no cmake integration on unix yet). Lots of feedstocks that are consuming those are CMake-based. If any of those wants to use the static builds for whatever reason (and is not as exceedingly careful as micromamba), it would break. I think https://github.com/conda-forge/onnxruntime-feedstock would be a candidate (using CMake & depending on libprotobuf-static), for example.
I don't think this accurate (or fair). So far, the balance of pros/cons is IMO leaning in favour of changing (what are the benefits of the status quo, aside from not having to change something?); one more aspect against the status quo is that even your described solution has a higher integration cost because the build scripts between static/shared would become more involved (or requiring patches) to not pick up the static targets, whereas my proposal just needs |
No, zstd and libprotobuf are not examples. I mean downstream packages where this is needed. micromamba obviosuly doesn't care. So you only have the example of onnxruntime. Please go into detail about why onnxruntime depends on libprotobuf-static. |
No, it doesn't. You just need cmake {build, install}. No patches necessary. You just need to make libzstd-static depend on libzstd in your solution. |
I'm well-aware of that, I just said that any dependent package using CMake & wanting a static lib for any reason would be an example.
I feel this is moving the goal posts; previously you asked what would break, now you ask why that feedstock needs a static lib. That's a deliberation per feedstock, I don't know this case specifically, or if it could be removed now; but that's besides the point, because it's an example of what I was describing.
Could you sketch how to do this based on - for example - the zstd feedstock? I'm not sure how one would build the static lib (currently just using
How would the CMake files of |
Since onnxruntime works just fine now, why are you insisting that you need cmake files to mention static just for onnxruntime to build correctly? |
It currently uses the static lib, but does that without being confused by shared-only CMake files, because those don't exist yet due to conda-forge/libprotobuf-feedstock#68 (that issue needed upstream fixes first which have landed now, but adding the CMake integration in conda-forge for libprotobuf would run into the issue I'm describing here). But zooming out a bit - why is it controversial that projects might want to consume static libs through CMake? That's a pretty normal thing to be doing (granted, not in conda-forge due to CFEP-18, but the exceptions that exist shouldn't have to be barred from using CMake?)... Regarding examples, https://github.com/conda-forge/grpc-cpp-feedstock having to consume Footnotes
|
So, you are saying that it links statically right now without issue and as soon as shared only CMake files are added, it will link to shared library?
Because you give no examples of doing so, but want to change the status quo. A change in status quo needs to be done when there's a real reason and you are not giving any.
I don't have time to go through these. Please explain in detail how this works right now and how having shared only Cmake files is an issue.
Because of CFEP-18. We want to encourage shared builds. Since you are going into hypotheticals, let me do the same. In the case of onnxruntime, your logic there is faulty as well. What if onnxruntime picks up a dependency that depends on shared libprotobuf? Then onnxruntime cannot link against libprotobuf-static because of the conda package conflict that you are introducing artificially. |
Yes, if we do it like you propose (no CMake targets for the static lib), I believe that would happen. That adding CMake targets is beneficial for other reasons should not be in question I presume? (conda-forge/libprotobuf-feedstock#68 is almost 2 years old)
I think working CMake integration is not "not giving any [reason]", see also below.
Yes, I get that, and those feedstock that diverge don't do this for fun. Those are often tricky packages in the first place, and I don't understand what benefit the status quo has that justifies subtly breaking their ability to use native CMake integration.
I put this in the footnotes above already, but in short: per abseil version, we cannot have more than one shared lib, but we need at least three different ABIs (per C++ standard version). Static builds make up the missing ones, and cannot be co-installed (due to different ABI). It's a special case as I said, but it literally cannot be handled (safely/sanely) through a static-depending-on-shared setup.
Yes, I noted this in the OP, and explained how this is likely not a problem in practice, and a work-around if it turns out to be. Quoted again for convenience:
It's of course a fair question if this is actually more painful than not having working CMake integration, and one where I'll gladly admit defeat if there are many affected feedstocks. But at least it would make the failure immediately visible, and presumably entice those feedstock to try building against the shared lib (if their dependency can do it, why not they themselves as well). |
You are not making sense to me at all. How would this happen? |
Currently, the CMake invocation in onnxruntime falls back on other means to detect |
No, that's not how it works. I took the time to research into this (which you could have done too) and onnxruntime uses cmake to find protobuf. The native CMake files are what CMake calls CONFIG mode in find_package and CMake already has in built config files for supporting protobuf (MODULE mode). MODULE mode is the default and irrespective of the presence of the files that you call native CMake files, it will find the static protobuf files because of the option Any other examples where you "need" this? |
I don't know why this discussion needs to be so combative. I'm trying to solve (what I perceive to be) a genuine problem - we can disagree on how broken things are, or what the right solutions are, but can we turn down the intensity a bit? I said I don't know the onnxruntime - I used it as an example I had found. For me the main point is in principle, for you it's specific examples that justify change. We can also disagree about that. Still, I feel your investigation underscores my point - it took a non-trivial amount of effort by one of the most knowledgeable people in conda-forge to find out whether my (I'd say, at least) plausible scenario would become relevant (and what if CMake didn't have built-in detection like it has for protobuf, or if onnxruntime didn't use How is all this effort / complexity in understanding a recipe beneficial, when we could have a completely unambiguous setup where that mixing never even becomes a possibility? I understand that changing something is work and engenders certain risks, but I feel also the status quo should be less set in stone, especially when problems are identified. Beyond that, I still don't know how a patch-free build setup would look like where
|
Sure. Sorry about that. I urge you to take a couple of hours before replying so as to give the impression that you have thought about things before replying.
There are use-cases where having both static and shared libraries are needed. For example, if you want to link your program against openblas static library (for performance, ABI, etc), but still want to use numpy in your stack (which comes with openblas shared), your suggestion basically makes it impossible for their use-case. On the other hand, your use-case can be worked-around in the build.sh in any feedstock. So, it's a choice between,
You have the solution right there. Add |
Thank you all for this discussion as I learned a lot from it! As someone who ends up in combative conversations often because I am not good at this and I am too quick to respond, I just want to say I appreciate all your work, @h-vetinari, and all your feedback, @isuruf. @isuruf can we enlist you for a review/view in the abseil/grpc migration? |
Needed to be a run-dep as well (to get the headers), but yes, this works (though it's unclear to me which lib gets found by CMake; but I don't care that much anymore, as this approach seems to solve the most issues at once). |
Is the following an accurate summary of the discussion? No top-level package. Build shared first output. Build static second output. Static output depends on shared output in both run and host. Use tests to check that shared output excludes static libraries. Static CMake files probably clobber shared CMake files (depends on build config)? |
This matches my understanding. Thanks for the summary. |
Static builds are already an exception in conda-forge (see CFEP-18), but some do exist for specific use-cases. The micromamba feedstock is a good example of a user of static libs (presumably to stay "micro" and have no runtime dependencies).
Several feedstocks of this kind follow a pattern as follows:
This has some advantages & disadvantages:
libxzy
don't get picked up though for a-static
host dep, so reliance on the dynamic lib would be detected by going boom at runtimeThe CMake issue in particular is quite painful, because an ever-increasing number of packages in C/C++-land come with built-in CMake integration. For example, recent LLVM-builds failed on conda-forge/zstd-feedstock#58. There, I untangled the dependence in conda-forge/zstd-feedstock#62, but this has the following trade-off:
The lack of being able to co-install
libxyz
andlibxyz-static
would become problematic in the following scenario (quoting @hmaarrfk):In this case, that PR was merged (and the existing consumers are not affected by the run-export-induced conflict between dynamic & static libs), but now I've encountered the same in libprotobuf, and fixing conda-forge/libprotobuf-feedstock#68 is not possible without running into the same issue, hence I thought I'd open a wider discussion.
@hmaarrfk sketched out a different approach:
to which I said:
Summary
Proposal: Forbid co-installation of
libxyz-static
builds withlibxyz
This would solve the CMake issues (each output could have its own copy of the respective CMake files without risks of inconsistency or clobbering). The only downside would be that users of
-static
packages cannot depend on another package (saya
) which has been built against a shared version of the same lib. Since most of the static libs live pretty close to the bottom of the stack, I don't think this would be much of a problem. And even so, there would be a solution: build a (perhaps also static?) version ofa
againstlibxyz-static
.I think (users of) static libs are special enough that we can inflict this (hypothetical!) pain on them
Thoughts @conda-forge/core?
The text was updated successfully, but these errors were encountered: