[POC RFC] header only API for singletons (Windows) #1595

marcalff · 2022-09-06T20:47:23Z

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

Add singleton_test unit test alone.

marcalff · 2022-09-06T21:40:38Z

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.

Testing alternate fix, with annotation on methods. Windows not covered.

codecov · 2022-09-06T21:56:22Z

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%)`	⬆️

owent · 2022-09-07T01:58:47Z

api/include/opentelemetry/common/macros.h

@@ -99,7 +99,8 @@
 #elif defined(__GNUC__)
 #  define OPENTELEMETRY_API_SINGLETON __attribute__((visibility("default")))
 #elif defined(_MSC_VER)
-#  define OPENTELEMETRY_API_SINGLETON
+/* Tentative fix */
+#  define OPENTELEMETRY_API_SINGLETON __declspec(selectany)


Just wondering why __declspec(selectany) is removed?Doesn't it work with MSVC on Windows?

@owent

__declspec(selectany) works when applied to a member.

This PR tested applying it to a method, which failed to compile.

See places where OPENTELEMETRY_API_SINGLETON is used in this patch,
which are different compared to PR#1525

Could we also add __declspec(selectany) to variables, so we can also support dll on Windows with MSVC?

@owent, see file macros.h in the latest push.

commit 74ec691 Author: WenTao Ou <[email protected]> Date: Thu Apr 7 14:06:59 2022 +0800 Move public definitions into `opentelemetry_api`. (open-telemetry#1314) Signed-off-by: owent <[email protected]> When using WITH_STL in cmake, HAVE_CPP_STDLIB must be defined for unit tests.

astitcher · 2022-09-07T14:00:48Z

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 · 2022-09-07T15:08:25Z

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 · 2022-09-07T15:35:07Z

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 #1520 (comment) , and in those codes selectany works well with MSVC, but don't work only with gcc and clang on Windows.

owent · 2022-09-07T15:43:44Z

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 · 2022-09-07T15:48:06Z

* 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 · 2022-09-07T15:51:36Z

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 · 2022-09-08T16:00:31Z

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 #1520 (comment) 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.).

owent · 2022-09-08T16:07:17Z

api/test/singleton/CMakeLists.txt

+include(GoogleTest)
+
+add_library(component_a STATIC component_a.cc)
+


target_link_libraries(component_a opentelemetry_api) is required here to import macros from opentelemetry_api by target_compile_definitions(opentelemetry_api INTERFACE ...) .
So do component_c, component_d, component_e and component_f.

Thanks, will fix.

marcalff · 2022-09-08T17:32:41Z

@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.

proper cmake dependency in opentelemetry-api

only test static libraries for windows.

owent · 2022-09-09T06:34:43Z

@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 · 2022-09-09T07:25:11Z

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.

testing CI with windows static lib, no annotation

marcalff · 2022-09-09T13:47:33Z

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 · 2022-10-06T10:13:46Z

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.

WIP

ef23861

Add singleton_test unit test alone.

marcalff requested a review from a team September 6, 2022 20:47

WIP

d231197

Testing alternate fix, with annotation on methods. Windows not covered.

marcalff added 2 commits September 7, 2022 00:35

Testing tentative fix for windows

7c52d4b

Disable unit test for windows

6a8f2ff

owent mentioned this pull request Sep 7, 2022

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

Closed

3 tasks

owent reviewed Sep 7, 2022

View reviewed changes

marcalff and others added 6 commits September 7, 2022 11:06

format

84d6bab

format

5de5b6a

Alternate implementation for windows.

cbf70a7

Fixed windows build break.

5aeeb74

Merge branch 'open-telemetry:main' into poc_api_singletons_1520_b

c6a8970

owent reviewed Sep 8, 2022

View reviewed changes

marcalff and others added 3 commits September 8, 2022 21:49

Fix code review comments,

556b592

proper cmake dependency in opentelemetry-api

Merge branch 'open-telemetry:main' into poc_api_singletons_1520_b

a4fa08e

Tentative, to test on CI:

b5b6552

only test static libraries for windows.

Tentative,

868b6b0

testing CI with windows static lib, no annotation

marcalff marked this pull request as draft September 9, 2022 14:48

marcalff mentioned this pull request Sep 12, 2022

[POC RFC] header only API for singletons #1525

Closed

3 tasks

Merge branch 'main' into poc_api_singletons_1520_b

4d11eac

marcalff added the help wanted Good for taking. Extra help will be provided by maintainers label Oct 6, 2022

marcalff changed the title ~~[POC RFC] header only API for singletons (v2)~~ [POC RFC] header only API for singletons (Windows) Oct 6, 2022

marcalff closed this Oct 27, 2022

marcalff deleted the poc_api_singletons_1520_b branch September 5, 2023 14:54

marcalff mentioned this pull request Feb 14, 2024

Header only singletons are not working properly for Windows #2534

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC RFC] header only API for singletons (Windows) #1595

[POC RFC] header only API for singletons (Windows) #1595

marcalff commented Sep 6, 2022

marcalff commented Sep 6, 2022

codecov bot commented Sep 6, 2022 •

edited

Loading

owent Sep 7, 2022

marcalff Sep 7, 2022

owent Sep 7, 2022

marcalff Sep 7, 2022

astitcher commented Sep 7, 2022

marcalff commented Sep 7, 2022 •

edited

Loading

owent commented Sep 7, 2022

owent commented Sep 7, 2022 •

edited

Loading

astitcher commented Sep 7, 2022

astitcher commented Sep 7, 2022

owent commented Sep 8, 2022 •

edited

Loading

owent Sep 8, 2022 •

edited

Loading

marcalff Sep 8, 2022

marcalff commented Sep 8, 2022

owent commented Sep 9, 2022 •

edited

Loading

lalitb commented Sep 9, 2022

marcalff commented Sep 9, 2022

marcalff commented Oct 6, 2022

		include(GoogleTest)

		add_library(component_a STATIC component_a.cc)

[POC RFC] header only API for singletons (Windows) #1595

[POC RFC] header only API for singletons (Windows) #1595

Conversation

marcalff commented Sep 6, 2022

Changes

marcalff commented Sep 6, 2022

codecov bot commented Sep 6, 2022 • edited Loading

Codecov Report

owent Sep 7, 2022

Choose a reason for hiding this comment

marcalff Sep 7, 2022

Choose a reason for hiding this comment

owent Sep 7, 2022

Choose a reason for hiding this comment

marcalff Sep 7, 2022

Choose a reason for hiding this comment

astitcher commented Sep 7, 2022

marcalff commented Sep 7, 2022 • edited Loading

owent commented Sep 7, 2022

owent commented Sep 7, 2022 • edited Loading

astitcher commented Sep 7, 2022

astitcher commented Sep 7, 2022

owent commented Sep 8, 2022 • edited Loading

owent Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

marcalff Sep 8, 2022

Choose a reason for hiding this comment

marcalff commented Sep 8, 2022

owent commented Sep 9, 2022 • edited Loading

lalitb commented Sep 9, 2022

marcalff commented Sep 9, 2022

marcalff commented Oct 6, 2022

codecov bot commented Sep 6, 2022 •

edited

Loading

marcalff commented Sep 7, 2022 •

edited

Loading

owent commented Sep 7, 2022 •

edited

Loading

owent commented Sep 8, 2022 •

edited

Loading

owent Sep 8, 2022 •

edited

Loading

owent commented Sep 9, 2022 •

edited

Loading