Skip to content

Commit 483bdb0

Browse files
noahsmartinnoahsmartin
committed
PR feedback
1 parent 747a231 commit 483bdb0

File tree

5 files changed

+104
-124
lines changed

5 files changed

+104
-124
lines changed

‎src/commands/mobile_app/upload.rs

Lines changed: 2 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,13 @@
11
use std::borrow::Cow;
2-
#[cfg(not(windows))]
3-
use std::fs;
42
use std::io::Write as _;
5-
#[cfg(not(windows))]
6-
use std::os::unix::fs::PermissionsExt as _;
7-
use std::path::{Path, PathBuf};
3+
use std::path::Path;
84

95
use anyhow::{anyhow, bail, Context as _, Result};
106
use clap::{Arg, ArgAction, ArgMatches, Command};
117
use indicatif::ProgressStyle;
12-
use itertools::Itertools as _;
138
use log::{debug, info, warn};
149
use sha1_smol::Digest;
1510
use symbolic::common::ByteView;
16-
use walkdir::WalkDir;
1711
use zip::write::SimpleFileOptions;
1812
use zip::{DateTime, ZipWriter};
1913

@@ -28,7 +22,7 @@ use crate::utils::fs::TempFile;
2822
use crate::utils::mobile_app::{
2923
handle_asset_catalogs, ipa_to_xcarchive, is_apple_app, is_ipa_file,
3024
};
31-
use crate::utils::mobile_app::{is_aab_file, is_apk_file, is_zip_file};
25+
use crate::utils::mobile_app::{is_aab_file, is_apk_file, is_zip_file, normalize_directory};
3226
use crate::utils::progress::ProgressBar;
3327
use crate::utils::vcs;
3428

@@ -271,22 +265,6 @@ fn normalize_file(path: &Path, bytes: &[u8]) -> Result<TempFile> {
271265
Ok(temp_file)
272266
}
273267

