fix: Add a linker script to enforce exported symbols #363
Conversation
CMakeLists.txt
Outdated
| # FIXME: cmake 3.13 introduced target_link_options | ||
| target_link_libraries(sentry PUBLIC | ||
| "$<$<OR:$<PLATFORM_ID:Linux>,$<PLATFORM_ID:Android>>:-Wl,--build-id=sha1,--version-script=${PROJECT_SOURCE_DIR}/src/exports.map>") |
There was a problem hiding this comment.
Shouldn't the --version-script be a PRIVATE flag instead of PULBIC and only enabled if SENTRY_SHARED_LIBS=ON ?
As you wouldn't want binaries linked to sentry to use that linker script.
There was a problem hiding this comment.
👍 sounds good. Still trying to figure out how all this plays together.
CMakeLists.txt
Outdated
| else() | ||
| set(CMAKE_C_VISIBILITY_PRESET hidden) | ||
| set(CMAKE_CXX_VISIBILITY_PRESET hidden) | ||
| set(CMAKE_VISIBILITY_INLINES_HIDDEN 1) |
There was a problem hiding this comment.
i'm not sure I understand the intent of altering the visibility settings.
supervacuus
force-pushed
the
fix/exported-symbols
branch
from
1e6e27d
to
ae515f3
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
+ Coverage 81.18% 81.25% +0.07%
==========================================
Files 53 53
Lines 6947 6947
Branches 1112 1112
==========================================
+ Hits 5640 5645 +5
+ Misses 1196 1192 -4
+ Partials 111 110 -1 |
...`sentry_`-prefix as a Native SDK `PRIVATE` linker setting.
supervacuus
force-pushed
the
fix/exported-symbols
branch
from
8c34c3d
to
ede1a1a
Compare
|
@Swatinem, I cannot invite you to a review since you started this. Let me know how you want to handle it. I am fine both ways (new PR or review via comment and I approve), I am happy either way. |
|
lgtm, you can approve and merge :-) |
Trying to implement the suggestion from https://github.com/getsentry/sentry-android/issues/509
CC @eakoli and @blinkov since you added that CMake setting.
I’m thinking if we should maybe expose the breakpad and crashpad symbols? Maybe users might expect to be able to call out to crashpad if they link to sentry built with the crashpad backend.