fix(launchpad): Ensure mobile artifact's zip structures are preserved by NicoHinderling · Pull Request #2585 · getsentry/sentry-cli

NicoHinderling · 2025-07-08T21:09:27Z

WIthout this, the code would remove the xcarchive wrapper directory, which causes issues when trying to parse the artifact in launchpad

szokeasaurusrex

Please see inline comments.

Also, small nitpick: prior to merge, please rename the commit message to fix(mobile-app): ... in keeping with Sentry's commit message naming conventions (we use fix instead of bug, and mobile-app is the user-facing name of this feature).

src/commands/mobile_app/upload.rs

szokeasaurusrex

Please see inline comments.

Also, I am struggling to see how the behavior has changed following these changes. As far as I can tell, this code should behave exactly the same as what we had before because it looks to me like the zip_path is equivalent to the previous relative_path.

Can you please explain what the desired difference in behavior is? If possible, please add a test case, which fails against the original code, but passes against the new code, to verify the desired behavior. This will also help me understand the purpose of this change, which I am currently struggling to grasp.

szokeasaurusrex · 2025-07-11T06:47:13Z

src/commands/mobile_app/upload.rs

+        .map(|entry| Ok(entry.into_path()))
        .collect::<Result<Vec<_>>>()?
        .into_iter()


Wrapping this in an Ok is unnecessary, because it is an infallible operation. Also, no need to collect into a vector just to make an iterator again.

Suggested change

.map(|entry| Ok(entry.into_path()))

.collect::<Result<Vec<_>>>()?

.into_iter()

.map(|entry| entry.into_path())

szokeasaurusrex · 2025-07-11T06:47:56Z

src/commands/mobile_app/upload.rs

+        debug!("Adding file to zip: {}", zip_path.display());

-        zip.start_file(relative_path.to_string_lossy(), options)?;
+        zip.start_file(zip_path.to_string_lossy(), options)?;


How is this zip_path different from the relative_path we had before?

The top level path is no longer being stripped.

Example: entry_path = /full/path/to/MyApp.xcarchive/Products/app.txt

relative_path stripped the entire path (/full/path/to/MyApp.xcarchive), losing the directory name
zip_path strips only the parent path (/full/path/to), keeping the directory name

Before: relative_path = Products/app.txt
After: zip_path = MyApp.xcarchive/Products/app.txt

Okay! I think I see the difference now. Whereas previously, you called strip_prefix(path), you now call strip_prefix(path.parent), correct?

Assuming the above is correct, I have two questions:

Couldn't you have simply added the path.parent call where we were previously computing the relative_path? It seems like you are also refactoring the code by moving this changed strip_prefix into the for loop. Is this refactor causing some other functionality change which I am missing?

Is it reasonable to assume that the path should always have a parent? E.g., what if we are running this from the root directory? Not that I think it would be super common, but perhaps there are some scenarios where this could occur (e.g. if running from within a Docker container)?

No I think you're right that the for loop refactor was not necessary. I've changed it to be this way, apologies for the thrash.

Yes it is but I've added a ok_or_else just in case

NicoHinderling · 2025-07-11T20:29:33Z

Please see inline comments.

Also, I am struggling to see how the behavior has changed following these changes. As far as I can tell, this code should behave exactly the same as what we had before because it looks to me like the zip_path is equivalent to the previous relative_path.

Can you please explain what the desired difference in behavior is? If possible, please add a test case, which fails against the original code, but passes against the new code, to verify the desired behavior. This will also help me understand the purpose of this change, which I am currently struggling to grasp.

It's about preserving the top directory in the path. I've added a unit test 👍

szokeasaurusrex

Thanks for adding the test and explaining the functionality change, this makes some more sense now. Although, I still have a couple small questions

szokeasaurusrex · 2025-07-14T12:56:24Z

src/commands/mobile_app/upload.rs

    Ok(())
 }
+
+#[cfg(not(windows))]


Why not run on Windows?

The unit test just fails because instead of MyApp.xcarchive/Products/app.txt it's something like C:\\MyApp.xcarchive\\Products\\app.txt. Lets just skip

szokeasaurusrex · 2025-07-14T13:05:51Z

src/commands/mobile_app/upload.rs

+        let mut archive = ZipArchive::new(zip_file)?;
+        let file = archive.by_index(0)?;
+        let file_path = file.name();
+


not sure why the formatter did not catch this

Suggested change

szokeasaurusrex · 2025-07-14T13:12:17Z

src/commands/mobile_app/upload.rs

+    for entry_path in entries {
+        let zip_path = entry_path.strip_prefix(
+            path.parent().ok_or_else(|| anyhow!("Failed to get parent directory"))?
+        )?.to_owned();


There's no need to call to_owned here; it just causes an unnecessary memory allocation and the code compiles without it.

Suggested change

)?.to_owned();

)?;

szokeasaurusrex · 2025-07-14T13:17:29Z

src/commands/mobile_app/upload.rs

+        debug!("Adding file to zip: {}", zip_path.display());

-        zip.start_file(relative_path.to_string_lossy(), options)?;
+        zip.start_file(zip_path.to_string_lossy(), options)?;


Okay! I think I see the difference now. Whereas previously, you called strip_prefix(path), you now call strip_prefix(path.parent), correct?

Assuming the above is correct, I have two questions:

Couldn't you have simply added the path.parent call where we were previously computing the relative_path? It seems like you are also refactoring the code by moving this changed strip_prefix into the for loop. Is this refactor causing some other functionality change which I am missing?

Is it reasonable to assume that the path should always have a parent? E.g., what if we are running this from the root directory? Not that I think it would be super common, but perhaps there are some scenarios where this could occur (e.g. if running from within a Docker container)?

szokeasaurusrex

🚀

NicoHinderling requested a review from szokeasaurusrex as a code owner July 8, 2025 21:09

NicoHinderling mentioned this pull request Jul 8, 2025

feat(launchpad): Update artifact factory logic to cover more cases getsentry/launchpad#140

Closed

szokeasaurusrex reviewed Jul 9, 2025

View reviewed changes

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

NicoHinderling changed the title ~~bug(launchpad): Ensure mobile artifact's zip structures are preserved~~ fix(launchpad): Ensure mobile artifact's zip structures are preserved Jul 9, 2025

NicoHinderling force-pushed the preserve-xcarchive-zip-structure branch from c782961 to 6991773 Compare July 9, 2025 14:11

szokeasaurusrex reviewed Jul 11, 2025

View reviewed changes

NicoHinderling added 5 commits July 11, 2025 13:30

bug(launchpad): Ensure mobile artifact's zip structures are preserved

311a5e0

address feedback

dbd4d5d

further simplify

b26bda4

feedback

391c363

lint error

b9d924c

NicoHinderling force-pushed the preserve-xcarchive-zip-structure branch from 7a4cc1a to b9d924c Compare July 11, 2025 20:30

skip test for windows

a91eba9

szokeasaurusrex reviewed Jul 14, 2025

View reviewed changes

feedback

22b3eb4

szokeasaurusrex approved these changes Jul 14, 2025

View reviewed changes

NicoHinderling merged commit 8ce9577 into master Jul 14, 2025
25 checks passed

NicoHinderling deleted the preserve-xcarchive-zip-structure branch July 14, 2025 14:12

Uh oh!

Conversation

NicoHinderling commented Jul 8, 2025

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicoHinderling commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NicoHinderling commented Jul 11, 2025 •

edited

Loading