ref(snapshots): Address PR review feedback

- Replace self-managed odiff binary download with npm install prompt.
  In interactive mode, offers to run `npm install -g odiff-bin`. In
  non-interactive mode (CI), errors with clear install instructions.
- Move `sentry-cli build snapshots` to `sentry-cli snapshots upload`.
  The old path is kept as a hidden deprecated alias with a warning.
- Add Emerge Tools as CODEOWNERS for snapshots and odiff directories.
- Remove orphaned `tar` dependency.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: NicoHinderling

.github/CODEOWNERS

@@ -16,6 +16,8 @@
 /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
+/src/commands/snapshots @getsentry/emerge-tools @getsentry/owners-sentry-cli
+/src/utils/odiff @getsentry/emerge-tools @getsentry/owners-sentry-cli
 /tests/integration/build @getsentry/emerge-tools @getsentry/owners-sentry-cli
 
 # Files without codeowner protection

Cargo.lock

@@ -69,7 +69,6 @@ sha1_smol = { version = "1.0.0", features = ["serde", "std"] }
 sha2 = "0.10.9"
 sourcemap = { version = "9.3.0", features = ["ram_bundle"] }
 symbolic = { version = "12.13.3", features = ["debuginfo-serde", "il2cpp"] }
-tar = "0.4"
 thiserror = "1.0.38"
 tokio = { version = "1.47", features = ["rt"] }
 url = "2.3.1"

Cargo.toml

@@ -4,13 +4,11 @@ use clap::{ArgMatches, Command};
 use crate::utils::args::ArgExt as _;
 
 pub mod download;
-pub mod snapshots;
 pub mod upload;
 
 macro_rules! each_subcommand {
     ($mac:ident) => {
         $mac!(download);
-        $mac!(snapshots);
         $mac!(upload);
     };
 }
@@ -31,6 +29,11 @@ pub fn make_command(mut command: Command) -> Command {
         .org_arg()
         .project_arg(true);
     each_subcommand!(add_subcommand);
+    command = command.subcommand(
+        crate::commands::snapshots::upload::make_command(Command::new("snapshots"))
+            .hide(true)
+            .about("[DEPRECATED] Use `sentry-cli snapshots upload` instead."),
+    );
     command
 }
 
@@ -45,5 +48,12 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
         }};
     }
     each_subcommand!(execute_subcommand);
+    if let Some(sub_matches) = matches.subcommand_matches("snapshots") {
+        eprintln!(
+            "WARNING: `sentry-cli build snapshots` is deprecated. \
+             Use `sentry-cli snapshots upload` instead."
+        );
+        return crate::commands::snapshots::upload::execute(sub_matches);
+    }
     unreachable!();
 }

src/commands/build/mod.rs

@@ -2,10 +2,12 @@ use anyhow::Result;
 use clap::{ArgMatches, Command};
 
 pub mod diff;
+pub mod upload;
 
 macro_rules! each_subcommand {
     ($mac:ident) => {
         $mac!(diff);
+        $mac!(upload);
     };
 }
 
@@ -19,7 +21,7 @@ pub fn make_command(mut command: Command) -> Command {
     }
 
     command = command
-        .about("[EXPERIMENTAL] Manage and compare snapshots locally.")
+        .about("[EXPERIMENTAL] Manage and compare snapshots.")
         .subcommand_required(true)
         .arg_required_else_help(true);
     each_subcommand!(add_subcommand);

src/commands/snapshots/mod.rs

@@ -26,15 +26,15 @@ use crate::utils::ci::is_ci;
 use crate::utils::fs::IMAGE_EXTENSIONS;
 
 const EXPERIMENTAL_WARNING: &str =
