-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
WIP: Build with opentelemetry support #1048
base: main
Are you sure you want to change the base?
Conversation
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 ( |
Looks like either opentelemetry or arrow need an |
@h-vetinari You might get better error messages if you deactivate the unity builds. The error message triggers a gut feeling of "oh, maybe there is a macro defined twice". |
Interestingly, this gets much further now (didn't know that knob). It now fails with missing symbols while trying to create
The |
The Although it is internal, it is used across library boundaries and thus it needs to be exported. Thus should be addressed upstream. |
Good analysis @xhochy. The build now succeeded, but we run into something else when trying to load the library:
I haven't seen this before, but it seems there's some double-loading(?) of proto information into a protobuf-internal database? If so, it sounds like arrow shouldn't be re-loading the otel proto definitions? CC @lidavidm |
To top things off, it looks like there's a third potential source for this (at least in conda-forge), in addition to opentelemetry & arrow: https://github.com/conda-forge/proto-opentelemetry-proto-feedstock The opentelemetry lib includes this as a build dependency(?), which seems wrong somehow? Shouldn't it be a host-dependence? Also, if there's a need to match the proto-opentelemetry-proto version here, I think this should become a run-export of libopentelemetry-cpp, rather than duplicating the version ("0.19") here and then trying to keep it in sync? |
This happens if two libraries load the same protobuf definition in a different, conflicting version. We have the same problem with all the onnx-* packages. The workaround for this is build all (except at most one) with static protobuf so that each library gets its own protobuf (global) namespace. |
Thanks for the input!
Can we not ensure that they get the same protobuf definitions? There's even a specific feedstock for those. |
BTW: The recent commits also showed that the unity build was directly implicated in the windows failures. Switching it back on lead to failure again. Should I raise an issue for this upstream? |
The Protobuf definitions are only needed to generate code, not at runtime. But the version needs to be matched, so if there's a better way...
Hmm, something seems wrong. Arrow itself shouldn't be loading the OpenTelemetry Protobuf generated code. Either we actually built a bundled copy of OpenTelemetry (doesn't seem so) or we still statically linked OpenTelemetry somehow? (Shouldn't be possible?) Or possibly multiple OpenTelemetry libraries included the Protobuf generated code?
Yes, please do. |
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 ( I do have some suggestions for making it better though... For recipe/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12001056330. Examine the logs at this URL for more detail. |
No description provided.