GH-50648: [Packaging][Linux] Enable OpenTelemetry - #50650
Conversation
|
|
|
@github-actions crossbow submit -g wheel -g r *emscripten |
|
Revision: 9c7c84c Submitted crossbow builds: ursacomputing/crossbow @ actions-2a277f2dc1
|
|
@github-actions crossbow submit test-conda-python-emscripten |
|
Revision: dc0fb97 Submitted crossbow builds: ursacomputing/crossbow @ actions-446f8349e0
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cpp/src/arrow/ArrowConfig.cmake.in:85
- In this
if()branch, usingif(${dependency} STREQUAL "opentelemetry-cpp")is fragile because unquoted values can be re-interpreted by CMake’sif()expression rules (e.g., as variable/keyword tokens), which can lead to unexpected evaluation if the dependency list ever contains an empty element or a token that matches an existing variable/keyword. Quoting the expanded value makes the comparison robust.
if(${dependency} STREQUAL "opentelemetry-cpp")
find_dependency(${dependency}
COMPONENTS
exporters_ostream
exporters_otlp_http
|
Only Pyodide + Emscripten build is still failing... It's difficult to debug... Can we open a new issue for it and work on it as a separated task...? |
I am ok with opening a separate issue for emscripten. |
|
Thanks. I opened it as #50774. |
| set(AZURE_SDK_VENDORED | ||
| TRUE | ||
| PARENT_SCOPE) | ||
| list(PREPEND |
There was a problem hiding this comment.
All the prepends are because we were overriding?
There was a problem hiding this comment.
Hmm, no.
We build bundled dependencies from products that are depended upon (e.g. Abseil) to products that depend on them (e.g. Protobuf). If we use APPEND, ARROW_BUNDLED_STATIC_LIBS uses Abseil -> Protobuf order. In general, static linking command line uses Protobuf -> Abseil order to resolve Abseil symbols in Protobuf. But this may not be needed for libarrow_bundled_dependencies.a because it includes all symbols in one .a.
If we ensure using produces that are depended upon -> products that depend on them order, we can't use alphabetical order for all Abseil CMake targets (absl::*). But in this PR, I used alphabetical order for Abseil CMake targets because I don't want to check all dependency list. And it works.
Should we keep using APPEND because PREPEND may show that we want to use produces that are depended upon -> products that depend on them order?
There was a problem hiding this comment.
Thanks for the explanation. In my opinion using PREPEND makes sense here even if not strictly necessary. The benefit of not having to check all Abseil CMake targets is enough to justify. Maintaining reverse order based on our build dependency resolution (build_abseil, build_protobuf: Protobud -> Abseil) even though not being required makes sense to me.
github-actions
Bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit f3a0848. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…bstrait and ORC (#50792) ### Rationale for this change Fix #50774. After #50650, the Pyodide build failed while loading PyArrow with ` ImportError: dynamic module does not define module export function (PyInit_lib)`. The ImportError is misleading because the `PyInit_lib` is present. The actual failure is an earlier abort during static initialization of `libarrow_python.so`, in Protobuf descriptor registration. #50650 changed the order of libraries merged into `libarrow_bundled_dependencies.a`. The archive contains duplicate weak Protobuf/Abseil symbols emitted by the Protobuf runtime and generated code in Substrait and in ORC. These symbols had inconsistent visibility: a) Protobuf was compiled with *hidden* visibility, b) Substrait and ORC generated Protobuf code used default visibility, so the order change caused `wasm-ld` to select symbols with different visibility. (e.g. `google::protobuf::internal::ZeroFieldsBase::Clear()` changed from hidden in Protobuf's `empty.pb.cc.o` to default visible in Substrait's `algebra.pb.cc.o`) The failure showed up in Emscripten, no failures on other platforms, but matching visibility globally is desirable to prevent any unintended exports and symbol interposition. ### What changes are included in this PR? Compile bundled Substrait and ORC with the same hidden visibility for symbols as Protobuf so not depending on archive order. Also add debug for Emscripten CI builds `--profiling-funcs`. ### Are these changes tested? Yes, `test-conda-python-emscripten` passes (Local repro, debug and fix verification with `ARCH=amd64 archery docker run conda-python-emscripten`, for symbols `llvm-nm` and `wasm-ld`) ### Are there any user-facing changes? No. * GitHub Issue: #50774 Authored-by: Tadeja Kadunc <tadeja.kadunc@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
Debian GNU/Linux trixie or later and Ubuntu 26.04 or later provide opentelemetry-cpp-dev:
What changes are included in this PR?
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.