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

Exclude header files for metrics from header-only cmake targets #806

Merged
merged 5 commits into from
May 28, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented May 27, 2021

Changes

If metrics is disabled during cmake build ( i.e, WITH_METRICS_PREVIEW=OFF ), the two header-only targets - opentelemetry-api and opentelemetry-sdk still contain the metrics-related header files. This should be removed.

After this fix:

$ cmake .. -DBUILD_TESTING=OFF -DWITH_METRICS_PREVIEW=OFF && make && make install
...
$ ls /usr/local/include/opentelemetry/
baggage  common  config.h  context  detail  exporters  ext  logs  nostd  plugin  sdk  std  trace  version.h
$ ls /usr/local/include/opentelemetry/sdk/
common  instrumentationlibrary  logs  resource  trace  version
$
$ cmake .. -DBUILD_TESTING=OFF -DWITH_METRICS_PREVIEW=ON && make && make install
...
$ ls /usr/local/include/opentelemetry/
baggage  common  config.h  context  detail  exporters  ext  logs  metrics  nostd  plugin  sdk  std  trace  version.h
$ ls /usr/local/include/opentelemetry/sdk/
common  instrumentationlibrary  logs  metrics  resource  trace  version
$ 

Please provide a brief description of the changes here.

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

@lalitb lalitb requested a review from a team May 27, 2021 14:16
@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #806 (95fd94c) into main (2734b78) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #806   +/-   ##
=======================================
  Coverage   96.00%   96.00%           
=======================================
  Files         176      176           
  Lines        7183     7183           
=======================================
  Hits         6896     6896           
  Misses        287      287           

api/CMakeLists.txt Outdated Show resolved Hide resolved
@lalitb lalitb merged commit 12e56f9 into open-telemetry:main May 28, 2021
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.

2 participants