feat: native crash backend as option for Windows#2692
Conversation
|
86d91fa to
c933961
Compare
| <Target Name="EnsureWindowsNativeSDK" | ||
| BeforeTargets="BeforeBuild" | ||
| Condition="'$(MSBuildProjectName)' == 'Sentry.Unity' | ||
| And $([MSBuild]::IsOSPlatform('Windows')) | ||
| And !Exists('$(SentryWindowsNativeArtifactsDestination)sentry.dll')"> | ||
| <CallTarget Targets="BuildWindowsNativeSDK" /> | ||
| </Target> |
There was a problem hiding this comment.
Bug: The EnsureWindowsNativeSDK build target runs on every Windows build if sentry.dll is missing, even when the native backend is disabled, causing unnecessary build failures.
Severity: MEDIUM
Suggested Fix
Modify the condition for the EnsureWindowsNativeSDK target to also check if the native backend is enabled for the current build. This could be done by checking a property that indicates the selected backend, preventing the target from running when it's not needed.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: build/native-sdks.targets#L183-L189
Potential issue: The MSBuild target `EnsureWindowsNativeSDK` in
`build/native-sdks.targets` is triggered on every Windows build where `sentry.dll` is
not present. The condition for this target only checks the OS and the existence of the
DLL, but not whether the native backend is actually enabled for the build. Consequently,
developers on Windows without the C++ build environment (CMake, submodules) will face a
build failure when running `dotnet build` on a fresh checkout, even if they don't intend
to use the native backend. This blocks development for a valid use case.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
lgtm!
one small q for the native backend in general; we have these three options (sentry_options_set_minidump_mode , sentry_options_set_crash_reporting_mode and sentry_options_set_crash_upload_mode) for the native backend, would it make sense to expose these in the Unity SDK too?
Fair point. I'll follow up on this. |
wip