-
Notifications
You must be signed in to change notification settings - Fork 475
[INSTALL] Unify cmake install functions and dynamically set component dependencies #3368
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
[INSTALL] Unify cmake install functions and dynamically set component dependencies #3368
Conversation
…ies based on target library links
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3368 +/- ##
=======================================
Coverage 90.05% 90.05%
=======================================
Files 212 212
Lines 6932 6932
=======================================
Hits 6242 6242
Misses 690 690 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job. I have a few questions after a quick look.
It's a big PR and I will test it locally later.
cmake/otel-install-functions.cmake
Outdated
endforeach() | ||
|
||
configure_file( | ||
"${CMAKE_SOURCE_DIR}/cmake/templates/thirdparty-dependency-definitions.cmake.in" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to use the simular path as below? ("${CMAKE_CURRENT_LIST_DIR}/templates/thirdparty-dependency-definitions.cmake.in"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Your change is right. It should use CMAKE_CURRENT_LIST_DIR
to ensure the project builds/installs correctly if used as a submodule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit to use PROJECT_SOURCE_DIR here and where we set/get properties used for install. That should work better when this project is used as a submodule/subdirectory in a larger project.
…d package. Use PROJECT_SOURCE_DIR for storing properties and configuring files
I'm on holiday these days.Can I continue to review several day's later |
@owent - for when you get back from holiday :) I've added a new test using CMake's FetchContent module and updated INSTALL documentation to include this as a supported approach to incorporate |
…. Clean up cmake script debug and error messages
# COMPONENT to TARGET lists | ||
# ---------------------------------------------------------------------- | ||
|
||
@OTEL_COMPONENTS_TARGETS_BLOCK@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also generate the target list comments in cmake/templates/opentelemetry-cpp-config.cmake.in
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That is a good idea. What do you think about just removing the current comments listing targets and components then adding a comment pointing to the component-definitions.cmake
file installed? That file contains all the information - just not in an .rst format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a follow up issue. #3398
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent job. I have already tested in my project. It works well.
This says it all. Ok to merge. |
Yes, LGTM. I think we can continue to work on generating document comments in another PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix.
Fixes #3293 as followup to #3220
Changes
otel_add_component()
otel_install_components()
otel_install_cmake_config()
otel_install_thirdparty_definitions()
LINK_LIBRARIES
andINTERFACE_LINK_LIBRARIES
.opentelemetry-cpp::
alias targets to support install and unify the public cmake interface toopentelemetry-cpp
opentelemetry-cpp::<target>
names imported withfind_package
can now be used whenopentelmetry-cpp
is built directly as a submodule (add_subdirectory
) in a larger project.For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes