Repository: mozilla/cargo-vet (rust)

Issue:
Consider pruning publisher entries even if `prune_imports` is disabled when a new publisher entry is being added for a crate
In order to avoid unnecessary changes to files such as `imports.lock` cargo-vet makes some effort to avoid removing entries from the file unless requested. The main way this happens in normal operation is when `cargo vet certify` is used to add a new audit for a crate, which can lead to other exemptions, imports, etc. for the crate to be removed.

Unfortunately this pattern doesn't work super well for crates with wildcard audits or trusted entries. For these crates, generally adding a dependency does not require the user running any command other than `cargo vet`, which does not prune unused entries, which means that the previous `publisher` entries are left behind and not cleaned up.

We could consider automatically pruning `publisher` entries for crates even when `prune_imports` is disabled so long as a new publisher entry is being added for that crate. This still avoids unnecessary updates, but means we can automatically clean up publisher entries when new entries are being added anyway.

We could also consider doing this for other forms of imports such as normal and wildcard audits, though removing those from `imports.lock` due to them becoming unnecessary is less frequent during normal dependency updates.

Pull request description:
Always prune imports when importing new audits for a crate
Currently we only prune imports when doing an explicit  prune or when certifying a new audit for the crate. This patch updates the behaviour to also prune imports whenever we are otherwise also adding new imports for the given crate.

This gives us an opportunity to automatically prune old publisher entries when updating a dependency with a trusted entry or wildcard audit, which otherwise wouldn't happen until `cargo vet prune` is explicitly called.

This pruning happens after resolution, so existing imports will still be preferred, and `cargo vet prune` may still perform additional cleanup. Exemptions, git audits, and unpublished entries aren't impacted.

Fixes #620

Patch:
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index a41298a..502a5ff 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -1,4 +1,4 @@
-# Copyright 2022-2024, axodotdev
+# Copyright 2022-2023, axodotdev
 # SPDX-License-Identifier: MIT or Apache-2.0
 #
 # CI that:
@@ -6,14 +6,15 @@
 # * checks for a Git Tag that looks like a release
 # * builds artifacts with cargo-dist (archives, installers, hashes)
 # * uploads those artifacts to temporary workflow zip
-# * on success, uploads the artifacts to a GitHub Release
+# * on success, uploads the artifacts to a Github Release
 #
-# Note that the GitHub Release will be created with a generated
+# Note that the Github Release will be created with a generated
 # title/body based on your changelogs.
 
 name: Release
+
 permissions:
-  "contents": "write"
+  contents: write
 
 # This task will run whenever you push a git tag that looks like a version
 # like "1.0.0", "v0.1.0-prerelease.1", "my-app/0.1.0", "releases/v1.0.0", etc.
@@ -30,22 +31,22 @@ permissions:
 # packages versioned/released in lockstep).
 #
 # If you push multiple tags at once, separate instances of this workflow will
-# spin up, creating an independent announcement for each one. However, GitHub
+# spin up, creating an independent announcement for each one. However Github
 # will hard limit this to 3 tags per commit, as it will assume more tags is a
 # mistake.
 #
 # If there's a prerelease-style suffix to the version, then the release(s)
 # will be marked as a prerelease.
 on:
-  pull_request:
   push:
     tags:
       - '**[0-9]+.[0-9]+.[0-9]+*'
+  pull_request:
 
 jobs:
   # Run 'cargo dist plan' (or host) to determine what tasks we need to do
   plan:
-    runs-on: "ubuntu-20.04"
+    runs-on: ubuntu-latest
     outputs:
       val: ${{ steps.plan.outputs.manifest }}
       tag: ${{ !github.event.pull_request && github.ref_name || '' }}
@@ -61,12 +62,7 @@ jobs:
         # we specify bash to get pipefail; it guards against the `curl` command
         # failing. otherwise `sh` won't catch that `curl` returned non-0
         shell: bash
-        run: "curl --proto '=https' --tlsv1.2 -LsSf https://github.com/axodotdev/cargo-dist/releases/download/v0.18.0/cargo-dist-installer.sh | sh"
-      - name: Cache cargo-dist
-        uses: actions/upload-artifact@v4
-        with:
-          name: cargo-dist-cache
-          path: ~/.cargo/bin/cargo-dist
+        run: "curl --proto '=https' --tlsv1.2 -LsSf https://github.com/axodotdev/cargo-dist/releases/download/v0.12.0/cargo-dist-installer.sh | sh"
       # sure would be cool if github gave us proper conditionals...
       # so here's a doubly-nested ternary-via-truthiness to try to provide the best possible
       # functionality based on whether this is a pull_request, and whether it's from a fork.
