Skip to content
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

allow defining SENTRY_SDK_NAME externally #677

Merged
merged 5 commits into from
Feb 15, 2022

Conversation

bruno-garcia
Copy link
Member

Is this a feasible solution to this issue? getsentry/sentry-java#1901

That assumes we can define SENTRY_SDK_NAME in the NDK CMakeLists or Gradle

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

the #endif was missing ;-)

Lets check if this works when integrated with the sentry-java build file.
I think it should be possible to just add that compile definiton to the sub-project there, otherwise you would actually need to make that a cmake option.

https://github.com/getsentry/sentry-java/blob/33aeeb081e38aac8da02fd4101b6dfeb25d424ce/sentry-android-ndk/CMakeLists.txt#L16

include/sentry.h Outdated Show resolved Hide resolved
Co-authored-by: Arpad Borsos <swatinem@swatinem.de>
include/sentry.h Outdated Show resolved Hide resolved
@bruno-garcia
Copy link
Member Author

I spent more time than I'm willing to admit on this:

  • Set cmake min version to 3.13, supposedly what supports what I wanted:
  • use target_compile_definitions after add_directory
  • this function doesn't support alias though

I suspect that because of add_library(sentry::sentry ALIAS sentry) it doesn't work. If I use target_compile_definitions(sentry PRIVATE SENTRY_SDK_NAME="sentry.native.android") in the CMakeLists of this repo, it works. If I use add_compile_definitions` in this CmakeLists it also works.

So I'm proposing a different solution, likely better anyway as it'll be less inviting for custom sdk names by folks compiling directly:

image

@bruno-garcia
Copy link
Member Author

No change needed in the Android repo

@bruno-garcia
Copy link
Member Author

I'll fix the tests during the week

@Mixaill
Copy link
Contributor

Mixaill commented Feb 13, 2022

Another approch would be something like.
Minimal changes to the external projects. Also it does not require for external project to known anything about this project internals.

#my_cool_project/Cmakelists.txt

...
set(SENTRY_SDK_NAME "another-sdk-name")
add_subdirectory(sentry-native)
...
#my_cool_project/sentry-native/Cmakelists.txt

...
if(DEFINED SENTRY_SDK_NAME)
    target_compile_definitions(sentry PRIVATE SENTRY_SDK_NAME="${SENTRY_SDK_NAME}")
endif()
...
//my_cool_project/sentry-native/include/sentry.h

...
#if !defined(SENTRY_SDK_NAME)
#    define SENTRY_SDK_NAME "sentry.native"
#endif
...

@bruno-garcia
Copy link
Member Author

Another approch would be something like. Minimal changes to the external projects. Also it does not require for external project to known anything about this project internals.

#my_cool_project/Cmakelists.txt

...
set(SENTRY_SDK_NAME "another-sdk-name")
add_subdirectory(sentry-native)
...
#my_cool_project/sentry-native/Cmakelists.txt

...
if(DEFINED SENTRY_SDK_NAME)
    target_compile_definitions(sentry PRIVATE SENTRY_SDK_NAME="${SENTRY_SDK_NAME}")
endif()
...
//my_cool_project/sentry-native/include/sentry.h

...
#if !defined(SENTRY_SDK_NAME)
#    define SENTRY_SDK_NAME "sentry.native"
#endif
...

On a second thought I believe it's best if it's not 'configurable' as we don't want custom values to be pass in here resulting in different variations. The Android case is a special one

@Swatinem Swatinem merged commit 4d9668e into master Feb 15, 2022
@Swatinem Swatinem deleted the feat/allow-custom-sdk-name branch February 15, 2022 09:05
@marandaneto
Copy link
Contributor

@Swatinem can we cut a release to get this into the Android SDK? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants