PR feedback

Author: noahsmartin

src/commands/mobile_app/upload.rs

@@ -1,19 +1,13 @@
 use std::borrow::Cow;
-#[cfg(not(windows))]
-use std::fs;
 use std::io::Write as _;
-#[cfg(not(windows))]
-use std::os::unix::fs::PermissionsExt as _;
-use std::path::{Path, PathBuf};
+use std::path::Path;
 
 use anyhow::{anyhow, bail, Context as _, Result};
 use clap::{Arg, ArgAction, ArgMatches, Command};
 use indicatif::ProgressStyle;
-use itertools::Itertools as _;
 use log::{debug, info, warn};
 use sha1_smol::Digest;
 use symbolic::common::ByteView;
-use walkdir::WalkDir;
 use zip::write::SimpleFileOptions;
 use zip::{DateTime, ZipWriter};
 
@@ -28,7 +22,7 @@ use crate::utils::fs::TempFile;
 use crate::utils::mobile_app::{
     handle_asset_catalogs, ipa_to_xcarchive, is_apple_app, is_ipa_file,
 };
-use crate::utils::mobile_app::{is_aab_file, is_apk_file, is_zip_file};
+use crate::utils::mobile_app::{is_aab_file, is_apk_file, is_zip_file, normalize_directory};
 use crate::utils::progress::ProgressBar;
 use crate::utils::vcs;
 
@@ -271,22 +265,6 @@ fn normalize_file(path: &Path, bytes: &[u8]) -> Result<TempFile> {
     Ok(temp_file)
 }
 