274-
fn sort_entries(path: &Path) -> Result<std::vec::IntoIter<(PathBuf, PathBuf)>> {
275-
Ok(WalkDir::new(path)
276-
.follow_links(true)
277-
.into_iter()
278-
.filter_map(Result::ok)
279-
.filter(|entry| entry.path().is_file())
280-
.map(|entry| {
281-
let entry_path = entry.into_path();
282-
let relative_path = entry_path.strip_prefix(path)?.to_owned();
283-
Ok((entry_path, relative_path))
284-
})
285-
.collect::<Result<Vec<_>>>()?
286-
.into_iter()
287-
.sorted_by(|(_, a), (_, b)| a.cmp(b)))
288-
}
289-
290268
fn handle_directory(path: &Path) -> Result<TempFile> {
291269
let temp_dir = TempDir::create()?;
292270
#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
@@ -296,78 +274,6 @@ fn handle_directory(path: &Path) -> Result<TempFile> {
296274
normalize_directory(path, temp_dir.path())
297275
}
298276

299-
// For XCArchive directories, we'll zip the entire directory
300-
fn normalize_directory(path: &Path, parsed_assets_path: &Path) -> Result<TempFile> {
301-
debug!("Creating normalized zip for directory: {}", path.display());
302-
303-
let temp_file = TempFile::create()?;
304-
let mut zip = ZipWriter::new(temp_file.open()?);
305-
306-
let mut file_count = 0;
307-
let directory_name = path.file_name().expect("Failed to get basename");
308-
309-
// Collect and sort entries for deterministic ordering
310-
// This is important to ensure stable sha1 checksums for the zip file as
311-
// an optimization is used to avoid re-uploading the same chunks if they're already on the server.
312-
let entries = sort_entries(path)?;
313-
314-
// Need to set the last modified time to a fixed value to ensure consistent checksums
315-
// This is important as an optimization to avoid re-uploading the same chunks if they're already on the server
316-
// but the last modified time being different will cause checksums to be different.
317-
let options = SimpleFileOptions::default()
318-
.compression_method(zip::CompressionMethod::Stored)
319-
.last_modified_time(DateTime::default());
320-
321-
for (entry_path, relative_path) in entries {
322-
let zip_path = format!(
323-
"{}/{}",
324-
directory_name.to_string_lossy(),
325-
relative_path.to_string_lossy()
326-
);
327-
debug!("Adding file to zip: {}", zip_path);
328-
329-
#[cfg(not(windows))]
330-
// On Unix, we need to preserve the file permissions.
331-
let options = options.unix_permissions(fs::metadata(&entry_path)?.permissions().mode());
332-
333-
zip.start_file(zip_path, options)?;
334-
let file_byteview = ByteView::open(&entry_path)?;
335-
zip.write_all(file_byteview.as_slice())?;
336-
file_count += 1;
337-
}
338-
339-
// Add parsed assets to the zip in a "ParsedAssets" directory
340-
if parsed_assets_path.exists() {
341-
debug!(
342-
"Adding parsed assets from: {}",
343-
parsed_assets_path.display()
344-
);
345-
346-
let parsed_assets_entries = sort_entries(parsed_assets_path)?;
347-
348-
for (entry_path, relative_path) in parsed_assets_entries {
349-
let zip_path = format!(
350-
"{}/ParsedAssets/{}",
351-
directory_name.to_string_lossy(),
352-
relative_path.to_string_lossy()
353-
);
354-
debug!("Adding parsed asset to zip: {}", zip_path);
355-
356-
zip.start_file(zip_path, options)?;
357-
let file_byteview = ByteView::open(&entry_path)?;
358-
zip.write_all(file_byteview.as_slice())?;
359-
file_count += 1;
360-
}
361-
}
362-
363-
zip.finish()?;
364-
debug!(
365-
"Successfully created normalized zip for directory with {} files",
366-
file_count
367-
);
368-
Ok(temp_file)
369-
}
370-
371277
fn upload_file(
372278
api: &AuthenticatedApi,
373279
bytes: &[u8],

‎src/utils/mobile_app/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@
22

33
#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
44
mod apple;
5+
mod normalize;
56
mod validation;
67

78
#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
89
pub use self::apple::{handle_asset_catalogs, ipa_to_xcarchive};
10+
pub use self::normalize::normalize_directory;
911
pub use self::validation::{is_aab_file, is_apk_file, is_zip_file};
1012
#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
1113
pub use self::validation::{is_apple_app, is_ipa_file};

‎src/utils/mobile_app/normalize.rs

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
#[cfg(not(windows))]
2+
use std::fs;
3+
use std::fs::File;
4+
use std::io::Write as _;
5+
#[cfg(not(windows))]
6+
use std::os::unix::fs::PermissionsExt as _;
7+
use std::path::{Path, PathBuf};
8+
9+
use crate::utils::fs::TempFile;
10+
use anyhow::Result;
11+
use itertools::Itertools as _;
12+
use log::debug;
13+
use symbolic::common::ByteView;
14+
use walkdir::WalkDir;
15+
use zip::write::SimpleFileOptions;
16+
use zip::{DateTime, ZipWriter};
17+
18+
fn sort_entries(path: &Path) -> Result<impl Iterator<Item = (PathBuf, PathBuf)>> {
19+
Ok(WalkDir::new(path)
20+
.follow_links(true)
21+
.into_iter()
22+
.filter_map(Result::ok)
23+
.filter(|entry| entry.path().is_file())
24+
.map(|entry| {
25+
let entry_path = entry.into_path();
26+
let relative_path = entry_path.strip_prefix(path)?.to_owned();
27+
Ok((entry_path, relative_path))
28+
})
29+
.collect::<Result<Vec<_>>>()?
30+
.into_iter()
31+
.sorted_by(|(_, a), (_, b)| a.cmp(b)))
32+
}
33+
34+
fn add_entries_to_zip(
35+
zip: &mut ZipWriter<File>,
36+
entries: impl Iterator<Item = (PathBuf, PathBuf)>,
37+
directory_name: &str,
38+
) -> Result<i32> {
39+
let mut file_count = 0;
40+
41+
// Need to set the last modified time to a fixed value to ensure consistent checksums
42+
// This is important as an optimization to avoid re-uploading the same chunks if they're already on the server
43+
// but the last modified time being different will cause checksums to be different.
44+
let options = SimpleFileOptions::default()
45+
.compression_method(zip::CompressionMethod::Stored)
46+
.last_modified_time(DateTime::default());
47+
48+
for (entry_path, relative_path) in entries {
49+
#[cfg(not(windows))]
50+
// On Unix, we need to preserve the file permissions.
51+
let options = options.unix_permissions(fs::metadata(&entry_path)?.permissions().mode());
52+
53+
let zip_path = format!("{}/{}", directory_name, relative_path.to_string_lossy());
54+
55+
zip.start_file(zip_path, options)?;
56+
let file_byteview = ByteView::open(&entry_path)?;
57+
zip.write_all(file_byteview.as_slice())?;
58+
file_count += 1;
59+
}
60+
61+
Ok(file_count)
62+
}
63+
64+
// For XCArchive directories, we'll zip the entire directory
65+
pub fn normalize_directory(path: &Path, parsed_assets_path: &Path) -> Result<TempFile> {
66+
debug!("Creating normalized zip for directory: {}", path.display());
67+
68+
let temp_file = TempFile::create()?;
69+
let mut zip = ZipWriter::new(temp_file.open()?);
70+
71+
let directory_name = path.file_name().expect("Failed to get basename");
72+
73+
// Collect and sort entries for deterministic ordering
74+
// This is important to ensure stable sha1 checksums for the zip file as
75+
// an optimization is used to avoid re-uploading the same chunks if they're already on the server.
76+
let entries = sort_entries(path)?;
77+
let mut file_count = add_entries_to_zip(&mut zip, entries, &directory_name.to_string_lossy())?;
78+
79+
// Add parsed assets to the zip in a "ParsedAssets" directory
80+
if parsed_assets_path.exists() {
81+
debug!(
82+
"Adding parsed assets from: {}",
83+
parsed_assets_path.display()
84+
);
85+
86+
let parsed_assets_entries = sort_entries(parsed_assets_path)?;
87+
file_count += add_entries_to_zip(
88+
&mut zip,
89+
parsed_assets_entries,
90+
&format!("{}/ParsedAssets", directory_name.to_string_lossy()),
91+
)?;
92+
}
93+
94+
zip.finish()?;
95+
debug!(
96+
"Successfully created normalized zip for directory with {} files",
97+
file_count
98+
);
99+
Ok(temp_file)
100+
}

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

Lines changed: 0 additions & 9 deletions
This file was deleted.

‎tests/integration/mobile_app/upload.rs

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -92,25 +92,6 @@ fn command_mobile_app_upload_apk_all_uploaded() {
9292
.with_default_token();
9393
}
9494

95-
#[test]
96-
#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
97-
fn command_mobile_app_upload_xcarchive_all_uploaded() {
98-
TestManager::new()
99-
.mock_endpoint(
100-
MockEndpointBuilder::new("GET", "/api/0/organizations/wat-org/chunk-upload/")
101-
.with_response_file("mobile_app/get-chunk-upload.json"),
102-
)
103-
.mock_endpoint(
104-
MockEndpointBuilder::new(
105-
"POST",
106-
"/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/",
107-
)
108-
.with_response_body(r#"{"state":"ok","missingChunks":[]}"#),
109-
)
110-
.register_trycmd_test("mobile_app/mobile_app-upload-xcarchive-all-uploaded.trycmd")
111-
.with_default_token();
112-
}
113-
11495
/// This regex is used to extract the boundary from the content-type header.
11596
/// We need to match the boundary, since it changes with each request.
11697
/// The regex matches the format as specified in

0 commit comments

Comments
 (0)