@@ -109,12 +105,10 @@ jobs:
       GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
       BUILD_MANIFEST_NAME: target/distrib/${{ join(matrix.targets, '-') }}-dist-manifest.json
     steps:
-      - name: enable windows longpaths
-        run: |
-          git config --global core.longpaths true
       - uses: actions/checkout@v4
         with:
           submodules: recursive
+      - uses: swatinem/rust-cache@v2
       - name: Install cargo-dist
         run: ${{ matrix.install_dist }}
       # Get the dist-manifest
@@ -141,7 +135,7 @@ jobs:
         run: |
           # Parse out what we just built and upload it to scratch storage
           echo "paths<<EOF" >> "$GITHUB_OUTPUT"
-          jq --raw-output ".upload_files[]" dist-manifest.json >> "$GITHUB_OUTPUT"
+          jq --raw-output ".artifacts[]?.path | select( . != null )" dist-manifest.json >> "$GITHUB_OUTPUT"
           echo "EOF" >> "$GITHUB_OUTPUT"
 
           cp dist-manifest.json "$BUILD_MANIFEST_NAME"
@@ -166,12 +160,9 @@ jobs:
       - uses: actions/checkout@v4
         with:
           submodules: recursive
-      - name: Install cached cargo-dist
-        uses: actions/download-artifact@v4
-        with:
-          name: cargo-dist-cache
-          path: ~/.cargo/bin/
-      - run: chmod +x ~/.cargo/bin/cargo-dist
+      - name: Install cargo-dist
+        shell: bash
+        run: "curl --proto '=https' --tlsv1.2 -LsSf https://github.com/axodotdev/cargo-dist/releases/download/v0.12.0/cargo-dist-installer.sh | sh"
       # Get all the local artifacts for the global tasks to use (for e.g. checksums)
       - name: Fetch local artifacts
         uses: actions/download-artifact@v4
@@ -187,7 +178,7 @@ jobs:
 
           # Parse out what we just built and upload it to scratch storage
           echo "paths<<EOF" >> "$GITHUB_OUTPUT"
-          jq --raw-output ".upload_files[]" dist-manifest.json >> "$GITHUB_OUTPUT"
+          jq --raw-output ".artifacts[]?.path | select( . != null )" dist-manifest.json >> "$GITHUB_OUTPUT"
           echo "EOF" >> "$GITHUB_OUTPUT"
 
           cp dist-manifest.json "$BUILD_MANIFEST_NAME"
@@ -215,12 +206,8 @@ jobs:
       - uses: actions/checkout@v4
         with:
           submodules: recursive
-      - name: Install cached cargo-dist
-        uses: actions/download-artifact@v4
-        with:
-          name: cargo-dist-cache
-          path: ~/.cargo/bin/
-      - run: chmod +x ~/.cargo/bin/cargo-dist
+      - name: Install cargo-dist
+        run: "curl --proto '=https' --tlsv1.2 -LsSf https://github.com/axodotdev/cargo-dist/releases/download/v0.12.0/cargo-dist-installer.sh | sh"
       # Fetch artifacts from scratch-storage
       - name: Fetch artifacts
         uses: actions/download-artifact@v4
@@ -228,6 +215,7 @@ jobs:
           pattern: artifacts-*
           path: target/distrib/
           merge-multiple: true
+      # This is a harmless no-op for Github Releases, hosting for that happens in "announce"
       - id: host
         shell: bash
         run: |
@@ -241,29 +229,8 @@ jobs:
           # Overwrite the previous copy
           name: artifacts-dist-manifest
           path: dist-manifest.json
