fix: address open PR review threads

dcbuilder.eth · dcbuilder.eth · commit 94d870a0d078 · 2026-02-16T13:29:04.000+01:00
diff --git a/.github/actions/mobench/action.yml b/.github/actions/mobench/action.yml
@@ -152,40 +152,69 @@ runs:
     - name: Install mobench
       if: inputs.install-mobench == 'true'
       shell: bash
+      env:
+        MOBENCH_VERSION: ${{ inputs.mobench-version }}
       run: |
         if command -v cargo-mobench >/dev/null 2>&1 || command -v mobench >/dev/null 2>&1; then
           exit 0
         fi
-        version_flag=""
-        if [ -n "${{ inputs.mobench-version }}" ]; then
-          version_flag="--version ${{ inputs.mobench-version }}"
+        install_args=()
+        if [ -n "$MOBENCH_VERSION" ]; then
+          install_args=(--version "$MOBENCH_VERSION")
         fi
         if command -v cargo-binstall >/dev/null 2>&1; then
-          cargo binstall -y mobench $version_flag
+          cargo binstall -y mobench "${install_args[@]}"
         else
           cargo install cargo-binstall
-          cargo binstall -y mobench $version_flag
+          cargo binstall -y mobench "${install_args[@]}"
         fi
 
     - name: Run mobench
       shell: bash
+      env:
+        ANDROID_SDK_ROOT_INPUT: ${{ inputs.android-sdk-root }}
+        NDK_VERSION_INPUT: ${{ inputs.ndk-version }}
+        MOBENCH_CI: ${{ inputs.ci }}
+        MOBENCH_COMMAND: ${{ inputs.command }}
+        MOBENCH_RUN_ARGS: ${{ inputs.run-args }}
       run: |
-        export ANDROID_SDK_ROOT="${{ inputs.android-sdk-root }}"
-        export ANDROID_HOME="${{ inputs.android-sdk-root }}"
-        export ANDROID_NDK_HOME="${{ inputs.android-sdk-root }}/ndk/${{ inputs.ndk-version }}"
-        extra_args=""
-        if [ "${{ inputs.ci }}" = "true" ] && [ "${{ inputs.command }}" = "cargo mobench run" ]; then
-          extra_args="--ci"
+        export ANDROID_SDK_ROOT="$ANDROID_SDK_ROOT_INPUT"
+        export ANDROID_HOME="$ANDROID_SDK_ROOT_INPUT"
+        export ANDROID_NDK_HOME="$ANDROID_SDK_ROOT_INPUT/ndk/$NDK_VERSION_INPUT"
+        extra_args=()
+        if [ "$MOBENCH_CI" = "true" ] && [ "$MOBENCH_COMMAND" = "cargo mobench run" ]; then
+          extra_args=(--ci)
+        fi
+        case "$MOBENCH_COMMAND" in
+          "cargo mobench run"|"cargo mobench ci run")
+            ;;
+          *)
+            echo "Unsupported command input: $MOBENCH_COMMAND"
+            exit 1
+            ;;
+        esac
+        read -r -a command_parts <<< "$MOBENCH_COMMAND"
+        run_args=()
+        if [ -n "$MOBENCH_RUN_ARGS" ]; then
+          while IFS= read -r line; do
+            if [ -z "$line" ]; then
+              continue
+            fi
+            read -r -a parts <<< "$line"
+            run_args+=("${parts[@]}")
+          done <<< "$MOBENCH_RUN_ARGS"
         fi
-        ${{ inputs.command }} $extra_args ${{ inputs.run-args }}
+        "${command_parts[@]}" "${extra_args[@]}" "${run_args[@]}"
 
     - name: Publish sticky PR comment
       if: always() && inputs.pr-comment == 'true'
       shell: bash
       env:
         GITHUB_TOKEN: ${{ inputs.github-token }}
+        PR_NUMBER_INPUT: ${{ inputs.pr-number }}
+        PR_COMMENT_MARKER: ${{ inputs.pr-comment-marker }}
       run: |
