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 CMake OTELCPP_MAINTAINER_MODE #1650

Merged
merged 33 commits into from
Oct 15, 2022

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Oct 3, 2022

Fixes #1648

Changes

CMake OTELCPP_MAINTAINER_MODE

Add a new CMake option, OTELCPP_MAINTAINER_MODE.

Option OTELCPP_MAINTAINER_MODE=ON is supported for:

  • GCC
  • CLANG
  • MSVC

and causes the build to fail on warnings (-Wall -Werror).

This option is useful for maintainers, when testing a build locally.

New CI builds

Add new builds in Github [CI]

  • GCC (maintainer mode)
  • CLANG (maintainer mode)
  • MSVC (maintainer mode)

These build use OTELCPP_MAINTAINER_MODE=ON,
to enforce the build is warnings free for all compilers.

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

Add a MAINTAINER_MODE in CMake.

Build with gcc and clang in maintainer mode in github CI.
@marcalff marcalff requested a review from a team October 3, 2022 14:47
@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

Merging #1650 (97ad7ae) into main (89eb1ec) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1650      +/-   ##
==========================================
- Coverage   85.82%   85.77%   -0.05%     
==========================================
  Files         169      169              
  Lines        5133     5134       +1     
==========================================
- Hits         4405     4403       -2     
- Misses        728      731       +3     
Impacted Files Coverage Δ
...dk/metrics/exemplar/histogram_exemplar_reservoir.h 81.25% <100.00%> (+1.25%) ⬆️
ext/src/http/client/curl/http_client_curl.cc 80.31% <0.00%> (-1.13%) ⬇️

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@marcalff
Copy link
Member Author

marcalff commented Oct 3, 2022

To @open-telemetry/cpp-approvers

This PR adds three new builds in CI:

  • CI / CMake gcc (maintainer mode)
  • CI / CMake clang (maintainer mode)
  • CI / CMake msvc (maintainer mode)

These new build actually fail, which is the point: find additional warnings in the code.

Failure seen by GCC:

[ 55%] Building CXX object sdk/test/trace/CMakeFiles/tracer_test.dir/tracer_test.cc.o
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/test/trace/tracer_test.cc: In member function ‘virtual opentelemetry::v1::sdk::trace::SamplingResult MockSampler::ShouldSample(const opentelemetry::v1::trace::SpanContext&, opentelemetry::v1::trace::TraceId, opentelemetry::v1::nostd::string_view, opentelemetry::v1::trace::SpanKind, const opentelemetry::v1::common::KeyValueIterable&, const opentelemetry::v1::trace::SpanContextKeyValueIterable&)’:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/test/trace/tracer_test.cc:51:80: error: missing initializer for member ‘opentelemetry::v1::sdk::trace::SamplingResult::trace_state’ [-Werror=missing-field-initializers]
   51 |                       {{"sampling_attr1", 123}, {"sampling_attr2", "string"}}))};
      |                                                                                ^
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/test/trace/tracer_test.cc:57:29: error: missing initializer for member ‘opentelemetry::v1::sdk::trace::SamplingResult::attributes’ [-Werror=missing-field-initializers]
   57 |       return {Decision::DROP};
      |                             ^
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/sdk/test/trace/tracer_test.cc:57:29: error: missing initializer for member ‘opentelemetry::v1::sdk::trace::SamplingResult::trace_state’ [-Werror=missing-field-initializers]
cc1plus: all warnings being treated as errors

Failure seen by CLang:

[  7%] Building CXX object api/test/context/CMakeFiles/runtime_context_test.dir/runtime_context_test.cc.o
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/test/context/runtime_context_test.cc:4:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/context/runtime_context.h:7:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/context/context.h:7:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/context/context_value.h:8:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/baggage/baggage.h:8:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/common/kv_properties.h:6:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/common/key_value_iterable_view.h:10:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/common/key_value_iterable.h:6:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/common/attribute_value.h:10:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/nostd/variant.h:52:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/nostd/./internal/absl/types/variant.h:79:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/nostd/./internal/absl/types/../types/internal/variant.h:37:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/nostd/./internal/absl/types/../types/internal/../../types/bad_variant_access.h:78:2: error: extra ';' outside of a function is incompatible with C++98 [-Werror,-Wc++98-compat-extra-semi]
};
 ^
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/test/context/runtime_context_test.cc:4:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/context/runtime_context.h:7:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/context/context.h:7:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/context/context_value.h:8:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/baggage/baggage.h:8:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/common/kv_properties.h:273:28: error: no newline at end of file [-Werror,-Wnewline-eof]
OPENTELEMETRY_END_NAMESPACE
                           ^
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/test/context/runtime_context_test.cc:4:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/context/runtime_context.h:7:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/context/context.h:7:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/context/context_value.h:13:
In file included from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/trace/span.h:17:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/trace/span_metadata.h:43:28: error: no newline at end of file [-Werror,-Wnewline-eof]
OPENTELEMETRY_END_NAMESPACE
                           ^
