-
Notifications
You must be signed in to change notification settings - Fork 511
Support build proto as shared lib on Windows #3714
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables building Protocol Buffer (protobuf) targets as shared libraries on Windows by implementing proper DLL export/import declarations. Previously, protobuf targets were forced to be static libraries on Windows, but this change adds the necessary infrastructure to support shared library builds across platforms.
Key changes:
- Added CMake helper functions for managing export/import declarations across different compilers and platforms
- Modified protobuf generation to include
dllexport_declparameter for proper Windows DLL support - Removed Windows-specific restriction forcing static library builds for protobuf targets
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmake/tools.cmake | Adds utility functions for generating platform and compiler-specific DLL export/import declarations |
| cmake/opentelemetry-proto.cmake | Integrates DLL declarations into protobuf targets, removes Windows static-only restriction, and adds formatting improvements |
| elseif(SunPro) | ||
| set(${OUTPUT_VARNAME} | ||
| "__global" | ||
| PARENT_SCOPE) |
Copilot
AI
Oct 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SunPro compiler check uses an undefined variable SunPro instead of checking CMAKE_CXX_COMPILER_ID. This condition will never be true. Change to elseif(CMAKE_CXX_COMPILER_ID STREQUAL \"SunPro\") to properly detect the SunPro compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this valid comment - we are doing this check correctly at line 283?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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 🚀 New features to boost your workflow:
|
|
My MSVC have LNK2001 unresolved external symbol for |
c45538c to
39314ef
Compare
01b3a7c to
1a4630c
Compare
|
|
||
| if(OTELCPP_PROTO_LIB_TYPE STREQUAL "SHARED_LIBRARY") | ||
| if(CMAKE_SYSTEM_NAME MATCHES "Windows|MinGW|WindowsStore") | ||
| # The codes generated by gRPC plugin do not support dll export/import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gRPC does not seem well supported when built as a windows dll. See https://chromium.googlesource.com/external/github.com/grpc/grpc/+/HEAD/BUILDING.md#windows_a-note-on-building-shared-libs-dlls
Should this be something the project tries to support and if so can it be a separate PR? It is unclear if we can/should support gRPC as a windows dll and test it in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve reviewed the gRPC-generated code and confirmed it doesn’t contain global variables, so forcing it to build as static libraries should be safe. However, this might confuse users, create many duplicate symbols, and increase linker workload, similar to #3620.
We should discuss whether to build the gRPC code as a DLL or keep it as static libraries. I can rarely attend the SIG meeting due to the time zone, so could we discuss this on GitHub or Slack, or could someone share the outcome of the discussion with me afterward?
| endforeach() | ||
| endfunction() | ||
|
|
||
| function(project_build_tools_set_export_declaration OUTPUT_VARNAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unclear what the impacts will be to platforms not currently testing in CI (including cross compilation which is not tested). Ideally we don't embed more platform specific information in these CMake files/functions.
Please consider making only the changes needed support opentelemetry-proto built as a windows dll and adding a specific test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes apply only to users who build otel-cpp components as separate libraries rather than a single all-in-one DLL (ext/src/dll). The CI jobs in ci/do_ci.ps1 that omit -DOPENTELEMETRY_BUILD_DLL=1 and use -DVCPKG_TARGET_TRIPLET=x64-windows already cover this configuration.
The new functions also align behavior between Unix-like systems and Windows. Issue #3665 is not Windows-specific. For example, if users link against shared protobuf libraries but build otel-cpp’s proto targets as static libraries on Unix-like systems, then use those targets in multiple shared libraries, symbol registration can conflict and cause runtime crashes. If they instead build otel-cpp’s proto targets as shared libraries with -fvisibility=hidden on Unix-like systems, linking fails due to missing symbols.
We can use project_build_tools_set_export_declaration to enforce default symbol visibility on Unix-like systems and to set the appropriate export declarations on Windows.
| opentelemetry_ext | ||
| # Links flags of opentelemetry_http_client_curl should be public when | ||
| # building internal components | ||
| "$<BUILD_INTERFACE:opentelemetry_proto>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share more about why this change is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some examples don’t include all required dependencies. To avoid unrelated changes in this PR, this is a temporary workaround. Can we optimize them in a separate PR?
|
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:
Separately we should investigate why the formatting tools are not applied consistently - is it possible the powershell scripts and .cmake files are not covered? |
0ad054e to
93cf490
Compare
| PROPERTIES CXX_INCLUDE_WHAT_YOU_USE "" | ||
| CXX_CLANG_TIDY "" | ||
| C_VISIBILITY_PRESET "default" | ||
| CXX_VISIBILITY_PRESET "default" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting visibility to "default" might export more symbols than intended. Is this intended? If yes, please document as comment why do we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is default value on Unix like system. And this is for users who change the default settings by -fvisibility=hidden, so it do not increase symbols than before.
Just like WINDOWS_EXPORT_ALL_SYMBOLS on Windows, we need something like dllexport_decl=XXX in protobuf, but gRPC do not support it now. we can optimize it when gRPC support this feature.
| # declarations. To work around this, we enable WINDOWS_EXPORT_ALL_SYMBOLS | ||
| # property to export all symbols when building a shared library. | ||
| set_target_properties(opentelemetry_proto_grpc | ||
| PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerned about this - this will increase the DLL size, export internal implementation details, and also potential symbol conflict - better to document these limitations as TODO to revisit in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments have been added. We can either use the static otel-cpp libraries directly or enable ext/src/dll, which bundles all .lib files into a single DLL.
With static otel-cpp libraries, each linked DLL/EXE gets its own copy of the symbols. Using ext/src/dll ensures there is only one copy of these symbols after linking, so this approach is no worse than before.
Once gRPC supports export declarations, we can further optimize this.
See also: googleapis/google-cloud-cpp#5849 and grpc/grpc#33032
…more comments for opentelemetry_proto and opentelemetry_proto_grpc
93cf490 to
f1cde89
Compare
|
@lalitb @ThomsonTan @dbarker We currently do not support 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 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? |
Fixes #3665
Fixes #3620
Changes
dllexport_declfor proto targets and can be built as shared on windows.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes