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

Add OPENTELEMETRY_INSTALL to allow user to skip install targets. #2022

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

owent
Copy link
Member

@owent owent commented Mar 7, 2023

Fixes #2015

Changes

  • Add OPENTELEMETRY_INSTALL to allow user to skip install targets.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@owent owent requested a review from a team March 7, 2023 02:52
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #2022 (1ae78af) into main (0b72b03) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2022   +/-   ##
=======================================
  Coverage   87.32%   87.32%           
=======================================
  Files         166      166           
  Lines        4723     4723           
=======================================
  Hits         4124     4124           
  Misses        599      599           

Signed-off-by: owent <[email protected]>
Copy link
Member

@marcalff marcalff left a 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.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@lalitb lalitb merged commit 2b08db2 into open-telemetry:main Mar 7, 2023
@ThomsonTan ThomsonTan mentioned this pull request Mar 7, 2023
3 tasks
Comment on lines +121 to +124
if(NOT CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
set(OPENTELEMETRY_INSTALL_default ON)
else()
set(OPENTELEMETRY_INSTALL_default OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

(Maybe this is too late now, but) Do you think this should default to ON in all cases for backwards compatibility?

Do you think the new feature should be listed in INSTALL.md?

Some of the available cmake build variables we can use during cmake
configuration:
- `-DCMAKE_POSITION_INDEPENDENT_CODE=ON` : Please note that with default
configuration, the code is compiled without `-fpic` option, so it is not
suitable for inclusion in shared libraries. To enable the code for
inclusion in shared libraries, this variable is used.
- `-DBUILD_SHARED_LIBS=ON` : To build shared libraries for the targets.
Please refer to note [below](#building-shared-libs-for-windows) for
Windows DLL support.
- `-DWITH_OTLP=ON` : To enable building OTLP exporter.
- `-DWITH_PROMETHEUS=ON` : To enable building prometheus exporter.

Copy link
Member

@lalitb lalitb Mar 8, 2023

Choose a reason for hiding this comment

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

(Maybe this is too late now, but) Do you think this should default to ON in all cases for backwards compatibility?

True, somehow missed this during review. Not sure if reverting back would be good idea as the release is already done now. We can mention this in release summary as breaking change for builds cc @ThomsonTan ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CMake option to skip export and install
4 participants