-        pr_number="${{ inputs.pr-number }}"
+        pr_number="$PR_NUMBER_INPUT"
         if [ -z "$pr_number" ] && [ -n "${GITHUB_REF:-}" ]; then
           pr_number="$(echo "$GITHUB_REF" | awk -F/ '/^refs\/pull\// { print $3 }')"
         fi
@@ -204,7 +233,7 @@ runs:
         cargo mobench report github \
           --pr "$pr_number" \
           --summary target/mobench/ci/summary.json \
-          --marker "${{ inputs.pr-comment-marker }}" \
+          --marker "$PR_COMMENT_MARKER" \
           --publish
 
     - name: Upload artifacts
diff --git a/crates/mobench/src/lib.rs b/crates/mobench/src/lib.rs
@@ -1245,17 +1245,44 @@ pub fn run() -> Result<()> {
                 println!("No BrowserStack run to fetch (devices not provided?)");
             }
 
+            let mut baseline_compare_path = None;
+            let mut baseline_snapshot_path = None;
+            if let Some(baseline_source) = baseline.as_deref() {
+                let resolved_baseline = resolve_baseline_source(baseline_source)?;
+                if paths_point_to_same_file(&resolved_baseline, &summary_paths.json)? {
+                    if !resolved_baseline.exists() {
+                        bail!(
+                            "config_error: baseline source `{}` resolves to output path {}; provide an existing baseline file or a different path",
+                            baseline_source,
+                            summary_paths.json.display()
+                        );
+                    }
+                    let snapshot = snapshot_baseline_for_compare(&resolved_baseline)?;
+                    baseline_snapshot_path = Some(snapshot.clone());
+                    baseline_compare_path = Some(snapshot);
+                } else {
+                    baseline_compare_path = Some(resolved_baseline);
+                }
+            }
+
             run_summary.summary = build_summary(&run_summary)?;
             write_summary(&run_summary, &summary_paths, summary_csv)?;
 
             let mut compare_report = None;
             let mut regression_findings: Vec<RegressionFinding> = Vec::new();
