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

[BUILD] Need fine-grained HAVE_CPP_STDLIB #2304

Merged
merged 24 commits into from
Sep 28, 2023

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Sep 11, 2023

Fixes #1071

Changes

Fixed the following issues with unit tests:

  • In nostd/span_test.cc:

    • the unit tests were expecting specific behaviors when contructing a span with a static extent, and providing the wrong element count.
    • According to item (2) in https://en.cppreference.com/w/cpp/container/span/span, this is undefined behavior.
    • Resolved by removing tests that expected undefined behavior, as the result is implementation dependent anyway (the STL implementation was not tested because of possible differences).
  • In url_parser_test:

    • The makefile with CMake did not depend on opentelemetry_api.
    • As a result, option WITH_STL was not honored per CMake settings, it was always unset
    • Fixed to always honor WITH_STL by adding the dependency on opentelemetry_api

Fixed namespace pollution in the API header files:

  • #define HAVE_SPAN is replaced by #define OPENTELEMETRY_HAVE_SPAN
  • #define HAVE_CPP_STDLIB is replaced by #define OPENTELEMETRY_STL_VERSION=<version>

Improved the granularity of the CMake WITH_STL option:

  • Before, WITH_STL was either a "all" (C++20, or C++17 with extra code) or nothing choice.
  • After, WITH_STL can gradually pick witch parts of the STL to use, with OFF, CXX11, CXX14, CXX17, CXX20 or CXX23 options.

Fixed compiling options pollution in CMake:

  • Before, the makefile could decide to set CMAKE_CXX_STANDARD to specific values, possibly interfering with existing applications makefiles and compiling options.
  • After, CMAKE_CXX_STANDARD is no longer overwritten. The CMAKE_CXX_STANDARD setting, if any, or the stdc++ compiler options, if any, provided by the caller are honored.

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

@marcalff marcalff requested a review from a team September 11, 2023 13:12
@marcalff marcalff marked this pull request as draft September 11, 2023 13:13
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #2304 (0c0b5c0) into main (5e96b80) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2304   +/-   ##
=======================================
  Coverage   87.45%   87.45%           
=======================================
  Files         199      199           
  Lines        5997     5997           
=======================================
  Hits         5244     5244           
  Misses        753      753           
Files Coverage Δ
api/include/opentelemetry/nostd/shared_ptr.h 100.00% <ø> (ø)
api/include/opentelemetry/nostd/span.h 90.00% <ø> (ø)
api/include/opentelemetry/nostd/string_view.h 97.96% <ø> (ø)
api/include/opentelemetry/nostd/unique_ptr.h 100.00% <ø> (ø)
api/include/opentelemetry/nostd/utility.h 83.34% <ø> (ø)
api/include/opentelemetry/nostd/variant.h 66.67% <ø> (ø)

api/CMakeLists.txt Outdated Show resolved Hide resolved
@marcalff marcalff marked this pull request as ready for review September 12, 2023 17:55
@marcalff marcalff added the pr:please-review This PR is ready for review label Sep 20, 2023
@marcalff marcalff mentioned this pull request Sep 25, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
api/CMakeLists.txt Outdated Show resolved Hide resolved
@marcalff marcalff merged commit ad626ce into open-telemetry:main Sep 28, 2023
46 checks passed
@marcalff marcalff deleted the fix_cpp_stdlib_1071 branch October 27, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please-review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need fine-grained HAVE_CPP_STDLIB
4 participants