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

POC fix for GCC/Clang under Linux for header only singletons #1594

Closed
wants to merge 1 commit into from

Conversation

astitcher
Copy link
Contributor

Changes

This the minimal change that will work for GCC with Linux. This approach
won't work for Windows. MacOS is unknown but might work.

Fixes #1520

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

  • CHANGELOG.md updated for non-trivial changes
    • This is a very small perhaps trivial change
  • Unit tests have been added
  • Changes in public API reviewed
    • No changes to public API

This the minimal change that will work for GCC with Linux. This approach
won't work for Windows. MacOS is unknown but might work.
@astitcher astitcher requested a review from a team September 6, 2022 18:34
@astitcher
Copy link
Contributor Author

I have created this PR because this is the patch that is going to be used by the Fedora/RHEL rpm packages until this issue is resolved in an official release of opentelemetry-cpp.

I think this is a fine approach for Linux etc. and it is about the minimal change I could find. I'm completely fine with this not being merged though if there is a better approach that might work for Windows/Visual Studio too.

@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #1594 (e593a9c) into main (22e64e2) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1594      +/-   ##
==========================================
+ Coverage   84.96%   85.00%   +0.05%     
==========================================
  Files         156      156              
  Lines        4977     4977              
==========================================
+ Hits         4228     4230       +2     
+ Misses        749      747       -2     
Impacted Files Coverage Δ
api/include/opentelemetry/baggage/baggage.h 97.33% <100.00%> (ø)
...ntelemetry/context/propagation/global_propagator.h 100.00% <100.00%> (ø)
...pi/include/opentelemetry/context/runtime_context.h 97.60% <100.00%> (ø)
api/include/opentelemetry/metrics/provider.h 100.00% <100.00%> (ø)
api/include/opentelemetry/trace/provider.h 100.00% <100.00%> (ø)
api/include/opentelemetry/trace/trace_state.h 97.60% <100.00%> (ø)
sdk/src/trace/batch_span_processor.cc 90.63% <0.00%> (-0.78%) ⬇️
...dk/include/opentelemetry/sdk/metrics/instruments.h 0.00% <0.00%> (ø)
ext/src/http/client/curl/http_client_curl.cc 81.44% <0.00%> (+1.14%) ⬆️

@owent
Copy link
Member

owent commented Sep 7, 2022

Is this duplicated of #1595 ?

@marcalff
Copy link
Member

marcalff commented Sep 7, 2022

PR #1595 tests the api fix from this branch, with unit tests implemented by PR #1525

In general, the fix works for gcc and clang, with the exception of a crash seen when using the STL.

See CI / CMake C++20 test

Update:

the fix works with gcc and clang, verified by the unit test.
The WITH_STL segfault was a makefile bug, fixed in PR#1595

@astitcher
Copy link
Contributor Author

@marcalff Thank you for #1595. should I close this PR in favor of that one?

@marcalff
Copy link
Member

marcalff commented Sep 7, 2022

@marcalff Thank you for #1595. should I close this PR in favor of that one?

@astitcher ,

Thanks for also testing on your side.

The change for gcc and clang is the similar in PR#1595 and here, with added unit tests in 1595,
so yes, I don't see the need to keep this PR open.

PR#1595 contains more code for windows, but if this can not be made to work I will cleanup the extra ifdef when merging, so it will look like this PR here if only the gcc/clang fix is merged.

@astitcher
Copy link
Contributor Author

Closing this in favor of #1595

@astitcher astitcher closed this Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants