Skip to content

chore: Add manually configured entitlement files and configurations #5213

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

Merged
merged 4 commits into from
May 16, 2025

Conversation

armcknight
Copy link
Member

These were added in #5208 but the entitlement files aren't actually needed for the test to pass.

#skip-changelog

@philprime
Copy link
Contributor

To answer your questions in this PR comment.

I was looking at sandbox settings due to the discovery of sentry-wizard#988, where we realized that the SentrySDK can not be used when sandbox is enabled without checking "Outgoing Connections (Client)".

That's when I realized that the Entitlements file where absent and that the App Sandbox settings where also not there where I expected to find them in Xcode:

Screenshot 2025-05-09 at 09 36 34

So basically when checking out this PR, I am not able to use macOS-Swift-Sandboxed to perform tests with data being send to Sentry.

I was not aware that ENABLE_APP_SANDBOX exists as I am used to manage Info.plist and Entitlements as actual files instead of letting Xcode generate them from build settings. Thanks for that, will keep it in mind when fixing the wizard 👍

But to be able to use the Sentry SDK and have it actually send envelopes in a sandboxed macOS app, we will have to check the "Outgoing Connections (Client)" in the App Sandbox settings. Can we do that from the build settings as well, when using ENABLE_APP_SANDBOX?

  • why does the entitlements file for macOS-Swift also have com.apple.security.app-sandbox set to ? actually, looking again, i think macOS-Swift.entitlements is simply unused, removing in

Ah yes, that file was a left-over from local testing and accidentally committed by me and missed in PR review. Thanks for pointing this out, that one should be removed as it isn't used.

why do we provide the entitlement files with the settings, but then also specify the setting again in the xcodegen spec?

see answers above

ETA: Ah I see what happened here, you checked in the generated files. Please don't commit multiple sources of truth to the repository as it can cause confusion and drift, as I mentioned in #5106 (comment)

That's why I reached out on Slack, as I wanted to circle back to you that it doesn't lead to a misunderstanding or perceives as undermining of the previous discussion. It wasn't entirely clear which files are checked in and which ones not:

File Can be generated? Is being generated? Is ignored?
.xcodeproj YES YES YES
Info.plist YES NO NO
.entitlements YES NO YES

@armcknight
Copy link
Member Author

@philprime Thanks for the detailed response. I'm totally fine using entitlements for sandboxing and the outgoing client connection thing as I didn't know about the work being done around that.

My preference would be to check in an entitlements file and reference it with CODE_SIGN_ENTITLEMENTS in an xcconfig, instead of declaring the settings in the XcodeGen spec to have it generate an entitlements file, but I don't feel too strongly about this.

The one thing I will continue to request is, if we go the XcodeGen-managed route, that we don't check in the generated entitlements files.

Would you be ok with me modifying this PR as such, with checked in entitlements files with all the needed settings, and referencing them from the projects?

@philprime
Copy link
Contributor

philprime commented May 14, 2025

In that case let's treat it the same way as we do with Info.plist and will not generate the file using xcodegen and directly configure it using CODE_SIGN_ENTITLEMENTS.

Would you be ok with me modifying this PR as such, with checked in entitlements files with all the needed settings, and referencing them from the projects?

Yes, I can do that. I'll create an entitlements file for each, even if empty, and reference it accordingly.

@philprime philprime self-assigned this May 14, 2025
@philprime philprime changed the title test: remove entitlement files to force sandboxing chore: Add manually configured entitlement files and configurations May 14, 2025
@philprime philprime added the Waiting for: CI Marks PR as ready-for-merge when CI passes. To be replaced with required checks. label May 14, 2025
@philprime
Copy link
Contributor

@armcknight I went through all the configs and manually tested if I can configure capabilities from the Xcode UI

Copy link
Contributor

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1233.92 ms 1246.45 ms 12.53 ms
Size 23.76 KiB 865.92 KiB 842.16 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
01a28a9 1225.55 ms 1249.96 ms 24.41 ms
8f212af 1246.41 ms 1270.98 ms 24.57 ms
c6a8035 1236.80 ms 1253.04 ms 16.24 ms
24e0744 1241.98 ms 1262.44 ms 20.46 ms
0f4071f 1212.80 ms 1239.22 ms 26.43 ms
bfe863d 1223.19 ms 1236.23 ms 13.04 ms
5bcb070 1223.38 ms 1246.45 ms 23.07 ms
4f848d0 1233.79 ms 1258.49 ms 24.70 ms
26530fe 1233.98 ms 1250.06 ms 16.08 ms
c0ff306 1218.92 ms 1240.64 ms 21.72 ms

App size

Revision Plain With Sentry Diff
01a28a9 22.85 KiB 405.39 KiB 382.55 KiB
8f212af 22.84 KiB 403.14 KiB 380.29 KiB
c6a8035 22.31 KiB 780.90 KiB 758.58 KiB
24e0744 21.58 KiB 709.06 KiB 687.48 KiB
0f4071f 21.58 KiB 681.72 KiB 660.14 KiB
bfe863d 21.58 KiB 414.57 KiB 392.99 KiB
5bcb070 21.58 KiB 699.29 KiB 677.71 KiB
4f848d0 21.58 KiB 713.91 KiB 692.33 KiB
26530fe 21.58 KiB 714.93 KiB 693.35 KiB
c0ff306 20.76 KiB 434.65 KiB 413.89 KiB

@philprime philprime removed the Waiting for: CI Marks PR as ready-for-merge when CI passes. To be replaced with required checks. label May 15, 2025
@armcknight armcknight enabled auto-merge (squash) May 15, 2025 08:45
Copy link
Contributor

@philprime philprime left a comment

Choose a reason for hiding this comment

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

As we @armcknight switched roles I take your enabling of the auto-merge as an "approval" for the changes, and actually approve this PR to get it merged.

@armcknight armcknight merged commit dd34512 into main May 16, 2025
72 of 74 checks passed
@armcknight armcknight deleted the armcknight/test/remove-sandbox-entitlement-files branch May 16, 2025 07:49
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.

2 participants