-            if let Some(baseline_source) = baseline.as_deref() {
-                let baseline_path = resolve_baseline_source(baseline_source)?;
+            if let Some(baseline_path) = baseline_compare_path.as_deref() {
                 let report = compare_summaries(&baseline_path, &summary_paths.json)?;
                 regression_findings = detect_regressions(&report, regression_threshold_pct);
                 compare_report = Some(report);
             }
+            if let Some(snapshot_path) = baseline_snapshot_path {
+                if let Err(err) = fs::remove_file(&snapshot_path) {
+                    eprintln!(
+                        "Warning: failed to remove baseline snapshot {}: {err}",
+                        snapshot_path.display()
+                    );
+                }
+            }
             if let Some(report) = &compare_report {
                 inject_compare_into_summary(
                     &summary_paths.json,
@@ -2450,7 +2477,8 @@ fn resolve_run_spec(
 ) -> Result<RunSpec> {
     if let Some(cfg_path) = config {
         let cfg = load_config(cfg_path)?;
-        let matrix = load_device_matrix(&cfg.device_matrix)?;
+        let matrix_path = device_matrix.unwrap_or(cfg.device_matrix.as_path());
+        let matrix = load_device_matrix(matrix_path)?;
         let resolved_tags = if !device_tags.is_empty() {
             Some(device_tags)
         } else {
@@ -3717,6 +3745,37 @@ fn resolve_baseline_source(source: &str) -> Result<PathBuf> {
     Ok(PathBuf::from(trimmed))
 }
 
+fn normalized_path(path: &Path) -> Result<PathBuf> {
+    let absolute = if path.is_absolute() {
+        path.to_path_buf()
+    } else {
+        env::current_dir()
+            .context("resolving current directory for baseline path comparison")?
+            .join(path)
+    };
+    Ok(fs::canonicalize(&absolute).unwrap_or(absolute))
+}
+
+fn paths_point_to_same_file(lhs: &Path, rhs: &Path) -> Result<bool> {
+    Ok(normalized_path(lhs)? == normalized_path(rhs)?)
+}
+
+fn snapshot_baseline_for_compare(path: &Path) -> Result<PathBuf> {
+    let stamp = SystemTime::now()
+        .duration_since(UNIX_EPOCH)
+        .unwrap_or_else(|_| Duration::from_secs(0))
+        .as_nanos();
+    let snapshot_path = env::temp_dir().join(format!("mobench-baseline-{stamp}.json"));
+    fs::copy(path, &snapshot_path).with_context(|| {
+        format!(
+            "copying baseline snapshot from {} to {}",
+            path.display(),
+            snapshot_path.display()
+        )
+    })?;
+    Ok(snapshot_path)
+}
+
 fn resolve_artifact_baseline(reference: &str) -> Result<PathBuf> {
     if reference.is_empty() {
         bail!("config_error: baseline artifact reference is empty");
@@ -6692,6 +6751,7 @@ fn check_xcodegen() -> PrereqCheck {
 mod tests {
     use super::*;
     use jsonschema::JSONSchema;
+    use tempfile::TempDir;
 
     // Register a lightweight benchmark for tests so the inventory contains at least one entry.
     #[mobench_sdk::benchmark]
@@ -6724,6 +6784,84 @@ mod tests {
         assert!(spec.ios_xcuitest.is_none());
     }
 
+    #[test]
+    fn resolve_run_spec_prefers_cli_device_matrix_with_config() {
+        let temp_dir = TempDir::new().expect("temp dir");
+        let config_matrix_path = temp_dir.path().join("config-matrix.yml");
+        let cli_matrix_path = temp_dir.path().join("cli-matrix.yml");
+        let config_path = temp_dir.path().join("bench-config.toml");
+
+        write_file(
+            &config_matrix_path,
+            br#"devices:
+  - name: Config Device
+    os: android
+    os_version: "14"
+"#,
+        )
+        .expect("write config matrix");
+        write_file(
+            &cli_matrix_path,
+            br#"devices:
+  - name: CLI Device
+    os: android
+    os_version: "14"
+"#,
+        )
+        .expect("write cli matrix");
+
+        let config_toml = format!(
+            r#"target = "android"
+function = "sample_fns::fibonacci"
+iterations = 10
+warmup = 2
+device_matrix = "{}"
+
+[browserstack]
+app_automate_username = "user"
+app_automate_access_key = "key"
+project = "proj"
+"#,
+            config_matrix_path.display()
+        );
+        write_file(&config_path, config_toml.as_bytes()).expect("write config");
+
+        let spec = resolve_run_spec(
+            MobileTarget::Android,
+            "ignored::value".into(),
+            1,
+            0,
+            Vec::new(),
+            Some(config_path.as_path()),
+            Some(cli_matrix_path.as_path()),
+            Vec::new(),
+            None,
+            None,
+            false,
+            false,
+        )
+        .expect("resolve spec");
+
+        assert_eq!(spec.devices, vec!["CLI Device".to_string()]);
+    }
+
+    #[test]
+    fn snapshot_baseline_creates_distinct_copy() {
+        let temp_dir = TempDir::new().expect("temp dir");
+        let baseline = temp_dir.path().join("baseline.json");
+        write_file(&baseline, br#"{"ok":true}"#).expect("write baseline");
+
+        assert!(paths_point_to_same_file(&baseline, &baseline).expect("compare path"));
+
+        let snapshot = snapshot_baseline_for_compare(&baseline).expect("snapshot baseline");
+        assert_ne!(snapshot, baseline);
+        let original_contents = fs::read_to_string(&baseline).expect("read baseline");
+        let snapshot_contents = fs::read_to_string(&snapshot).expect("read snapshot");
+        assert_eq!(snapshot_contents, original_contents);
+
+        fs::remove_file(snapshot).expect("remove snapshot");
+    }
+
     #[test]
     fn local_smoke_produces_samples() {
         let spec = RunSpec {
diff --git a/crates/mobench/templates/ci/action.yml b/crates/mobench/templates/ci/action.yml
@@ -152,40 +152,69 @@ runs:
     - name: Install mobench
       if: inputs.install-mobench == 'true'
       shell: bash
+      env:
+        MOBENCH_VERSION: ${{ inputs.mobench-version }}
       run: |
         if command -v cargo-mobench >/dev/null 2>&1 || command -v mobench >/dev/null 2>&1; then
           exit 0
         fi
-        version_flag=""
-        if [ -n "${{ inputs.mobench-version }}" ]; then
-          version_flag="--version ${{ inputs.mobench-version }}"
+        install_args=()
+        if [ -n "$MOBENCH_VERSION" ]; then
+          install_args=(--version "$MOBENCH_VERSION")
         fi
         if command -v cargo-binstall >/dev/null 2>&1; then
-          cargo binstall -y mobench $version_flag
+          cargo binstall -y mobench "${install_args[@]}"
         else
           cargo install cargo-binstall
-          cargo binstall -y mobench $version_flag
+          cargo binstall -y mobench "${install_args[@]}"
         fi
 
     - name: Run mobench
       shell: bash
+      env:
+        ANDROID_SDK_ROOT_INPUT: ${{ inputs.android-sdk-root }}
+        NDK_VERSION_INPUT: ${{ inputs.ndk-version }}
+        MOBENCH_CI: ${{ inputs.ci }}
+        MOBENCH_COMMAND: ${{ inputs.command }}
+        MOBENCH_RUN_ARGS: ${{ inputs.run-args }}
       run: |
-        export ANDROID_SDK_ROOT="${{ inputs.android-sdk-root }}"
-        export ANDROID_HOME="${{ inputs.android-sdk-root }}"
-        export ANDROID_NDK_HOME="${{ inputs.android-sdk-root }}/ndk/${{ inputs.ndk-version }}"
-        extra_args=""
-        if [ "${{ inputs.ci }}" = "true" ] && [ "${{ inputs.command }}" = "cargo mobench run" ]; then
-          extra_args="--ci"
+        export ANDROID_SDK_ROOT="$ANDROID_SDK_ROOT_INPUT"
+        export ANDROID_HOME="$ANDROID_SDK_ROOT_INPUT"
+        export ANDROID_NDK_HOME="$ANDROID_SDK_ROOT_INPUT/ndk/$NDK_VERSION_INPUT"
+        extra_args=()
+        if [ "$MOBENCH_CI" = "true" ] && [ "$MOBENCH_COMMAND" = "cargo mobench run" ]; then
+          extra_args=(--ci)
+        fi
+        case "$MOBENCH_COMMAND" in
+          "cargo mobench run"|"cargo mobench ci run")
+            ;;
+          *)
+            echo "Unsupported command input: $MOBENCH_COMMAND"
+            exit 1
+            ;;
+        esac
+        read -r -a command_parts <<< "$MOBENCH_COMMAND"
+        run_args=()
+        if [ -n "$MOBENCH_RUN_ARGS" ]; then
+          while IFS= read -r line; do
+            if [ -z "$line" ]; then
+              continue
+            fi
+            read -r -a parts <<< "$line"
+            run_args+=("${parts[@]}")
+          done <<< "$MOBENCH_RUN_ARGS"
         fi
-        ${{ inputs.command }} $extra_args ${{ inputs.run-args }}
+        "${command_parts[@]}" "${extra_args[@]}" "${run_args[@]}"
 
     - name: Publish sticky PR comment
       if: always() && inputs.pr-comment == 'true'
       shell: bash
       env:
         GITHUB_TOKEN: ${{ inputs.github-token }}
+        PR_NUMBER_INPUT: ${{ inputs.pr-number }}
+        PR_COMMENT_MARKER: ${{ inputs.pr-comment-marker }}
       run: |
-        pr_number="${{ inputs.pr-number }}"
+        pr_number="$PR_NUMBER_INPUT"
         if [ -z "$pr_number" ] && [ -n "${GITHUB_REF:-}" ]; then
           pr_number="$(echo "$GITHUB_REF" | awk -F/ '/^refs\/pull\// { print $3 }')"
         fi
@@ -204,7 +233,7 @@ runs:
         cargo mobench report github \
           --pr "$pr_number" \
           --summary target/mobench/ci/summary.json \
-          --marker "${{ inputs.pr-comment-marker }}" \
+          --marker "$PR_COMMENT_MARKER" \
           --publish
 
     - name: Upload artifacts
