Skip to content
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

[CMake Build] Install necessary header files only? #1980

Open
ThomsonTan opened this issue Feb 11, 2023 · 4 comments
Open

[CMake Build] Install necessary header files only? #1980

ThomsonTan opened this issue Feb 11, 2023 · 4 comments
Labels
bug Something isn't working do-not-stale good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers

Comments

@ThomsonTan
Copy link
Contributor

Follow up for #1932 (comment)

we install all headers when we run cmake --install now.In my understanding, is it better to select a subset of headers to install(all exported classes and their dependencies). Maybe we can also improve it in another PR.

For cmake, we install header files as below, @owent, do you think we should list the files to install explicitly instead of relying on pattern with wildcard?

install(
DIRECTORY include/opentelemetry
DESTINATION include
FILES_MATCHING
PATTERN "*.h"
PATTERN "metrics" EXCLUDE)

@ThomsonTan ThomsonTan added the bug Something isn't working label Feb 11, 2023
@lalitb lalitb added help wanted Good for taking. Extra help will be provided by maintainers good first issue Good for newcomers labels Feb 13, 2023
@lalitb
Copy link
Member

lalitb commented Feb 13, 2023

For API - need to install all headers
For SDK - need to check the public facing headers, and install accordingly.

PATTERN "metrics" EXCLUDE - Metrics header needn't be excluded.

@owent
Copy link
Member

owent commented Feb 14, 2023

Or can we add install() in CMakeLists.txt of each target?
For example in sdk/src/common/CMakeLists.txt:

 install( 
   DIRECTORY ${PROJECT_SOURCE_DIR}/sdk/include/opentelemetry/sdk/common
   DESTINATION include/opentelemetry/sdk/) 

@lalitb
Copy link
Member

lalitb commented Feb 14, 2023

Or can we add install() in CMakeLists.txt of each target?

Agree, that would be better approach.

@github-actions
Copy link

This issue was marked as stale due to lack of activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working do-not-stale good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

No branches or pull requests

3 participants