-
-
Notifications
You must be signed in to change notification settings - Fork 223
Remove Headers and Modules from SentryCocoaFramework before packing #4533
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4533 +/- ##
==========================================
- Coverage 73.36% 73.36% -0.01%
==========================================
Files 479 479
Lines 17505 17505
Branches 3445 3445
==========================================
- Hits 12843 12842 -1
- Misses 3782 3784 +2
+ Partials 880 879 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BeforeTargets="ResolveNativeReferences" | ||
Condition="$([MSBuild]::IsOSPlatform('OSX')) and Exists('$(SentryCocoaFramework)')"> | ||
<Message Importance="High" Text="Sanitizing $(SentryCocoaFramework): removing dSYMs, Headers, Modules, PrivateHeaders (including symlinks)." /> | ||
<Exec Command="bash -c 'find "$(SentryCocoaFramework)" -depth \( -name dSYMs -o -name Headers -o -name Modules -o -name PrivateHeaders \) -exec echo Removing {} \; -exec rm -rf {} + 2>/dev/null || true'" /> |
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 think this executes for multiple target frameworks in parallel. Might be the reason you had to ignore errors because the two commands race and conflict?
What if this was already done at the end of _SetupCocoaSDK
, which is guarded to run only once? If we need to ignore errors regardless, we could get away without bash with <Exec IgnoreExitCode="true" ... />
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.
P.S. rm
has verbose mode (-v
) with quite similar output
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 don't think the Android Device test failures had anything to do with these changes to the Cocoa Bindings project... they appear to be flaky on other unrelated branches as well:
Given that, I think we can create a pre-release from this branch that some of our SDK users can test before we consider merging it into main.
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.
The same find $(SentryCocoaFramework) ... -exec rm {}
command runs in parallel for net8.0-ios17.0
and net8.0-maccatalyst17.0
even if both operate on the same modules/sentry-cocoa/Sentry-8.56.0.xcframework
directory.
Instead of ignoring errors caused by the race between those two commands running in the same directory, could we move the Exec-task to _SetupCocoaSDK
, which is executed exactly once? That way, we could simplify the command to:
<Exec Command="bash -c 'find "$(SentryCocoaFramework)" -depth \( -name dSYMs -o -name Headers -o -name Modules -o -name PrivateHeaders \) -exec echo Removing {} \; -exec rm -rf {} + 2>/dev/null || true'" /> | |
<Exec Command="find "$(SentryCocoaFramework)" -depth \( -name dSYMs -o -name Headers -o -name Modules -o -name PrivateHeaders \) -exec rm -rfv {} +" /> |
Workaround for #4292:
Warning
We should publish a pre-release from this branch and validate this resolves the problem before merging into main... this is quite a major change.