Skip to content

[BUILD] Fix exported definitions when building DLL with STL#2387

Merged
lalitb merged 9 commits intoopen-telemetry:mainfrom
perhapsmaple:stl_dll_fix
Nov 1, 2023
Merged

[BUILD] Fix exported definitions when building DLL with STL#2387
lalitb merged 9 commits intoopen-telemetry:mainfrom
perhapsmaple:stl_dll_fix

Conversation

@perhapsmaple
Copy link
Copy Markdown
Contributor

@perhapsmaple perhapsmaple commented Oct 28, 2023 •
edited
Loading

Partially Fixes #2202

Changes

The function definitions for MeterSelectorFactory::Create and InstrumentSelectorFactory::Create are different for the nostd and stl builds, which causes linker errors when trying to build the DLL.

Cmake build WITH_STL=OFF:

?Create@MeterSelectorFactory@metrics@sdk@v1@opentelemetry@@SA?AV?$unique_ptr@VMeterSelector@metrics@sdk@v1@opentelemetry@@U?$default_delete@VMeterSelector@metrics@sdk@v1@opentelemetry@@@std@@@std@@Vstring_view@nostd@45@00@Z

?Create@InstrumentSelectorFactory@metrics@sdk@v1@opentelemetry@@SA?AV?$unique_ptr@VInstrumentSelector@metrics@sdk@v1@opentelemetry@@U?$default_delete@VInstrumentSelector@metrics@sdk@v1@opentelemetry@@@std@@@std@@W4InstrumentType@2345@Vstring_view@nostd@45@1@Z

CMake build WITH_STL=ON:

?Create@MeterSelectorFactory@metrics@sdk@v1@opentelemetry@@SA?AV?$unique_ptr@VMeterSelector@metrics@sdk@v1@opentelemetry@@U?$default_delete@VMeterSelector@metrics@sdk@v1@opentelemetry@@@std@@@std@@V?$basic_string_view@DU?$char_traits@D@std@@@7@00@Z

?Create@InstrumentSelectorFactory@metrics@sdk@v1@opentelemetry@@SA?AV?$unique_ptr@VInstrumentSelector@metrics@sdk@v1@opentelemetry@@U?$default_delete@VInstrumentSelector@metrics@sdk@v1@opentelemetry@@@std@@@std@@W4InstrumentType@2345@V?$basic_string_view@DU?$char_traits@D@std@@@7@1@Z

Introduced a compile definition OPENTELEMETRY_STL_VERSION to the DLL build process to export the correct definitions.

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

@perhapsmaple perhapsmaple requested a review from a team October 28, 2023 10:58
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 28, 2023 •
edited
Loading

Codecov Report

Merging #2387 (80f31c5) into main (758687c) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 80f31c5 differs from pull request most recent head 105c1a6. Consider uploading reports for the commit 105c1a6 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2387   +/-   ##
=======================================
  Coverage   87.43%   87.43%           
=======================================
  Files         199      199           
  Lines        6030     6030           
=======================================
  Hits         5272     5272           
  Misses        758      758
Files Coverage Δ
...entelemetry/sdk/metrics/view/instrument_selector.h 100.00% <ø> (ø)
...de/opentelemetry/sdk/metrics/view/meter_selector.h 100.00% <100.00%> (ø)

marcalff
marcalff reviewed Oct 28, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the analysis and fix, see comments.

Comment thread ext/src/dll/CMakeLists.txt Outdated
Comment thread ext/src/dll/opentelemetry_cpp.src Outdated
marcalff
marcalff approved these changes Oct 29, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix.

Waiting for approval from @ThomsonTan who owns this area.

@lalitb
Copy link
Copy Markdown
Member

lalitb commented Oct 30, 2023 •
edited
Loading

I think it is better to change the MeterSelector (and so also MeterSelectorFactory) to take const std::string & instead of nostd::string_view for name, version and schema. There is no reason to use nostd::string_view in SDK surface, as we don't guarantee ABI stability there. And so just maintain single export definitions for DLL.

Edit - Same for InstrumentSelector.

lalitb
lalitb previously requested changes Oct 30, 2023
View reviewed changes
@marcalff
Copy link
Copy Markdown
Member

marcalff commented Oct 30, 2023

I think it is better to change the MeterSelector (and so also MeterSelectorFactory) to take const std::string & instead of nostd::string_view for name, version and schema. There is no reason to use nostd::string_view in SDK surface, as we don't guarantee ABI stability there. And so just maintain single export definitions for DLL.

Edit - Same for InstrumentSelector.

Thanks @lalitb

Agreed, this is a much better solution.

@marcalff marcalff self-requested a review October 30, 2023 15:55
marcalff
marcalff requested changes Oct 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per @lalitb findings, opentelemetry-cpp needs to cleanup the SDK exposed interface instead, to not use nostd::string_view.

owent
owent reviewed Oct 31, 2023
View reviewed changes
Comment thread ext/src/dll/CMakeLists.txt Outdated
@perhapsmaple
Fix formatting
f34c06c 
Signed-off-by: Harish Shan <[email protected]>
@perhapsmaple perhapsmaple requested a review from marcalff October 31, 2023 17:40
marcalff
marcalff approved these changes Oct 31, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks for revising the patch.

Please add a section in the CHANGELOG, for breaking changes,
listing the APIs affected by the change to const std::string&.

marcalff
marcalff reviewed Oct 31, 2023
View reviewed changes
Comment thread sdk/include/opentelemetry/sdk/metrics/view/instrument_selector.h
@@ -5,7 +5,6 @@

#include <memory>
Copy link
Copy Markdown
Member

@marcalff marcalff Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the string header as well ?
Same for every header file touched.

lalitb
lalitb approved these changes Oct 31, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM once the string header is included in modified header files.

@lalitb lalitb merged commit ca08c5a into open-telemetry:main Nov 1, 2023
@lalitb lalitb mentioned this pull request Nov 1, 2023
Closed
@Porkepix Porkepix mentioned this pull request Dec 7, 2023
Merged
@perhapsmaple perhapsmaple deleted the stl_dll_fix branch February 11, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@owent owent owent left review comments

@marcalff marcalff marcalff approved these changes

@lalitb lalitb lalitb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUILD] Link failure using x64 Windows DLL

4 participants

@perhapsmaple @lalitb @marcalff @owent