-    "[EXPERIMENTAL] The \"build snapshots\" command is experimental. \
+    "[EXPERIMENTAL] The \"snapshots upload\" command is experimental. \
     The command is subject to breaking changes, including removal, in any Sentry CLI release.";
 const MAX_PIXELS_PER_IMAGE: u64 = 40_000_000;
 
 pub fn make_command(command: Command) -> Command {
     command
-        .about("[EXPERIMENTAL] Upload build snapshots to a project.")
+        .about("[EXPERIMENTAL] Upload snapshots to a project.")
         .long_about(format!(
-            "Upload build snapshots to a project.\n\n{EXPERIMENTAL_WARNING}"
+            "Upload snapshots to a project.\n\n{EXPERIMENTAL_WARNING}"
         ))
         .org_arg()
         .project_arg(false)

src/commands/build/snapshots.rs src/commands/snapshots/upload.rssrc/commands/build/snapshots.rs renamed to src/commands/snapshots/upload.rs

@@ -1,190 +1,104 @@
-use std::fs;
-use std::io::{Read as _, Write as _};
-use std::path::PathBuf;
+use std::io::{self, IsTerminal as _, Write as _};
+use std::path::{Path, PathBuf};
+use std::process::Command;
 
 use anyhow::{bail, Context as _, Result};
 use log::info;
-use sha2::{Digest as _, Sha256};
 
-use crate::config::Config;
-
-const ODIFF_VERSION: &str = "4.3.8";
-const ODIFF_TARBALL_SHA256: &str =
-    "cf5a344223fdbbdd6367a08a412410a0c9e98fea65c9b14efbc745c42f054fb3";
-
-fn odiff_tarball_url() -> String {
-    format!("https://registry.npmjs.org/odiff-bin/-/odiff-bin-{ODIFF_VERSION}.tgz")
-}
-
-fn platform_binary_name() -> Result<&'static str> {
-    match (std::env::consts::OS, std::env::consts::ARCH) {
-        ("macos", "aarch64") => Ok("odiff-macos-arm64"),
-        ("macos", "x86_64") => Ok("odiff-macos-x64"),
-        ("linux", "x86_64") => Ok("odiff-linux-x64"),
-        ("linux", "aarch64") => Ok("odiff-linux-arm64"),
-        ("windows", "x86_64") => Ok("odiff-win-x64.exe"),
-        (os, arch) => bail!("Unsupported platform for odiff: {os}/{arch}"),
-    }
-}
-
-fn cache_dir() -> Result<PathBuf> {
-    let home = dirs::home_dir().context("Could not determine home directory")?;
-    Ok(home.join(".sentry-cli").join("odiff").join(ODIFF_VERSION))
-}
-
-fn download_and_extract() -> Result<PathBuf> {
-    let binary_name = platform_binary_name()?;
-    let cache = cache_dir()?;
-    let binary_path = cache.join(binary_name);
-
-    fs::create_dir_all(&cache).context("Failed to create odiff cache directory")?;
-
-    info!("Downloading odiff {ODIFF_VERSION} from npm registry...");
-
-    let mut data = Vec::new();
-    let mut handle = curl::easy::Easy::new();
-    handle.url(&odiff_tarball_url())?;
-    handle.follow_location(true)?;
-
-    if let Some(config) = Config::current_opt() {
-        if let Some(proxy_url) = config.get_proxy_url() {
-            handle.proxy(&proxy_url)?;
-        }
-        if let Some(proxy_username) = config.get_proxy_username() {
-            handle.proxy_username(proxy_username)?;
-        }
-        if let Some(proxy_password) = config.get_proxy_password() {
-            handle.proxy_password(proxy_password)?;
+const MIN_SUPPORTED_VERSION: &str = "4.0.0";
+
+fn check_version(binary_path: &Path) {
+    let output = Command::new(binary_path).arg("--version").output();
+
+    match output {
+        Ok(out) if out.status.success() => {
+            let version_str = String::from_utf8_lossy(&out.stdout).trim().to_owned();
+            let version = version_str
+                .strip_prefix("odiff ")
+                .unwrap_or(&version_str)
+                .trim();
+
+            let min = semver::Version::parse(MIN_SUPPORTED_VERSION)
+                .expect("MIN_SUPPORTED_VERSION is valid semver");
+            if let Ok(installed) = semver::Version::parse(version) {
+                if installed < min {
+                    eprintln!(
+                        "Warning: odiff {version} is below the minimum supported version \
+                         ({MIN_SUPPORTED_VERSION}). You may experience issues."
+                    );
+                }
+            }
         }
-        handle.ssl_verify_host(config.should_verify_ssl())?;
-        handle.ssl_verify_peer(config.should_verify_ssl())?;
-        let mut ssl_opts = curl::easy::SslOpt::new();
-        if config.disable_ssl_revocation_check() {
-            ssl_opts.no_revoke(true);
+        _ => {
+            eprintln!("Warning: Could not determine odiff version. You may experience issues.");
         }
-        handle.ssl_options(&ssl_opts)?;
     }
+}
 
-    if let Ok(ca_bundle) = std::env::var("CURL_CA_BUNDLE") {
-        handle.cainfo(&ca_bundle)?;
-    } else if let Ok(ca_bundle) = std::env::var("SSL_CERT_FILE") {
-        handle.cainfo(&ca_bundle)?;
-    }
-    {
-        let mut transfer = handle.transfer();
-        transfer.write_function(|chunk| {
-            data.extend_from_slice(chunk);
-            Ok(chunk.len())
-        })?;
-        transfer
-            .perform()
-            .context("Failed to download odiff tarball")?;
+fn prompt_npm_install() -> Result<PathBuf> {
+    if !io::stdin().is_terminal() || !io::stderr().is_terminal() {
+        bail!(
+            "This command requires `odiff`, but it is not installed.\n\n\
+             `odiff` can be installed with npm:\n\n    \
+             npm install -g odiff-bin"
+        );
     }
 
-    let status = handle.response_code()?;
-    if status != 200 {
-        bail!("Failed to download odiff: HTTP {status}");
-    }
+    eprintln!("This command requires `odiff`, but it is not installed.");
+    eprintln!();
+    eprintln!("`odiff` can be installed with npm:");
+    eprintln!();
+    eprintln!("    npm install -g odiff-bin");
+    eprintln!();
+    eprint!("Would you like us to install `odiff` with npm for you? [y/N] ");
+    io::stderr().flush()?;
+
+    let mut input = String::new();
+    io::stdin().read_line(&mut input)?;
 
-    let digest = Sha256::digest(&data);
-    let hash = digest.iter().fold(String::new(), |mut acc, b| {
-        use std::fmt::Write as _;
-        write!(acc, "{b:02x}").expect("writing hex to String should never fail");
-        acc
-    });
-    if hash != ODIFF_TARBALL_SHA256 {
+    if !matches!(input.trim().to_lowercase().as_str(), "y" | "yes") {
         bail!(
-            "odiff tarball integrity check failed: expected SHA-256 {ODIFF_TARBALL_SHA256}, got {hash}"
+            "`odiff` is required but not installed. \
+             Install it with: npm install -g odiff-bin"
         );
     }
 
-    let decoder = flate2::read::GzDecoder::new(data.as_slice());
-    let mut archive = tar::Archive::new(decoder);
-
-    let target_suffix = format!("raw_binaries/{binary_name}");
-
-    for entry in archive
-        .entries()
-        .context("Failed to read tarball entries")?
-    {
-        let mut entry = entry.context("Failed to read tarball entry")?;
-        let path = entry.path().context("Failed to read entry path")?;
-        let path_str = path.to_string_lossy().to_string();
-
-        if path_str.ends_with(&target_suffix) {
-            let mut contents = Vec::new();
-            entry
-                .read_to_end(&mut contents)
-                .context("Failed to extract odiff binary")?;
-
-            let temp_path = binary_path.with_extension("tmp");
-            let mut f = fs::File::create(&temp_path)
-                .context("Failed to create temp file for odiff binary")?;
-            f.write_all(&contents)
-                .context("Failed to write odiff binary")?;
-            f.sync_all()?;
-            drop(f);
-
-            #[cfg(unix)]
-            {
-                use std::os::unix::fs::PermissionsExt as _;
-                let mut perm = fs::metadata(&temp_path)?.permissions();
-                perm.set_mode(0o755);
-                fs::set_permissions(&temp_path, perm)?;
-            }
+    eprintln!();
+    eprintln!("Running `npm install -g odiff-bin`...");
+    eprintln!();
 
-            fs::rename(&temp_path, &binary_path).context("Failed to install odiff binary")?;
+    let status = Command::new("npm")
+        .args(["install", "-g", "odiff-bin"])
+        .status()
+        .context("Failed to run npm. Is npm installed?")?;
 
-            info!(
-                "odiff {ODIFF_VERSION} installed to {}",
-                binary_path.display()
-            );
-            return Ok(binary_path);
-        }
+    if !status.success() {
+        bail!("npm install failed. Please install odiff manually: npm install -g odiff-bin");
     }
 
-    bail!("Could not find {target_suffix} in odiff tarball")
+    which::which("odiff").context(
+        "odiff was installed but could not be found on PATH. \
+         You may need to restart your shell.",
+    )
 }
 
 pub fn ensure_binary() -> Result<PathBuf> {
     if let Ok(system_path) = which::which("odiff") {
         info!("Using system odiff at {}", system_path.display());
+        check_version(&system_path);
         return Ok(system_path);
     }
 
-    let binary_name = platform_binary_name()?;
-    let cache = cache_dir()?;
-    let cached_binary = cache.join(binary_name);
-    if cached_binary.exists() {
-        info!("Using cached odiff at {}", cached_binary.display());
-        return Ok(cached_binary);
-    }
-
-    download_and_extract()
+    prompt_npm_install()
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
 
     #[test]
-    fn test_platform_binary_name_returns_value() {
-        let name = platform_binary_name().expect("should return a binary name");
-        assert!(
-            name.starts_with("odiff-"),
-            "binary name should start with 'odiff-', got: {name}"
-        );
-    }
-
-    #[test]
-    fn test_cache_dir_is_under_home() {
-        let dir = cache_dir().expect("should return a cache dir");
-        let home = dirs::home_dir().expect("should have a home dir");
-        assert!(dir.starts_with(&home), "cache dir should be under home dir");
-        let dir_str = dir.to_string_lossy();
-        assert!(
-            dir_str.ends_with(ODIFF_VERSION),
-            "cache dir should end with version, got: {dir_str}"
-        );
+    fn test_min_version_parses() {
+        semver::Version::parse(MIN_SUPPORTED_VERSION)
+            .expect("MIN_SUPPORTED_VERSION should be valid semver");
     }
 }