[POC RFC] header only API for singletons (v2)

[marcalff]

Fix header only API for singletons (#1520)

Changes

Alternate solution, test annotation on methods instead of members

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]

The first commit fails 18 checks, which is actually good.

This commit only contains the unit test, which fails, showing that the unit test logic is working.

[codecov[bot]]

Codecov Report

Merging #1595 (4d11eac) into main (82a8115) will increase coverage by 0.07%. The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1595      +/-   ##
==========================================
+ Coverage   85.24%   85.30%   +0.07%     
==========================================
  Files         156      156              
  Lines        4978     4978              
==========================================
+ Hits         4243     4246       +3     
+ Misses        735      732       -3     

Impacted Files	Coverage Δ
api/include/opentelemetry/baggage/baggage.h	97.35% <ø> (ø)
...ntelemetry/context/propagation/global_propagator.h	100.00% <ø> (ø)
...pi/include/opentelemetry/context/runtime_context.h	97.60% <ø> (ø)
api/include/opentelemetry/metrics/provider.h	100.00% <ø> (ø)
api/include/opentelemetry/trace/provider.h	100.00% <ø> (ø)
api/include/opentelemetry/trace/trace_state.h	97.60% <ø> (ø)
ext/src/http/client/curl/http_client_curl.cc	81.44% <0.00%> (+1.14%)	:arrow_up:

[astitcher]

I think that the only way to make Windows dll work is to have the GetXxxx() singleton accessor member functions in their own library. At least in my windows testing __declspec(selectany) with member data didn't work to deduplicate the singleton across the executable and the dll. One approach to this might be to pull these members out of the class definitions and make them inline in the header files for Linux etc. but #ifdef the definitions for Windows and include them in a source file there.

[marcalff]

See file api/include/opentelemetry/common/macros.h for the technical solution

Status as of Sept 7:

• fix for gcc and clang works for linux and macos.
• the unit test added, singleton_test, builds, and passes, for linux and macos.
• the unit test builds for windows, links properly with a mix of .LIB and .DLL, and fails at runtime.
• the previous issue (segfault) affecting WITH_STL was due to a makefile issue in CMake, and is fixed.

So, at this point, we can:

• either continue to investigate on windows, to have a full solution
• or merge a partial fix for non windows platforms, to be amended later.

I don't have access to a windows machine, and can not debug, so help is most welcome.

Testing is needed on windows to see if the fix can at least work with .LIB alone.

Testing is also needed to see if gcc/clang on windows can work, and see if changes are needed in the macros.h header for that.

If it turns out there is no solution for windows, then the only option will be to have a windows DLL for the api singletons, which is causing other issues.

[owent]

I think that the only way to make Windows dll work is to have the GetXxxx() singleton accessor member functions in their own library. At least in my windows testing __declspec(selectany) with member data didn't work to deduplicate the singleton across the executable and the dll. One approach to this might be to pull these members out of the class definitions and make them inline in the header files for Linux etc. but #ifdef the definitions for Windows and include them in a source file there.

Could you please show the codes about the not working __declspec(selectany) above? I have written some small samples at https://github.com/open-telemetry/opentelemetry-cpp/issues/1520#issuecomment-1198104171 , and in those codes selectany works well with MSVC, but don't work only with gcc and clang on Windows.

[owent]

See file api/include/opentelemetry/common/macros.h for the technical solution

Status as of Sept 7:

• fix for gcc and clang works for linux and macos.
• the unit test added, singleton_test, builds, and passes, for linux and macos.
• the unit test builds for windows, links properly with a mix of .LIB and .DLL, and fails at runtime.
• the previous issue (segfault) affecting WITH_STL was due to a makefile issue in CMake, and is fixed.

So, at this point, we can:

• either continue to investigate on windows, to have a full solution
• or merge a partial fix for non windows platforms, to be amended later.

I don't have access to a windows machine, and can not debug, so help is most welcome.

Testing is needed on windows to see if the fix can at least work with .LIB alone.

Testing is also needed to see if gcc/clang on windows can work, and see if changes are needed in the macros.h header for that.

If it turns out there is no solution for windows, then the only option will be to have a windows DLL for the api singletons, which is causing other issues.

Agree to merge the visibility changes for unix like system first, and we can continue to find a better solution for Windows later.

I can help to test it some times later, the problem may be the dll can not be found by exe. Unlike Linux, the executable on Windows do not have a RPATH/RUNPATH like information which contain a search path of dependency shared library. It only search dll in PATH environment and the directory of executable file. I will verify if it's this problem later.