3 errors generated.

This PR is ready for review.

Once more warnings are reported by the build, subsequent warning cleanups PR can be filed to fix the issues found, like #1649

Note that the new builds are not required to pass, so this will not block any work.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Moved grpc work around to protobuf_include_prefix.h
Disabled some warnings for windows.

Also, used windows-latest (2022)
@marcalff marcalff marked this pull request as draft October 5, 2022 12:51
@marcalff
Copy link
Member Author

marcalff commented Oct 5, 2022

Setting to draft:

Using this branch to check existing warnings,
and pushing fixes independently.

Once the main branch is clean, this can be then merged to main.

@marcalff marcalff changed the title Add a MAINTAINER_MODE in CMake [WIP] Add a OTELCPP_MAINTAINER_MODE in CMake Oct 5, 2022
@marcalff
Copy link
Member Author

Status:

As of 2022-10-10, remaining issues count is:

  • 66 errors with gcc
  • 19 errors with clang
  • 21 errors with msvc

@marcalff
Copy link
Member Author

Status:

As of 2022-10-12:

  • gcc build is clean
  • clang build is clean
  • 4 errors remaining with msvc

@marcalff marcalff changed the title [WIP] Add a OTELCPP_MAINTAINER_MODE in CMake [WIP] Add CMake OTELCPP_MAINTAINER_MODE Oct 13, 2022
@marcalff marcalff marked this pull request as ready for review October 13, 2022 23:47
@marcalff marcalff changed the title [WIP] Add CMake OTELCPP_MAINTAINER_MODE Add CMake OTELCPP_MAINTAINER_MODE Oct 13, 2022
Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Copy link
Member

@esigo esigo 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 PR :)

@marcalff
Copy link
Member Author

Thanks all for the review.

Ready to merge.

@lalitb lalitb merged commit fa5f9fc into open-telemetry:main Oct 15, 2022
ays7 added a commit to ays7/opentelemetry-cpp that referenced this pull request Oct 29, 2022
…ad-local-stack

* commit '9acde87b08b225ce511fa8a20c6cba14f2921518': (36 commits)
  Prepare v1.7.0 release with Metrics API/SDK GA. (open-telemetry#1721)
  Fix: 1712 -  Validate Instrument meta data (name, unit, description) (open-telemetry#1713)
  Document libthrift 0.12.0 doesn't work with Jaeger exporter (open-telemetry#1714)
  Fix:1674, Add Monotonic Property to Sum Aggregation, and unit tests for Up Down Counter (open-telemetry#1675)
  [Metrics SDK] Move Metrics Exemplar processing behind feature flag (open-telemetry#1710)
  [Metrics API/SDK] Change Meter API/SDK to return nostd::unique_ptr for Sync Instruments (open-telemetry#1707)
  [Metrics] Switch to explicit 64 bit integers (open-telemetry#1686)
  Add metrics e2e test to asan & tsan CI (open-telemetry#1670)
  Add otlp-grpc example bazel (open-telemetry#1708)
  [Metrics SDK] Add support for Pull Metric Reader (open-telemetry#1701)
  Fix debug log of OTLP HTTP exporter and ES log exporter (open-telemetry#1703)
  [SEMANTIC CONVENTIONS] Upgrade to version 1.14.0 (open-telemetry#1697)
  Fix a potential precision loss on integer in ReservoirCellIndexFor (open-telemetry#1696)
  fix Histogram crash (open-telemetry#1685)
  Fix:1676 Segfault when short export period is used for metrics  (open-telemetry#1682)
  Add timeout support to MeterContext::ForceFlush (open-telemetry#1673)
  Add CMake OTELCPP_MAINTAINER_MODE (open-telemetry#1650)
  [DOCS] - Minor updates to OStream Metrics exporter documentation (open-telemetry#1679)
  Fix:open-telemetry#1575 API Documentation for Metrics SDK and API (open-telemetry#1678)
  Fixes open-telemetry#249 (open-telemetry#1677)
  ...
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
@marcalff marcalff deleted the fix_wall_werr_1648 branch July 4, 2023 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Add a -Wall -Werror build
5 participants