diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index f15ac7c9d7..5579263b31 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -13,6 +13,7 @@ # Files co-owned by Emerge Tools team /apple-catalog-parsing @getsentry/emerge-tools @getsentry/owners-sentry-cli +/src/api/data_types/snapshots.rs @getsentry/emerge-tools @getsentry/owners-sentry-cli /src/commands/build @getsentry/emerge-tools @getsentry/owners-sentry-cli /src/utils/build @getsentry/emerge-tools @getsentry/owners-sentry-cli /tests/integration/build @getsentry/emerge-tools @getsentry/owners-sentry-cli diff --git a/src/api/data_types/snapshots.rs b/src/api/data_types/snapshots.rs index 698c45e4a0..62fe80d4dd 100644 --- a/src/api/data_types/snapshots.rs +++ b/src/api/data_types/snapshots.rs @@ -7,7 +7,6 @@ use serde_json::Value; use super::VcsInfo; -const IMAGE_FILE_NAME_FIELD: &str = "image_file_name"; const WIDTH_FIELD: &str = "width"; const HEIGHT_FIELD: &str = "height"; @@ -42,16 +41,7 @@ pub struct ImageMetadata { } impl ImageMetadata { - pub fn new( - image_file_name: String, - width: u32, - height: u32, - mut extra: HashMap, - ) -> Self { - extra.insert( - IMAGE_FILE_NAME_FIELD.to_owned(), - Value::String(image_file_name), - ); + pub fn new(width: u32, height: u32, mut extra: HashMap) -> Self { extra.insert(WIDTH_FIELD.to_owned(), Value::from(width)); extra.insert(HEIGHT_FIELD.to_owned(), Value::from(height)); @@ -68,18 +58,16 @@ mod tests { #[test] fn cli_managed_fields_override_sidecar_fields() { let extra = serde_json::from_value(json!({ - (IMAGE_FILE_NAME_FIELD): "from-sidecar.png", (WIDTH_FIELD): 1, (HEIGHT_FIELD): 2, "custom": "keep-me" })) .unwrap(); - let metadata = ImageMetadata::new("from-cli.png".to_owned(), 100, 200, extra); + let metadata = ImageMetadata::new(100, 200, extra); let serialized = serde_json::to_value(metadata).unwrap(); let expected = json!({ - (IMAGE_FILE_NAME_FIELD): "from-cli.png", (WIDTH_FIELD): 100, (HEIGHT_FIELD): 200, "custom": "keep-me" diff --git a/src/commands/build/snapshots.rs b/src/commands/build/snapshots.rs index 25d74e09d5..cc48284323 100644 --- a/src/commands/build/snapshots.rs +++ b/src/commands/build/snapshots.rs @@ -331,11 +331,28 @@ fn upload_images( let mut many_builder = session.many(); let mut manifest_entries = HashMap::new(); - let image_count = images.len(); - + let mut collisions: HashMap> = HashMap::new(); + let mut kept_paths: HashMap = HashMap::new(); for image in images { debug!("Processing image: {}", image.path.display()); + let image_file_name = image + .relative_path + .file_name() + .unwrap_or_default() + .to_string_lossy() + .into_owned(); + + let relative_path = crate::utils::fs::path_as_url(&image.relative_path); + + if manifest_entries.contains_key(&image_file_name) { + collisions + .entry(image_file_name) + .or_default() + .push(relative_path); + continue; + } + let hash = compute_sha256_hash(&image.path)?; let file = runtime .block_on(tokio::fs::File::open(&image.path)) @@ -353,33 +370,40 @@ fn upload_images( .expiration_policy(expiration), ); - let image_file_name = image - .relative_path - .file_name() - .unwrap_or_default() - .to_string_lossy() - .into_owned(); - - let extra = read_sidecar_metadata(&image.path).unwrap_or_else(|err| { + let mut extra = read_sidecar_metadata(&image.path).unwrap_or_else(|err| { warn!("Error reading sidecar metadata, ignoring it instead: {err:#}"); HashMap::new() }); + extra.insert("content_hash".to_owned(), serde_json::Value::String(hash)); + kept_paths.insert(image_file_name.clone(), relative_path); manifest_entries.insert( - hash, - ImageMetadata::new(image_file_name, image.width, image.height, extra), + image_file_name, + ImageMetadata::new(image.width, image.height, extra), ); } + if !collisions.is_empty() { + let mut details = String::new(); + for (name, excluded_paths) in &collisions { + let mut all_paths = vec![kept_paths[name].as_str()]; + all_paths.extend(excluded_paths.iter().map(|s| s.as_str())); + details.push_str(&format!("\n {name}: {}", all_paths.join(", "))); + } + warn!("Some images share identical file names. Only the first occurrence of each is included:{details}"); + } + let result = runtime.block_on(async { many_builder.send().error_for_failures().await }); + let uploaded_count = manifest_entries.len(); + match result { Ok(()) => { println!( "{} Uploaded {} image {}", style(">").dim(), - style(image_count).yellow(), - if image_count == 1 { "file" } else { "files" } + style(uploaded_count).yellow(), + if uploaded_count == 1 { "file" } else { "files" } ); Ok(manifest_entries) } @@ -391,7 +415,7 @@ fn upload_images( eprintln!(" {}", style(format!("{error:#}")).red()); error_count += 1; } - anyhow::bail!("Failed to upload {error_count} out of {image_count} images") + anyhow::bail!("Failed to upload {error_count} out of {uploaded_count} images") } } }