opentelemetry-cpp icon indicating copy to clipboard operation
opentelemetry-cpp copied to clipboard

Support build proto as shared lib on Windows

Open owent opened this issue 2 months ago • 4 comments

Fixes #3665 Fixes #3620

Changes

  • Set dllexport_decl for proto targets and can be built as shared on windows.

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

  • [x] CHANGELOG.md updated for non-trivial changes
  • [x] Unit tests have been added
  • [x] Changes in public API reviewed

owent avatar Oct 25 '25 02:10 owent

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 89.95%. Comparing base (9de3493) to head (3df0e30). :warning: Report is 11 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3714      +/-   ##
==========================================
+ Coverage   89.93%   89.95%   +0.02%     
==========================================
  Files         225      225              
  Lines        7158     7158              
==========================================
+ Hits         6437     6438       +1     
+ Misses        721      720       -1     

see 1 file with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Oct 25 '25 03:10 codecov[bot]

My MSVC have LNK2001 unresolved external symbol for __std_find_last_trivial_1 , __std_search_1 , __std_search_1 , __std_min_8i and many other STL's symbols, I will wait the next VS update and continue this work then.

owent avatar Oct 29 '25 12:10 owent

Thanks for the PR @owent! I've taken a quick pass and shared some feedback. It may be worth a SIG meeting discussion if the project should support windows dlls of protobuf and especially grpc given that projects warnings about unexpected behavior:

you've been warned that there are some important drawbacks and some things might not work at all or will be broken in interesting ways.

Separately we should investigate why the formatting tools are not applied consistently - is it possible the powershell scripts and .cmake files are not covered?

dbarker avatar Nov 16 '25 21:11 dbarker

@lalitb @ThomsonTan @dbarker There is another suggestion for https://github.com/open-telemetry/opentelemetry-cpp/pull/3714#discussion_r2536061020_ and https://github.com/open-telemetry/opentelemetry-cpp/pull/3714#discussion_r2536064869_ .

We currently do not support -DCMAKE_BUILD_TYPE=ON on Windows. A better approach would be to use a macro to define __attribute__((__dllexport__)), __attribute__((__dllimport__)), or __attribute__((visibility("default"))) for each component and apply this macro to all exported APIs. This would allow us to build each component as a standalone DLL. By adopting this method, there would be no need to manually run ext/src/dll/make_def.ps1 or maintain ext/src/dll/input.src. When building ext/src/dll with -DCMAKE_BUILD_TYPE=ON, it would only need to link all components of otel-cpp. Conversely, when -DCMAKE_BUILD_TYPE=OFF is used, all symbols can be exported using a single CMake function, simplifying the process of maintaining symbols in the future.

For example, we can add the following CMake utility function:

function(project_build_tools_set_default_library_declaration DEFINITION_VARNAME)
  if(BUILD_SHARED_LIBS OR ((CMAKE_SYSTEM_NAME STREQUAL "Windows") AND DEFINED OPENTELEMETRY_BUILD_DLL))
    project_build_tools_set_shared_library_declaration(${DEFINITION_VARNAME}
                                                       ${ARGN})
  else()
    project_build_tools_set_static_library_declaration(${DEFINITION_VARNAME}
                                                       ${ARGN})
  endif()
endfunction()

Then use it in opentelemetry_common as follows: project_build_tools_set_default_library_declaration(OPENTELEMETRY_SDK_COMMON_API opentelemetry_common).

Public functions can then be declared like this:

class GlobalLogHandler
{
public:
  OPENTELEMETRY_SDK_COMMON_API static nostd::shared_ptr<LogHandler> GetLogHandler() noexcept;

  OPENTELEMETRY_SDK_COMMON_API static void SetLogHandler(const nostd::shared_ptr<LogHandler> &eh) noexcept;

  // ...
};

What do you think of this approach? Should we open a new issue or discuss it further?
It touches many files, and I can complete the changes if we agree to proceed.

owent avatar Nov 29 '25 11:11 owent