-      # Create a GitHub Release while uploading all files to it
-      - name: "Download GitHub Artifacts"
-        uses: actions/download-artifact@v4
-        with:
-          pattern: artifacts-*
-          path: artifacts
-          merge-multiple: true
-      - name: Cleanup
-        run: |
-          # Remove the granular manifests
-          rm -f artifacts/*-dist-manifest.json
-      - name: Create GitHub Release
-        env:
-          PRERELEASE_FLAG: "${{ fromJson(steps.host.outputs.manifest).announcement_is_prerelease && '--prerelease' || '' }}"
-          ANNOUNCEMENT_TITLE: "${{ fromJson(steps.host.outputs.manifest).announcement_title }}"
-          ANNOUNCEMENT_BODY: "${{ fromJson(steps.host.outputs.manifest).announcement_github_body }}"
-          RELEASE_COMMIT: "${{ github.sha }}"
-        run: |
-          # Write and read notes from a file to avoid quoting breaking things
-          echo "$ANNOUNCEMENT_BODY" > $RUNNER_TEMP/notes.txt
-
-          gh release create "${{ needs.plan.outputs.tag }}" --target "$RELEASE_COMMIT" $PRERELEASE_FLAG --title "$ANNOUNCEMENT_TITLE" --notes-file "$RUNNER_TEMP/notes.txt" artifacts/*
 
+  # Create a Github Release while uploading all files to it
   announce:
     needs:
       - plan
@@ -279,3 +246,21 @@ jobs:
       - uses: actions/checkout@v4
         with:
           submodules: recursive
+      - name: "Download Github Artifacts"
+        uses: actions/download-artifact@v4
+        with:
+          pattern: artifacts-*
+          path: artifacts
+          merge-multiple: true
+      - name: Cleanup
+        run: |
+          # Remove the granular manifests
+          rm -f artifacts/*-dist-manifest.json
+      - name: Create Github Release
+        uses: ncipollo/release-action@v1
+        with:
+          tag: ${{ needs.plan.outputs.tag }}
+          name: ${{ fromJson(needs.host.outputs.val).announcement_title }}
+          body: ${{ fromJson(needs.host.outputs.val).announcement_github_body }}
+          prerelease: ${{ fromJson(needs.host.outputs.val).announcement_is_prerelease }}
+          artifacts: "artifacts/*"
diff --git a/Cargo.toml b/Cargo.toml
index 0db3428..c87859c 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -71,9 +71,9 @@ lto = "thin"
 # Config for 'cargo dist'
 [workspace.metadata.dist]
 # The preferred cargo-dist version to use in CI (Cargo.toml SemVer syntax)
-cargo-dist-version = "0.18.0"
+cargo-dist-version = "0.12.0"
 # CI backends to support
-ci = "github"
+ci = ["github"]
 # The installers to generate for each app
 installers = []
 # Target platforms to build apps for (Rust target-triple syntax)
diff --git a/src/errors.rs b/src/errors.rs
index b9446c6..ae5f366 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -471,7 +471,7 @@ pub struct InvalidCriteriaError {
 
 #[derive(Debug, Error, Diagnostic)]
 #[error("A file in the store is not correctly formatted:\n\n{unified_diff}")]
-#[diagnostic(help("run `cargo vet fmt` to reformat files in the store"))]
+#[diagnostic(help("run `cargo vet` without --locked to reformat files in the store"))]
 pub struct BadFormatError {
     pub unified_diff: String,
 }
diff --git a/src/resolver.rs b/src/resolver.rs
index 1c79cb5..daf0ba1 100644
--- a/src/resolver.rs
+++ b/src/resolver.rs
@@ -2858,6 +2858,39 @@ pub fn update_store(
     get_store_updates(cfg, store, mode).apply(store);
 }
 
+/// Helper function to determine if we should be pruning imports.
+///
+/// We always prune if requested, but will also prune imports if a new audit or
+/// publisher entry is being added for the crate.
+fn should_prune_imports(
+    store: &Store,
+    required_entries: &Option<SortedMap<RequiredEntry, CriteriaSet>>,
+    mode: UpdateMode,
+    pkgname: PackageStr<'_>,
+) -> bool {
+    if mode.prune_imports {
+        true
+    } else if let Some(required_entries) = required_entries {
+        let import = |idx| store.imported_audits().values().nth(idx).unwrap();
+        required_entries.keys().any(|entry| match entry {
+            RequiredEntry::Audit {
+                import_index,
+                audit_index,
+            } => import(*import_index).audits[pkgname][*audit_index].is_fresh_import,
+            RequiredEntry::WildcardAudit {
+                import_index,
+                audit_index,
+            } => import(*import_index).wildcard_audits[pkgname][*audit_index].is_fresh_import,
+            RequiredEntry::Publisher { publisher_index } => {
+                store.publishers()[pkgname][*publisher_index].is_fresh_import
+            }
+            _ => false,
+        })
+    } else {
+        false
+    }
+}
+
 /// The non-mutating core of `update_store` for use in non-mutating situations.
 pub(crate) fn get_store_updates(
     cfg: &Config,
@@ -2932,10 +2965,11 @@ pub(crate) fn get_store_updates(
                 .wildcard_audits
                 .iter()
                 .map(|(pkgname, wildcard_audits)| {
-                    let prune_imports = mode(&pkgname[..]).prune_imports;
                     let required_entries = required_entries
                         .get(&pkgname[..])
                         .unwrap_or(&no_required_entries);
+                    let prune_imports =
+                        should_prune_imports(store, required_entries, mode(&pkgname[..]), pkgname);
                     (
                         pkgname,
                         wildcard_audits
@@ -2974,12 +3008,13 @@ pub(crate) fn get_store_updates(
                 .audits
                 .iter()
                 .map(|(pkgname, audits)| {
-                    let prune_imports = mode(&pkgname[..]).prune_imports;
                     let (uses_package, required_entries) = match required_entries.get(&pkgname[..])
                     {
                         Some(e) => (true, e),
                         None => (false, &no_required_entries),
                     };
+                    let prune_imports =
+                        should_prune_imports(store, required_entries, mode(&pkgname[..]), pkgname);
                     (
                         pkgname,
                         audits
@@ -3029,10 +3064,11 @@ pub(crate) fn get_store_updates(
 
     // Determine which live publisher information to keep in the imports.lock file.
     for (pkgname, publishers) in store.publishers() {
-        let prune_imports = mode(&pkgname[..]).prune_imports;
         let required_entries = required_entries
             .get(&pkgname[..])
             .unwrap_or(&no_required_entries);
+        let prune_imports =
+            should_prune_imports(store, required_entries, mode(&pkgname[..]), pkgname);
         let mut publishers: Vec<_> = publishers
             .iter()
             .enumerate()


Test patch:
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_formatting.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_formatting.snap
index 3935fe8..354be2d 100644
--- a/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_formatting.snap
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__invalid_formatting.snap
@@ -27,7 +27,7 @@ Error:   × A file in the store is not correctly formatted:
   │  version = "1.0.0"
   │  criteria = "safe-to-deploy"
   │ 
-  help: run `cargo vet fmt` to reformat files in the store
+  help: run `cargo vet` without --locked to reformat files in the store
 Error:   × A file in the store is not correctly formatted:
   │ 
   │ --- old/audits.toml
@@ -46,5 +46,5 @@ Error:   × A file in the store is not correctly formatted:
   │ -notes = "valid field"
   │ +version = "2.0.0"
   │ 
-  help: run `cargo vet fmt` to reformat files in the store
+  help: run `cargo vet` without --locked to reformat files in the store
 
diff --git a/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_config.snap b/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_config.snap
index 944e04e..5fbcf3b 100644
--- a/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_config.snap
+++ b/src/tests/snapshots/cargo_vet__tests__store_parsing__unknown_field_config.snap
@@ -19,5 +19,5 @@ Error:   × A file in the store is not correctly formatted:
   │  criteria = "safe-to-deploy"
   │ -unknown-field = "hi"
   │ 
-  help: run `cargo vet fmt` to reformat files in the store
+  help: run `cargo vet` without --locked to reformat files in the store
 
diff --git a/src/tests/snapshots/cargo_vet__tests__unpublished__audit_as_crates_io_unpublished_wildcard_nounpublished_prefer_exemptions.snap b/src/tests/snapshots/cargo_vet__tests__unpublished__audit_as_crates_io_unpublished_wildcard_nounpublished_prefer_exemptions.snap
index 145db01..d5ad564 100644
--- a/src/tests/snapshots/cargo_vet__tests__unpublished__audit_as_crates_io_unpublished_wildcard_nounpublished_prefer_exemptions.snap
+++ b/src/tests/snapshots/cargo_vet__tests__unpublished__audit_as_crates_io_unpublished_wildcard_nounpublished_prefer_exemptions.snap
@@ -13,17 +13,11 @@ imports.lock:
 +audited_as = "9.0.0"
 +
  [[publisher.descriptive]]
- version = "8.0.0"
+-version = "8.0.0"
++version = "9.0.0"
  when = "2022-12-15"
  user-id = 1
  user-login = "user1"
  user-name = "User One"
-+
-+[[publisher.descriptive]]
-+version = "9.0.0"
-+when = "2022-12-15"
-+user-id = 1
-+user-login = "user1"
-+user-name = "User One"