Skip to content

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Jan 7, 2026

Rationale for this change

After commit cbd48a4 migrated OpenTelemetry from ExternalProject to FetchContent, the documentation build started failing with symbol redefinition errors:

error: redefinition of 'const google::protobuf::internal::MigrationSchema schemas []'

FetchContent integrates OpenTelemetry into the parent CMake configuration, causing it to inherit CMAKE_UNITY_BUILD=ON. Unity builds combine multiple .pb.cc files into a single compilation unit, which causes collision of static symbols (like schemas[] and file_default_instances[]) that exist in each protobuf-generated file.

What changes are included in this PR?

Added set(CMAKE_UNITY_BUILD FALSE) in the build_opentelemetry() function in cpp/cmake_modules/ThirdpartyToolchain.cmake to disallow Unity build in protobuf-generated files.

Are these changes tested?

Has to be tested via CI.

Are there any user-facing changes?

No, dev-only.

@github-actions
Copy link

github-actions bot commented Jan 7, 2026

⚠️ GitHub issue #48750 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added the awaiting review Awaiting review label Jan 7, 2026
@HyukjinKwon
Copy link
Member Author

@github-actions crossbow submit test-debian-*-docs

@github-actions
Copy link

github-actions bot commented Jan 7, 2026

Revision: 5cbafac

Submitted crossbow builds: ursacomputing/crossbow @ actions-71a6b31f76

Task Status
test-debian-12-docs GitHub Actions

@kou
Copy link
Member

kou commented Jan 7, 2026

Can we use this instead?

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 8a26b46d0b..d25aa01b62 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -3724,6 +3724,8 @@ function(build_opentelemetry)
 
   prepare_fetchcontent()
 
+  # TODO: Add the reason why we need to disable unity build...
+  set(CMAKE_UNITY_BUILD FALSE)
   set(OTELCPP_PROTO_PATH "${opentelemetry_proto_SOURCE_DIR}")
   set(WITH_EXAMPLES OFF)
   set(WITH_OTLP_HTTP ON)

@HyukjinKwon
Copy link
Member Author

Ohh let me try!

@HyukjinKwon
Copy link
Member Author

@github-actions crossbow submit test-debian-*-docs

@github-actions
Copy link

github-actions bot commented Jan 7, 2026

Revision: 5703dc4

Submitted crossbow builds: ursacomputing/crossbow @ actions-3921253546

Task Status
test-debian-12-docs GitHub Actions

@HyukjinKwon HyukjinKwon changed the title GH-48750: [CI][Documentation] Disable Unity builds to fix OpenTelemetry symbol collision in docs build GH-48750: [CI][Documentation] Disable Unity build for OpenTelemetry Jan 7, 2026
@raulcd
Copy link
Member

raulcd commented Jan 7, 2026

@github-actions crossbow submit preview-docs

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change! LGTM.
I am triggering the preview-docs job which seems to fail with the same issue to validate, will merge once the job finishes

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Jan 7, 2026
@github-actions
Copy link

github-actions bot commented Jan 7, 2026

Revision: 5703dc4

Submitted crossbow builds: ursacomputing/crossbow @ actions-0d86636019

Task Status
preview-docs GitHub Actions

@HyukjinKwon
Copy link
Member Author

seems passing fine 👍

@raulcd raulcd merged commit 4edefc3 into apache:main Jan 7, 2026
44 checks passed
@raulcd raulcd removed the awaiting merge Awaiting merge label Jan 7, 2026
@HyukjinKwon
Copy link
Member Author

Awesomeness!

@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 4edefc3.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants