feat: Add asset catalog parsing for apple platforms #2545

noahsmartin · 2025-06-05T16:25:59Z

Adds asset catalog parsing support. This uses Apple Objective-C APIs, which is why it needs to be run from Swift (or ObjC but we try to use Swift as much as possible)

src/commands/mobile_app/upload.rs

szokeasaurusrex · 2025-06-10T15:41:12Z

Hey @noahsmartin, I still need to review the code changes, but since you modified build.rs, I triggered our "Release Build" CI actions on this PR by also pushing the commit to the release/test branch (the "Release Build" actions only run on branches matching the pattern release/*). The "Release Build" flow is responsible for building the release binaries, so it is important that these actions succeed.

It looks like the build for macOS x86_64 is failing, and the failure appears to be related to the changes introduced in this PR. The Docker image also failed to build, but this failure is unrelated to your change, and I already put up a PR to fix it (#2550).

I would appreciate if you could investigate and try to fix this failure; once you have made the necessary changes, you can rerun the action by pushing to release/test (or another release/* branch of your choosing). 🙏

szokeasaurusrex · 2025-06-11T08:29:23Z

I updated this branch with the latest changes from origin/master and the release build is still failing on macOS x86_64 after pushing the updated branch as release/test, which confirms the failure is most likely due to changes in this PR

szokeasaurusrex

Hey, thanks for adding this!

I have some comments, mostly regarding project structure and some feedback on Rust idioms. I'll leave the review of the Swift and .h and .m files to someone more familiar with these languages/systems

src/apple/AssetCatalogReader.swift

szokeasaurusrex · 2025-06-12T12:37:00Z

src/apple/safeValueForKey.m

Likewise, would appreciate if someone more familiar with this code reviews this file and the corresponding .h file. It would be good also to make sure that these are tested somehow

src/commands/mobile_app/upload.rs

src/utils/mobile_app/apple.rs

src/utils/mobile_app/mod.rs

src/utils/mobile_app/validation.rs

Cargo.toml

szokeasaurusrex · 2025-06-12T13:26:12Z

Also, the release build is still failing on x86_64

noahsmartin · 2025-06-12T14:42:20Z

Thanks @szokeasaurusrex! Just pushed a commit for those comments + fixed the release build

rbro112

LGTM on command/rust side. Can't speak to the .h/.m/.swift files as I'm not experienced enough in that realm

szokeasaurusrex · 2025-06-23T14:59:36Z

apple-catalog-parsing/src/lib.rs

@@ -0,0 +1,5 @@
+#![cfg(target_os = "macos")]


@noahsmartin FYI this is what I meant by using #![cfg(target_os = "macos")] to make the crate empty on non-macOS systems. You'll notice that this allows us to remove the other #[cfg(target_os = "macos"] directives that were scattered around here

apple-catalog-parsing/src/asset_catalog.rs

szokeasaurusrex · 2025-06-23T15:38:22Z

apple-catalog-parsing/build.rs

+            "--scratch-path",
+            &format!("{out_dir}/swift-scratch"),
+            "--triple",
+            &format!("{arch}-apple-macosx11"),


Does this break compatibility with macos versions below 11?

I ended up being able to remove this entirely. Based on https://doc.rust-lang.org/cargo/commands/cargo-build.html#compilation-options it sounds like the supported macOS versions are determined from the machine that it builds from?

@noahsmartin I'm confused – why are the Rust docs at all relevant here? These arguments are being passed to the swift command

Rust also has a triple argument which I thought is how it is determined what OS the rust code is compatible with

I confirmed the current CLI (downloaded from GitHub releases) supports macOS 11+

noahmartin@L4F00WF7KT sentry-cli % otool -l /Users/noahmartin/Downloads/sentry-cli-Darwin-arm64 | grep LC_BUILD_VERSION -A 5 cmd LC_BUILD_VERSION cmdsize 32 platform 1 minos 11.0 sdk 14.5 ntools 1

Yeah, but this is the triple you are passing to the swift compiler

Also, you are checking the arm64 build there. The first ARM based Mac was released running macOS 11, so perhaps that explains the minimum version.

I'd be interested in seeing the min version for the x86_64 version

Good catch, it was 10.12, just updated the PR to build for this minimum:

Load command 9 cmd LC_VERSION_MIN_MACOSX cmdsize 16 version 10.12 sdk 15.5

apple-catalog-parsing/build.rs

apple-catalog-parsing/Cargo.toml

crates/sentry-cli-apple/Cargo.toml

MANIFEST.in

szokeasaurusrex · 2025-06-23T15:58:10Z

I think this is super close to being ready, @noahsmartin! Just a few more small items :)

Please pull my minor changes before changing anything, I fixed something in Cargo.toml

Co-authored-by: Daniel Szoke <[email protected]>

szokeasaurusrex · 2025-06-24T07:45:35Z

@noahsmartin the x86_64 release build for macOS is failing again :(

szokeasaurusrex

Looks good, thank you for bearing with me here! I would also like to get a review from another Rust engineer before we merge this, however.

szokeasaurusrex · 2025-06-25T10:17:01Z

apple-catalog-parsing/src/asset_catalog.rs

+    let c_string = CString::new(path.as_ref().as_os_str().as_bytes())?;
+    let string_ptr = c_string.as_ptr();
+    unsafe {
+        // The string pointed to is immutable, in Swift we cannot change it.


@noahsmartin just to confirm here: Is the Swift code itself respecting the string's immutability?

Although it is true that the string is immutable in Rust, once you obtain a raw pointer to it, and once the Swift code has access to that raw pointer, the Rust compiler can no longer enforce the immutability invariant. But, the way your comment is written makes it sound like you are relying on the Rust compiler to be ensuring immutability here. To ensure memory safety, therefore, it is essential that the Swift code does not (and ideally cannot, via Swift's type system) mutate anything the pointer is pointing to.

It is also immutable in Swift, the type in swift is UnsafePointer<CChar> which is immutable, to mutate it the type would have needed to be UnsafeMutablePointer<CChar>

Awesome, thanks! If you could update the comment to clarify that the pointer is immutable in Swift, that would be nice

noahsmartin force-pushed the assetCatalogParsing branch 7 times, most recently from fd4561a to 112642d Compare June 6, 2025 16:21

feat: Add asset catalog parsing for apple platforms

ff6040d

noahsmartin force-pushed the assetCatalogParsing branch from 112642d to ff6040d Compare June 7, 2025 04:20

noahsmartin marked this pull request as ready for review June 7, 2025 04:24

noahsmartin requested a review from szokeasaurusrex as a code owner June 7, 2025 04:24

rbro112 reviewed Jun 9, 2025

View reviewed changes

src/commands/mobile_app/upload.rs Outdated Show resolved Hide resolved

rbro112 reviewed Jun 9, 2025

View reviewed changes

src/commands/mobile_app/upload.rs Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into assetCatalogParsing

64226f4

PR feedback and fix for intel

2627c94

noahsmartin force-pushed the assetCatalogParsing branch from a71a846 to 2627c94 Compare June 11, 2025 18:23

noahsmartin added 2 commits June 11, 2025 11:23

Merge branch 'master' into assetCatalogParsing

8bdde60

Fixes

95a53dd

szokeasaurusrex reviewed Jun 12, 2025

View reviewed changes

noahsmartin force-pushed the assetCatalogParsing branch 3 times, most recently from e172264 to 28d9d12 Compare June 12, 2025 14:31

PR review

06ac99e

noahsmartin force-pushed the assetCatalogParsing branch from 28d9d12 to 06ac99e Compare June 12, 2025 14:34

rbro112 approved these changes Jun 12, 2025

View reviewed changes

noahsmartin force-pushed the assetCatalogParsing branch from 21da0bd to b15a485 Compare June 12, 2025 18:59

noahsmartin force-pushed the assetCatalogParsing branch from a530d38 to dd13219 Compare June 17, 2025 21:03

szokeasaurusrex and others added 3 commits June 23, 2025 15:12

ensure lints run in each crate

c2c86a5

Update build settings

bc0b8be

Use search paths

41dc087

noahsmartin force-pushed the assetCatalogParsing branch 2 times, most recently from 4b17b79 to 57ca420 Compare June 23, 2025 14:50

lint

c91cb3d

noahsmartin force-pushed the assetCatalogParsing branch from 57ca420 to c91cb3d Compare June 23, 2025 14:51

ref: compile empty crate on macos

e71f2ff

szokeasaurusrex reviewed Jun 23, 2025

View reviewed changes

Add [workspace]

a6907aa

szokeasaurusrex reviewed Jun 23, 2025

View reviewed changes

noahsmartin and others added 7 commits June 23, 2025 12:08

Update apple-catalog-parsing/src/asset_catalog.rs

a35cde8

Co-authored-by: Daniel Szoke <[email protected]>

Update apple-catalog-parsing/src/asset_catalog.rs

722318e

Co-authored-by: Daniel Szoke <[email protected]>

Update MANIFEST.in

a5fd862

Co-authored-by: Daniel Szoke <[email protected]>

Update apple-catalog-parsing/Cargo.toml

9c48e4a

Co-authored-by: Daniel Szoke <[email protected]>

Update apple-catalog-parsing/build.rs

4c58a4c

Co-authored-by: Daniel Szoke <[email protected]>

Update apple-catalog-parsing/build.rs

cdc35a3

Co-authored-by: Daniel Szoke <[email protected]>

PR feedback

0f70e30

noahsmartin force-pushed the assetCatalogParsing branch from 87743fc to 0f70e30 Compare June 23, 2025 16:27

Fix intel

dfc8cc1

noahsmartin force-pushed the assetCatalogParsing branch from d852375 to dfc8cc1 Compare June 24, 2025 16:47

Drop min version

0fae017

szokeasaurusrex approved these changes Jun 25, 2025

View reviewed changes

szokeasaurusrex requested a review from Dav1dde June 25, 2025 10:35

Update comment

1e2ef30

noahsmartin merged commit de69b5e into master Jun 25, 2025
54 checks passed

noahsmartin deleted the assetCatalogParsing branch June 25, 2025 16:36

Uh oh!

feat: Add asset catalog parsing for apple platforms #2545

feat: Add asset catalog parsing for apple platforms #2545

Uh oh!

Conversation

noahsmartin commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szokeasaurusrex commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szokeasaurusrex commented Jun 11, 2025

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szokeasaurusrex commented Jun 12, 2025

Uh oh!

noahsmartin commented Jun 12, 2025

Uh oh!

rbro112 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szokeasaurusrex commented Jun 23, 2025

Uh oh!

szokeasaurusrex commented Jun 24, 2025

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

noahsmartin commented Jun 5, 2025 •

edited

Loading

szokeasaurusrex commented Jun 10, 2025 •

edited

Loading

rbro112 left a comment •

edited

Loading