-
-
Notifications
You must be signed in to change notification settings - Fork 245
feat(mobileapp): Update mobile app command for ipa uploads #2619
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
Changes from 7 commits
2ba5dff
46ebd0f
9586ada
6c078ce
b9cf3ed
0b088cb
e8cb4a3
5b8fd2e
e050988
a26b6ad
b2cef47
9daae6d
620bce8
cd04205
d681185
0eeaa67
8ff0a95
b8518fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| #![cfg(feature = "unstable-mobile-app")] | ||
|
|
||
| use anyhow::{anyhow, Result}; | ||
| use log::debug; | ||
| use std::io::Cursor; | ||
| use std::path::{Path, PathBuf}; | ||
| use zip::ZipArchive; | ||
|
|
||
| use crate::utils::fs::TempDir; | ||
|
|
||
| /// Converts an IPA file to an XCArchive directory structure. The provided IPA must be a valid IPA file. | ||
| /// | ||
| /// # Format Overview | ||
| /// | ||
| /// ## IPA (iOS App Store Package) | ||
| /// An IPA file is a compressed archive containing an iOS app ready for distribution. | ||
| /// It has the following structure: | ||
| /// ``` | ||
| /// MyApp.ipa | ||
| /// └── Payload/ | ||
| /// └── MyApp.app/ | ||
| /// ├── Info.plist | ||
| /// ├── MyApp (executable) | ||
| /// ├── Assets.car | ||
| /// └── ... (other app resources) | ||
| /// ``` | ||
| /// | ||
| /// ## XCArchive (Xcode Archive) | ||
| /// An XCArchive is a directory structure created by Xcode when archiving an app for distribution. | ||
| /// It has the following structure: | ||
| /// ``` | ||
| /// MyApp.xcarchive/ | ||
| /// ├── Info.plist | ||
| /// ├── Products/ | ||
| /// │ └── Applications/ | ||
| /// │ └── MyApp.app/ | ||
| /// │ ├── Info.plist | ||
| /// │ ├── MyApp (executable) | ||
| /// │ ├── Assets.car | ||
| /// │ └── ... (other app resources) | ||
| /// └── ... (other archive metadata) | ||
| /// ``` | ||
| /// | ||
| /// # Transformation Process | ||
| /// | ||
| /// This function performs the following steps: | ||
| /// 1. Creates the XCArchive directory structure (`archive.xcarchive/Products/Applications/`) | ||
| /// 2. Extracts the app name from the IPA by finding the shortest path ending with `.app/Info.plist` | ||
| /// 3. Extracts all files from the IPA's `Payload/` directory into the XCArchive structure | ||
| /// 4. Creates an `Info.plist` file for the XCArchive with the app path reference | ||
| /// 5. Returns the path to the XCArchive directory structure | ||
|
noahsmartin marked this conversation as resolved.
Outdated
|
||
| pub fn ipa_to_xcarchive(ipa_path: &Path, ipa_bytes: &[u8], temp_dir: &TempDir) -> Result<PathBuf> { | ||
| debug!( | ||
| "Converting IPA to XCArchive structure: {}", | ||
| ipa_path.display() | ||
| ); | ||
|
|
||
| let xcarchive_dir = temp_dir.path().join("archive.xcarchive"); | ||
| let products_dir = xcarchive_dir.join("Products"); | ||
| let applications_dir = products_dir.join("Applications"); | ||
|
|
||
| debug!("Creating XCArchive directory structure"); | ||
| std::fs::create_dir_all(&applications_dir)?; | ||
|
|
||
| // Extract IPA file | ||
| let cursor = Cursor::new(ipa_bytes); | ||
| let mut ipa_archive = ZipArchive::new(cursor)?; | ||
|
|
||
| let app_name = extract_app_name_from_ipa(&ipa_archive)?; | ||
|
|
||
| // Extract all files from the archive | ||
| for i in 0..ipa_archive.len() { | ||
| let mut file = ipa_archive.by_index(i)?; | ||
|
|
||
| if let Some(name) = file.enclosed_name() { | ||
| if let Ok(stripped) = name.strip_prefix("Payload/") { | ||
|
noahsmartin marked this conversation as resolved.
Outdated
|
||
| if !file.is_dir() { | ||
| // Create the file path in the XCArchive structure | ||
| let target_path = applications_dir.join(stripped); | ||
|
|
||
| // Create parent directories if necessary | ||
| if let Some(parent) = target_path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
|
|
||
| // Extract file | ||
| let mut target_file = std::fs::File::create(&target_path)?; | ||
| std::io::copy(&mut file, &mut target_file)?; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Create Info.plist for XCArchive | ||
| let info_plist_path = xcarchive_dir.join("Info.plist"); | ||
|
|
||
| let info_plist_content = format!( | ||
| r#"<?xml version="1.0" encoding="UTF-8"?> | ||
| <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
| <plist version="1.0"> | ||
| <dict> | ||
| <key>ApplicationProperties</key> | ||
| <dict> | ||
| <key>ApplicationPath</key> | ||
| <string>Applications/{app_name}.app</string> | ||
| </dict> | ||
| <key>ArchiveVersion</key> | ||
| <integer>1</integer> | ||
| </dict> | ||
| </plist>"# | ||
| ); | ||
|
|
||
| std::fs::write(&info_plist_path, info_plist_content)?; | ||
|
|
||
| debug!( | ||
| "Created XCArchive Info.plist at: {}", | ||
| info_plist_path.display() | ||
| ); | ||
| Ok(xcarchive_dir) | ||
| } | ||
|
|
||
| fn extract_app_name_from_ipa(archive: &ZipArchive<Cursor<&[u8]>>) -> Result<String> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am still waiting for an answer to this question from my previous review |
||
| archive | ||
| .file_names() | ||
| .filter(|name| name.starts_with("Payload/") && name.ends_with(".app/Info.plist")) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tested that this works as expected if running this command on Windows? Since on Windows, the paths might have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It wouldn't really make sense to upload an IPA from windows, they are always on Macs and other things like the asset catalog support would not work on windows. So I will just add the code to avoid ever calling this path so users don't get confused about why thinks like asset catalogs stop working |
||
| .min_by_key(|name| name.len()) | ||
|
noahsmartin marked this conversation as resolved.
Outdated
|
||
| .and_then(|name| name.strip_prefix("Payload/")) | ||
| .and_then(|name| name.split('/').next()) | ||
| .and_then(|name| name.strip_suffix(".app")) | ||
| .map(|name| name.to_owned()) | ||
| .ok_or_else(|| anyhow!("No .app found in IPA")) | ||
| } | ||
|
noahsmartin marked this conversation as resolved.
Outdated
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,19 @@ pub fn is_aab_file(bytes: &[u8]) -> Result<bool> { | |
| Ok(has_bundle_config && has_base_manifest) | ||
| } | ||
|
|
||
| pub fn is_ipa_file(bytes: &[u8]) -> Result<bool> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just wondering, why do we have this validation code in the first place? Why not just try to parse the file as all the different file types directly, then if we fail to, we assume that the file is invalid? Having the separate validation logic seems like it is leading to duplicated code paths. We already check this structure in Fixing this (especially for the other file types) is of course out of scope for this PR, but hoping you can shed some light on this architectural decision.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed on a call, it's mostly because this was ported from our typescript code, we probably could refactor this and make it at least just file extension checking |
||
| let cursor = std::io::Cursor::new(bytes); | ||
| let archive = zip::ZipArchive::new(cursor)?; | ||
|
|
||
| let is_ipa = archive.file_names().any(|name| { | ||
| name.starts_with("Payload/") | ||
| && name.ends_with(".app/Info.plist") | ||
| && name.matches('/').count() == 2 | ||
| }); | ||
|
Comment on lines
+45
to
+49
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could reuse the regex here, but this is also fine |
||
|
|
||
| Ok(is_ipa) | ||
| } | ||
|
|
||
| pub fn is_xcarchive_directory<P>(path: P) -> bool | ||
| where | ||
| P: AsRef<Path>, | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please rename this file to indicate this is macOS specific |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| ``` | ||
| $ sentry-cli mobile-app upload tests/integration/_fixtures/mobile_app/ipa.ipa --sha test_sha | ||
| ? success | ||
| Successfully uploaded 1 file to Sentry | ||
| - tests/integration/_fixtures/mobile_app/ipa.ipa | ||
|
|
||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| invalid ipa content |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -55,6 +55,18 @@ fn command_mobile_app_upload_invalid_xcarchive() { | |||
| .run_and_assert(AssertCommand::Failure); | ||||
| } | ||||
|
|
||||
| #[test] | ||||
| fn command_mobile_app_upload_invalid_ipa() { | ||||
|
noahsmartin marked this conversation as resolved.
|
||||
| TestManager::new() | ||||
| .assert_cmd(vec![ | ||||
| "mobile-app", | ||||
| "upload", | ||||
| "tests/integration/_fixtures/mobile_app/invalid.ipa", | ||||
| ]) | ||||
| .with_default_token() | ||||
| .run_and_assert(AssertCommand::Failure); | ||||
| } | ||||
|
|
||||
| #[test] | ||||
| fn command_mobile_app_upload_apk_all_uploaded() { | ||||
| TestManager::new() | ||||
|
|
@@ -160,3 +172,57 @@ fn command_mobile_app_upload_apk_chunked() { | |||
| .register_trycmd_test("mobile_app/mobile_app-upload-apk.trycmd") | ||||
| .with_default_token(); | ||||
| } | ||||
|
|
||||
| #[test] | ||||
| #[cfg(target_os = "macos")] | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why only run the test on macOS? The code we are testing is available on all platforms from what I can tell.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the ipa support to be macOS only
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [question] do we expect that in the future users may want to be able to upload IPA files from a non-macOS platform, e.g. from a (non-macOS) CI runner? If so, would it be possible to add such support? |
||||
| /// This test simulates a full chunk upload for an IPA file (with only one chunk). | ||||
| /// It verifies that the Sentry CLI makes the expected API calls to the chunk upload endpoint | ||||
| /// and that the data sent to the chunk upload endpoint is exactly as expected. | ||||
| /// It also verifies that the correct calls are made to the assemble endpoint. | ||||
| fn command_mobile_app_upload_ipa_chunked() { | ||||
| let is_first_assemble_call = AtomicBool::new(true); | ||||
|
|
||||
| TestManager::new() | ||||
| .mock_endpoint( | ||||
| MockEndpointBuilder::new("GET", "/api/0/organizations/wat-org/chunk-upload/") | ||||
| .with_response_file("mobile_app/get-chunk-upload.json"), | ||||
| ) | ||||
| .mock_endpoint( | ||||
| MockEndpointBuilder::new("POST", "/api/0/organizations/wat-org/chunk-upload/") | ||||
| .with_response_fn(move |request| { | ||||
| let content_type_headers = request.header("content-type"); | ||||
| assert_eq!( | ||||
| content_type_headers.len(), | ||||
| 1, | ||||
| "content-type header should be present exactly once, found {} times", | ||||
| content_type_headers.len() | ||||
| ); | ||||
| vec![] // Client does not expect a response body | ||||
| }), | ||||
| ) | ||||
| .mock_endpoint( | ||||
| MockEndpointBuilder::new( | ||||
| "POST", | ||||
| "/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/", | ||||
| ) | ||||
| .with_header_matcher("content-type", "application/json") | ||||
| .with_matcher(r#"{"checksum":"ed9da71e3688261875db21b266da84ffe004a8a4","chunks":["ed9da71e3688261875db21b266da84ffe004a8a4"],"git_sha":"test_sha"}"#) | ||||
| .with_response_fn(move |_| { | ||||
| if is_first_assemble_call.swap(false, Ordering::Relaxed) { | ||||
| r#"{ | ||||
| "state": "created", | ||||
| "missingChunks": ["ed9da71e3688261875db21b266da84ffe004a8a4"] | ||||
| }"# | ||||
| } else { | ||||
| r#"{ | ||||
| "state": "ok", | ||||
| "missingChunks": [] | ||||
| }"# | ||||
| } | ||||
| .into() | ||||
| }) | ||||
| .expect(2), | ||||
| ) | ||||
| .register_trycmd_test("mobile_app/mobile_app-upload-ipa.trycmd") | ||||
| .with_default_token(); | ||||
| } | ||||
Uh oh!
There was an error while loading. Please reload this page.