-
-
Notifications
You must be signed in to change notification settings - Fork 220
fix: support musl on Linux #4188
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
eee505e
to
333332c
Compare
Hi @jamescrosswell, I finally managed to get Is the approach with
|
<SentryNativeBuildOutputs Condition="$([MSBuild]::IsOsPlatform('Linux'))">$(SentryNativeOutputDirectory-linux-x64)lib$(SentryNativeLibraryName).a</SentryNativeBuildOutputs> | ||
<SentryNativeBuildOutputs Condition="'$(RuntimeIdentifier)' == 'linux-x64'">$(SentryNativeOutputDirectory-linux-x64)lib$(SentryNativeLibraryName).a</SentryNativeBuildOutputs> | ||
<SentryNativeBuildOutputs Condition="'$(RuntimeIdentifier)' == 'linux-musl-x64'">$(SentryNativeOutputDirectory-linux-musl-x64)lib$(SentryNativeLibraryName).a</SentryNativeBuildOutputs> |
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.
Is there any potential downside to checking the runtime identifier instead of the platform reported by msbuild? Are there any relevant cross-build scenarios we should consider?
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.
hm, I'm not sure. I know that RID can be explicitly set, to make a build that's specific for a platform.
Or not set (it's optinal is what I mean) in which case the build is not platform specific. But I haven't looked at it in details in years. Maybe @Flash0ver has tips/pointers
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.
Publish the app for a specific runtime identifier using dotnet publish -r .
Looks like a RID is required in order to build for Native AOT (which makes a lot of sense) so this shouldn't be an isssue
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.
For some reason, checking for RID did not work as expected in this context :(
With RID checks:
$ dotnet build Sentry-CI-Build-Linux-musl.slnf -c Release
[...]
$ tree src/Sentry/Platforms/Native/
src/Sentry/Platforms/Native/
├── CFunctions.cs
├── NativeContextWriter.cs
├── NativeScopeObserver.cs
├── Sentry.Native.targets
├── SentryNative.cs
├── SentrySdk.cs
├── buildTransitive
│ └── Sentry.Native.targets
└── windows-config.cmake
Without RID checks:
$ dotnet build Sentry-CI-Build-Linux-musl.slnf -c Release
[...]
$ tree src/Sentry/Platforms/Native/
src/Sentry/Platforms/Native/
├── CFunctions.cs
├── NativeContextWriter.cs
├── NativeScopeObserver.cs
├── Sentry.Native.targets
├── SentryNative.cs
├── SentrySdk.cs
├── buildTransitive
│ └── Sentry.Native.targets
├── sentry-native
│ └── linux-musl-x64
│ └── libsentry-native.a # <==
└── windows-config.cmake
3 directories, 9 files
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 wonder if we're fine with the slightly disjoint behavior for local builds between operating systems ... if I read it correctly:
For ordinary builds (dotnet build
or simple build via IDE)
- on Windows and macOS we also build the native dependencies, although not necessarily required, when out-of-date, because
$(SentryNativeBuildOutputs)
will be set - on Linux we do not build the native dependencies during ordinary builds locally, because
$(SentryNativeBuildOutputs)
will not be set when the$(RuntimeIdentifier)
is empty
However, when building for a specific platform, and in particular AOT, then the $(RuntimeIdentifier)
is set and we build the native dependencies also on Linux.
If I understand the Inputs="$(SentryNativeBuildInputs)" Outputs="$(SentryNativeBuildOutputs)"
of the _BuildSentryNativeSDK
Target correctly.
Actually, I'm not entirely certain: if $(SentryNativeBuildOutputs)
is empty ... which it is in the case of an "ordinary" build on Linux when the $(RuntimeIdentifier)
is not set, will the _BuildSentryNativeSDK
Target below then be invoked or not invoked?
However, my thought here may be totally nit-picky, as when the Native Dependencies are required, they will be built, so we may have a slight build behavior disjoint between Operating Systems, but the runtime locally should be perfectly fine indeed.
UPDATE ... oops, I missed your previous comment @jpnurmi ... I believe I described the same behavior
An "ordinary" dotnet build
does not set the $(RuntimeIdentifier)
.
Do we need the native dependency for just a dotnet build
? Conversely, do we need the native dependencies only when specifying an RID
?
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.
Aha, thanks! So it might just be my misunderstanding. ;) I compared what outputs were present after dotnet build Sentry-CI-Build-Linux(-musl).slnf
on Ubuntu vs. Alpine Linux and tried to achieve the same.
On Ubuntu, if I run dotnet workload restore
+ dotnet build Sentry-CI-Build-Linux.slnf
in a clean sentry-dotnet checkout of the main branch, it does seem to build src/Sentry/Platforms/Native/sentry-native/linux-x64/libsentry-native.a
.
I'd say so, and I copied your approach on #4187 (which won't land anytime soon since it's blocked on |
Yeah I think that makes sense. The names we give to these solutions are maybe what's confusing. Implicitly building musl we're trying to provide better support for SDK users deploying ASP.NET Core apps using docker containers... so no android required. However |
5eb47e5
to
9971b68
Compare
I'm not sure if it's caused you any issues here but just FYI: |
TODO:
Notes:
Close: #4127