-fn sort_entries(path: &Path) -> Result<std::vec::IntoIter<(PathBuf, PathBuf)>> {
-    Ok(WalkDir::new(path)
-        .follow_links(true)
-        .into_iter()
-        .filter_map(Result::ok)
-        .filter(|entry| entry.path().is_file())
-        .map(|entry| {
-            let entry_path = entry.into_path();
-            let relative_path = entry_path.strip_prefix(path)?.to_owned();
-            Ok((entry_path, relative_path))
-        })
-        .collect::<Result<Vec<_>>>()?
-        .into_iter()
-        .sorted_by(|(_, a), (_, b)| a.cmp(b)))
-}
-
 fn handle_directory(path: &Path) -> Result<TempFile> {
     let temp_dir = TempDir::create()?;
     #[cfg(all(target_os = "macos", target_arch = "aarch64"))]
@@ -296,78 +274,6 @@ fn handle_directory(path: &Path) -> Result<TempFile> {
     normalize_directory(path, temp_dir.path())
 }
 
-// For XCArchive directories, we'll zip the entire directory
-fn normalize_directory(path: &Path, parsed_assets_path: &Path) -> Result<TempFile> {
-    debug!("Creating normalized zip for directory: {}", path.display());
-
-    let temp_file = TempFile::create()?;
-    let mut zip = ZipWriter::new(temp_file.open()?);
-
-    let mut file_count = 0;
-    let directory_name = path.file_name().expect("Failed to get basename");
-
-    // Collect and sort entries for deterministic ordering
-    // This is important to ensure stable sha1 checksums for the zip file as
-    // an optimization is used to avoid re-uploading the same chunks if they're already on the server.
-    let entries = sort_entries(path)?;
-
-    // Need to set the last modified time to a fixed value to ensure consistent checksums
-    // This is important as an optimization to avoid re-uploading the same chunks if they're already on the server
-    // but the last modified time being different will cause checksums to be different.
-    let options = SimpleFileOptions::default()
-        .compression_method(zip::CompressionMethod::Stored)
-        .last_modified_time(DateTime::default());
-
-    for (entry_path, relative_path) in entries {
-        let zip_path = format!(
-            "{}/{}",
-            directory_name.to_string_lossy(),
-            relative_path.to_string_lossy()
-        );
-        debug!("Adding file to zip: {}", zip_path);
-
-        #[cfg(not(windows))]
-        // On Unix, we need to preserve the file permissions.
-        let options = options.unix_permissions(fs::metadata(&entry_path)?.permissions().mode());
-
-        zip.start_file(zip_path, options)?;
-        let file_byteview = ByteView::open(&entry_path)?;
-        zip.write_all(file_byteview.as_slice())?;
-        file_count += 1;
-    }
-
-    // Add parsed assets to the zip in a "ParsedAssets" directory
-    if parsed_assets_path.exists() {
-        debug!(
-            "Adding parsed assets from: {}",
-            parsed_assets_path.display()
-        );
-
-        let parsed_assets_entries = sort_entries(parsed_assets_path)?;
-
-        for (entry_path, relative_path) in parsed_assets_entries {
-            let zip_path = format!(
-                "{}/ParsedAssets/{}",
-                directory_name.to_string_lossy(),
-                relative_path.to_string_lossy()
-            );
-            debug!("Adding parsed asset to zip: {}", zip_path);
-
-            zip.start_file(zip_path, options)?;
-            let file_byteview = ByteView::open(&entry_path)?;
-            zip.write_all(file_byteview.as_slice())?;
-            file_count += 1;
-        }
-    }
-
-    zip.finish()?;
-    debug!(
-        "Successfully created normalized zip for directory with {} files",
-        file_count
-    );
-    Ok(temp_file)
-}
-
 fn upload_file(
     api: &AuthenticatedApi,
     bytes: &[u8],

src/utils/mobile_app/mod.rs

@@ -2,10 +2,12 @@
 
 #[cfg(all(target_os = "macos", target_arch = "aarch64"))]
 mod apple;
+mod normalize;
 mod validation;
 
 #[cfg(all(target_os = "macos", target_arch = "aarch64"))]
 pub use self::apple::{handle_asset_catalogs, ipa_to_xcarchive};
+pub use self::normalize::normalize_directory;
 pub use self::validation::{is_aab_file, is_apk_file, is_zip_file};
 #[cfg(all(target_os = "macos", target_arch = "aarch64"))]
 pub use self::validation::{is_apple_app, is_ipa_file};

src/utils/mobile_app/normalize.rs

@@ -0,0 +1,100 @@
+#[cfg(not(windows))]
+use std::fs;
+use std::fs::File;
+use std::io::Write as _;
+#[cfg(not(windows))]
+use std::os::unix::fs::PermissionsExt as _;
+use std::path::{Path, PathBuf};
+
+use crate::utils::fs::TempFile;
+use anyhow::Result;
+use itertools::Itertools as _;
+use log::debug;
+use symbolic::common::ByteView;
+use walkdir::WalkDir;
+use zip::write::SimpleFileOptions;
+use zip::{DateTime, ZipWriter};
+
+fn sort_entries(path: &Path) -> Result<impl Iterator<Item = (PathBuf, PathBuf)>> {
+    Ok(WalkDir::new(path)
+        .follow_links(true)
+        .into_iter()
+        .filter_map(Result::ok)
+        .filter(|entry| entry.path().is_file())
+        .map(|entry| {
+            let entry_path = entry.into_path();
+            let relative_path = entry_path.strip_prefix(path)?.to_owned();
+            Ok((entry_path, relative_path))
+        })
+        .collect::<Result<Vec<_>>>()?
+        .into_iter()
+        .sorted_by(|(_, a), (_, b)| a.cmp(b)))
+}
+
+fn add_entries_to_zip(
+    zip: &mut ZipWriter<File>,
+    entries: impl Iterator<Item = (PathBuf, PathBuf)>,
+    directory_name: &str,
+) -> Result<i32> {
+    let mut file_count = 0;
+
+    // Need to set the last modified time to a fixed value to ensure consistent checksums
+    // This is important as an optimization to avoid re-uploading the same chunks if they're already on the server
+    // but the last modified time being different will cause checksums to be different.
+    let options = SimpleFileOptions::default()
+        .compression_method(zip::CompressionMethod::Stored)
+        .last_modified_time(DateTime::default());
+
+    for (entry_path, relative_path) in entries {
+        #[cfg(not(windows))]
+        // On Unix, we need to preserve the file permissions.
+        let options = options.unix_permissions(fs::metadata(&entry_path)?.permissions().mode());
+
+        let zip_path = format!("{directory_name}/{}", relative_path.to_string_lossy());
+
+        zip.start_file(zip_path, options)?;
+        let file_byteview = ByteView::open(&entry_path)?;
+        zip.write_all(file_byteview.as_slice())?;
+        file_count += 1;
+    }
+
+    Ok(file_count)
+}
+
+// For XCArchive directories, we'll zip the entire directory
+pub fn normalize_directory(path: &Path, parsed_assets_path: &Path) -> Result<TempFile> {
+    debug!("Creating normalized zip for directory: {}", path.display());
+
+    let temp_file = TempFile::create()?;
+    let mut zip = ZipWriter::new(temp_file.open()?);
+
+    let directory_name = path.file_name().expect("Failed to get basename");
+
+    // Collect and sort entries for deterministic ordering
+    // This is important to ensure stable sha1 checksums for the zip file as
+    // an optimization is used to avoid re-uploading the same chunks if they're already on the server.
+    let entries = sort_entries(path)?;
+    let mut file_count = add_entries_to_zip(&mut zip, entries, &directory_name.to_string_lossy())?;
+
+    // Add parsed assets to the zip in a "ParsedAssets" directory
+    if parsed_assets_path.exists() {
+        debug!(
+            "Adding parsed assets from: {}",
+            parsed_assets_path.display()
+        );
+
+        let parsed_assets_entries = sort_entries(parsed_assets_path)?;
+        file_count += add_entries_to_zip(
+            &mut zip,
+            parsed_assets_entries,
+            &format!("{}/ParsedAssets", directory_name.to_string_lossy()),
+        )?;
+    }
+
+    zip.finish()?;
+    debug!(
+        "Successfully created normalized zip for directory with {} files",
+        file_count
+    );
+    Ok(temp_file)
+}

tests/integration/_cases/mobile_app/mobile_app-upload-xcarchive-all-uploaded.trycmd

@@ -92,25 +92,6 @@ fn command_mobile_app_upload_apk_all_uploaded() {
         .with_default_token();
 }
 
-#[test]
-#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
-fn command_mobile_app_upload_xcarchive_all_uploaded() {
-    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/projects/wat-org/wat-project/files/preprodartifacts/assemble/",
-            )
-            .with_response_body(r#"{"state":"ok","missingChunks":[]}"#),
-        )
-        .register_trycmd_test("mobile_app/mobile_app-upload-xcarchive-all-uploaded.trycmd")
-        .with_default_token();
-}
-
 /// This regex is used to extract the boundary from the content-type header.
 /// We need to match the boundary, since it changes with each request.
 /// The regex matches the format as specified in