Skip to content

[BUILD] Propagate INTERFACE_COMPILE_DEFINITIONS from API through common_foo_library #3440

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 28, 2025
Merged

[BUILD] Propagate INTERFACE_COMPILE_DEFINITIONS from API through common_foo_library #3440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
04c609c
[BUILD] Propagate INTERFACE_COMPILE_DEFINITIONS from API through comm…
ThomsonTan May 23, 2025
4925109
Update PR number
ThomsonTan May 23, 2025
106dc93
Merge branch 'main' into fix_dll_examples
ThomsonTan May 23, 2025
66c07f6
Merge branch 'main' into fix_dll_examples
ThomsonTan May 23, 2025
35b7f87
Merge branch 'main' into fix_dll_examples
marcalff May 26, 2025
59d1fea
Merge branch 'main' into fix_dll_examples
marcalff May 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ Increment the:

## [Unreleased]

* [BUILD] Propagate INTERFACE_COMPILE_DEFINITIONS from API through common_foo_library
[#3440](https://github.com/open-telemetry/opentelemetry-cpp/pull/3440)

* [BUILD] Error out when building DLL without MSVC
[#3438](https://github.com/open-telemetry/opentelemetry-cpp/pull/3438)

Expand Down
2 changes: 1 addition & 1 deletion examples/common/foo_library/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@ endif()

target_include_directories(common_foo_library
PUBLIC "$<BUILD_INTERFACE:${EXAMPLES_COMMON_DIR}>")
target_link_libraries(common_foo_library PRIVATE opentelemetry-cpp::api)
Copy link
Member

Choose a reason for hiding this comment

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

The example common foo library headers don't expose otel-cpp headers and that seems to be an important aspect of the example (user instrumentation of a library shouldn't leak otel-cpp if it doesn't need to).

If the executable linking to the dll target opentelemetry-cpp::opentelemetry_cpp isn't getting the definitions it needs from the api target, then it points to a missing link by the dll target.

Can you try adding target_link_libraries(opentelemetry_cpp INTERFACE opentelemetry_api) to the dll's cmake file?

opentelemetry-cpp/ext/src/dll/CMakeLists.txt

Line 7 in 4b6f52a

add_library(opentelemetry_cpp SHARED dllmain.cc ${OPENTELEMETRY_EXPORT_DEF})

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion whether the original fix from @ThomsonTan or the suggestion from @dbarker is the best solution.

Please resolve the code review, so a fix we all agree with can go in the repo.

This is blocking for the next release 1.21.0, as we want to make sure that examples work properly with a consistent build, it affects people trying / evaluating opentelemetry-cpp.

Copy link
Member

Choose a reason for hiding this comment

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

@marcalff Okay to merge now to fix the new examples build and unblock the release.

Users who link to the dll target in their projects will need to explicitly link the API target to get its declarations until we add the API link to the dll target. This can be done in the next release since the issue is not new

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the suggestion from @dbarker with target_link_libraries(opentelemetry_cpp INTERFACE opentelemetry_api) could fix the issue, but I suspect the same issue could happen to non-DLL case, probably somehow there is some fallback for the linking in the same executable.

I also suggest we merge the current PR and tune the fix later (after the release), because the current PR restores the previous behavior.

File a new issue to track the follow-up #3447.

target_link_libraries(common_foo_library PUBLIC opentelemetry-cpp::api)
2 changes: 1 addition & 1 deletion examples/common/logs_foo_library/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@ endif()
target_include_directories(common_logs_foo_library
PUBLIC $<BUILD_INTERFACE:${EXAMPLES_COMMON_DIR}>)

target_link_libraries(common_logs_foo_library PRIVATE opentelemetry-cpp::api)
target_link_libraries(common_logs_foo_library PUBLIC opentelemetry-cpp::api)
2 changes: 1 addition & 1 deletion examples/common/metrics_foo_library/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@ endif()
target_include_directories(common_metrics_foo_library
PUBLIC $<BUILD_INTERFACE:${EXAMPLES_COMMON_DIR}>)

target_link_libraries(common_metrics_foo_library PRIVATE opentelemetry-cpp::api)
target_link_libraries(common_metrics_foo_library PUBLIC opentelemetry-cpp::api)
Loading