[astitcher]

* either continue to investigate on windows, to have a full solution

* or merge a partial fix for non windows platforms, to be amended later.

I would really like to get a solution for non windows into a release so we can create packages based on an actual opentelemetry-cpp release rather than taking one and patching it.

[astitcher]

I don't have access to a windows machine, and can not debug, so help is most welcome.

Testing is needed on windows to see if the fix can at least work with .LIB alone.

I'll try to find some time to test this with just static linking on Windows and to try to at least build this shared on Windows.

If it turns out there is no solution for windows, then the only option will be to have a windows DLL for the api singletons, which is causing other issues.

My expectation is that this is unfortunately going to be the option that will we will end up with for Windows DLLs.

[owent]

See file api/include/opentelemetry/common/macros.h for the technical solution Status as of Sept 7:

• fix for gcc and clang works for linux and macos.
• the unit test added, singleton_test, builds, and passes, for linux and macos.
• the unit test builds for windows, links properly with a mix of .LIB and .DLL, and fails at runtime.
• the previous issue (segfault) affecting WITH_STL was due to a makefile issue in CMake, and is fixed.

So, at this point, we can:

• either continue to investigate on windows, to have a full solution
• or merge a partial fix for non windows platforms, to be amended later.

I don't have access to a windows machine, and can not debug, so help is most welcome. Testing is needed on windows to see if the fix can at least work with .LIB alone. Testing is also needed to see if gcc/clang on windows can work, and see if changes are needed in the macros.h header for that. If it turns out there is no solution for windows, then the only option will be to have a windows DLL for the api singletons, which is causing other issues.

Agree to merge the visibility changes for unix like system first, and we can continue to find a better solution for Windows later.

I can help to test it some times later, the problem may be the dll can not be found by exe. Unlike Linux, the executable on Windows do not have a RPATH/RUNPATH like information which contain a search path of dependency shared library. It only search dll in PATH environment and the directory of executable file. I will verify if it's this problem later.

Sorry, but I found __declspec(selectany) doesn't work when linking the .lib of a shared library on Windows, and I didn't find a way to let cmake link the .dll file. I have update https://github.com/open-telemetry/opentelemetry-cpp/issues/1520#issuecomment-1198104171 to record this case. Maybe can we document this and find a better way in the future?

BTW: target_link_libraries(component_a opentelemetry_api) , target_link_libraries(component_b opentelemetry_api) ... is required to let component_a ... component_f to inherit all the include directory and macros from opentelemetry_api (NOMINMAX, HAVE_GSL, HAVE_ABSEIL and etc.).

[marcalff]

@owent

Sorry, but I found __declspec(selectany) doesn't work when linking the .lib of a shared library on Windows, and I didn't find a way to let cmake link the .dll file. I have update #1520 (comment) to record this case. Maybe can we document this and find a better way in the future?

To clarify my understanding: So this does not work for DLL, but because of makefile issues with cmake. When using the proper link commands manually, the binary works for DLL ?

Does the fix work with simply static libraries (.LIB), for component_a and component_b alone ?

If so, at least we will have a solution for windows static libs only, which is better than none.

Thanks.

[owent]

@owent

Sorry, but I found __declspec(selectany) doesn't work when linking the .lib of a shared library on Windows, and I didn't find a way to let cmake link the .dll file. I have update #1520 (comment) to record this case. Maybe can we document this and find a better way in the future?

To clarify my understanding: So this does not work for DLL, but because of makefile issues with cmake. When using the proper link commands manually, the binary works for DLL ?

Does the fix work with simply static libraries (.LIB), for component_a and component_b alone ?

If so, at least we will have a solution for windows static libs only, which is better than none.

Thanks.

When using the proper link commands manually, the binary works for DLL ?

Yes.

Does the fix work with simply static libraries (.LIB), for component_a and component_b alone ?

Static libraries (.LIB) do not has a standalone heap. I think it works well as before.

[lalitb]

Does the fix work with simply static libraries (.LIB), for component_a and component_b alone ?

Static libraries (.LIB) do not has a standalone heap. I think it works well as before.

Yes, static libraries should work fine for all the platforms in the existing setup.

[marcalff]

Status:

This PR, 1595, is kept open to continue investigation for windows.

The fix for gcc and clang, which is working, is to be merged in PR #1604.

[marcalff]

Added the help wanted tag.

Not a windows expert, so anyone with more knowledge is welcome to investigate, either using this patch as a base (if it helps) or proposing a different solution.