-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Build shared libs, add run-export, register in global pinning #38
Comments
@lidavidm, any thoughts on this? |
Ah, sorry. All of these should be fixed, but I haven't had a chance to do much other than merge updates for this. |
Ok, this was mostly fixed as of #44. I'd still like to rename the library to something like But the opentelemetry stuff is distributed over quite a few feedstocks1 and I'm not sure if that's conflicting with something else. Thoughts @lidavidm @wjones127 @xhochy? Footnotes
|
renaming is a-ok by me. the clobbering is an unfortunate aspect of: some things want to just use opentelemetry as a producer/API-only, and don't want to pull in the actual implementation bits, only the headers. But the package itself isn't really designed with that in mind. So we're basically building it twice, and so things get clobbered. I'm not sure how best to deal with this. |
Maybe upstream would be open to splitting up the package further. |
Cool! Do you think we should rename the API output in parallel too? Looking at the open-telemetry org and its repos, we might want to keep the So perhaps:
(I'll also note that looking at the content of the package, it's effectively just a header-only package; a similar naming discussion is ongoing for boost currently, where we might use
How about not including the CMake metadata in the api package? I opened a PR: conda-forge/cpp-opentelemetry-api-feedstock#17 |
libopentelemetry-cpp/-cpp-api sounds good to me. The -api scheme is because that's what the package itself calls the header-only variant, not sure which is preferable but I don't mind libopentelemetry-cpp-header-only either. I think we still want the CMake metadata so that downstream packages can still |
I think it would be fine to sacrifice that. Adding another include directory is trivial, whereas CMake metadata contains a lot more metadata than just headers. |
Right, but then that means projects that depend on opentelemetry will have to adjust builds to work both against the conda-forge package and a 'regular' installation? I'd like to avoid that if possible, though I suppose that's just the nature of dependencies in C++. |
Hm, I tend to "live" in a world where upstream doesn't care about conda-forge (or barely), so to me it's completely fine if upstream continues working only with I just think that clobbering files is a bad packaging smell, not least because we'll constantly spam every user with warnings about that (but also in general). |
I would strongly support removing any clobbering as this can easily cause issues if you start to patch some of the files in only one feedstock and you cannot rely that the patch is correctly installed in the clobbering situation. |
Wouldn't we get around the clobbering problems if this package here depended on |
The problem (AFAICT) is that the content of the CMake target files will be different if it's installed as API-only or with the libs. So having it in host would not help. |
Ok! I'll defer to you and Uwe. I'll give the PR a review in a little bit. |
Merged (and I didn't squash this time!) |
Opened a PR for the rename here: #46 After this I think we should be good to add it to the global pinning. |
Hey all
I just noticed that arrow comes with an ability to build against opentelemetry, so it would be nice to have this lib ready for that use-case.
AFAICT from the various feedstocks floating around, this is the main one for the CPP lib, but it:
Those things would be the minimum necessary for arrow to build against this feedstock.
The text was updated successfully, but these errors were encountered: