Auto merge of #14074 - tweag:verification-order, r=epage

Change verification order during packaging.

Once we support packaging workspaces with dependencies, dependency packages need to be built before anything is verified. In addition to a little refactoring, this commit reorders the console messages so that package metadata (archive size, etc.) is reported before verification results.

As [suggested](ecba15c#r1640182916) on #13947  this, splits out the first commit (which has a lot of test output churn) as a separate PR.

Author: bors

src/cargo/ops/cargo_package.rs

@@ -28,6 +28,7 @@ use tar::{Archive, Builder, EntryType, Header, HeaderMode};
 use tracing::debug;
 use unicase::Ascii as UncasedAscii;
 
+#[derive(Clone)]
 pub struct PackageOpts<'gctx> {
     pub gctx: &'gctx GlobalContext,
     pub list: bool,
@@ -82,48 +83,39 @@ struct GitVcsInfo {
     sha1: String,
 }
 
+/// Packages a single package in a workspace, returning the resulting tar file.
+///
+/// # Panics
+/// Panics if `opts.list` is true. In that case you probably don't want to
+/// actually build the package tarball; you should just make and print the list
+/// of files. (We don't currently provide a public API for that, but see how
+/// [`package`] does it.)
 pub fn package_one(
     ws: &Workspace<'_>,
     pkg: &Package,
     opts: &PackageOpts<'_>,
-) -> CargoResult<Option<FileLock>> {
-    let gctx = ws.gctx();
-    let mut src = PathSource::new(pkg.root(), pkg.package_id().source_id(), gctx);
-    src.update()?;
+) -> CargoResult<FileLock> {
+    assert!(!opts.list);
 
-    if opts.check_metadata {
-        check_metadata(pkg, gctx)?;
-    }
+    let ar_files = prepare_archive(ws, pkg, opts)?;
+    let tarball = create_package(ws, pkg, ar_files)?;
 
-    if !pkg.manifest().exclude().is_empty() && !pkg.manifest().include().is_empty() {
-        gctx.shell().warn(
-            "both package.include and package.exclude are specified; \
-             the exclude list will be ignored",
-        )?;
+    if opts.verify {
+        run_verify(ws, pkg, &tarball, opts)?;
     }
-    let src_files = src.list_files(pkg)?;
 
-    // Check (git) repository state, getting the current commit hash if not
-    // dirty.
-    let vcs_info = if !opts.allow_dirty {
-        // This will error if a dirty repo is found.
-        check_repo_state(pkg, &src_files, gctx)?
-    } else {
-        None
-    };
-
-    let ar_files = build_ar_list(ws, pkg, src_files, vcs_info)?;
+    Ok(tarball)
+}
 
+// Builds a tarball and places it in the output directory.
+fn create_package(
+    ws: &Workspace<'_>,
+    pkg: &Package,
+    ar_files: Vec<ArchiveFile>,
+) -> CargoResult<FileLock> {
+    let gctx = ws.gctx();
     let filecount = ar_files.len();
 
-    if opts.list {
-        for ar_file in ar_files {
-            drop_println!(gctx, "{}", ar_file.rel_str);
-        }
-
-        return Ok(None);
-    }
-
     // Check that the package dependencies are safe to deploy.
     for dep in pkg.dependencies() {
         super::check_dep_has_version(dep, false)?;
@@ -145,10 +137,6 @@ pub fn package_one(
     dst.file().set_len(0)?;
     let uncompressed_size = tar(ws, pkg, ar_files, dst.file(), &filename)
         .with_context(|| "failed to prepare local package for uploading")?;
-    if opts.verify {
-        dst.seek(SeekFrom::Start(0))?;
-        run_verify(ws, pkg, &dst, opts).with_context(|| "failed to verify package tarball")?
-    }
 
     dst.seek(SeekFrom::Start(0))?;
     let src_path = dst.path();
@@ -172,7 +160,7 @@ pub fn package_one(
     // It doesn't really matter if this fails.
     drop(gctx.shell().status("Packaged", message));
 
-    return Ok(Some(dst));
+    return Ok(dst);
 }
 
 pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult<Option<Vec<FileLock>>> {
@@ -185,7 +173,6 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult<Option
         }
     }
     let pkgs = ws.members_with_features(specs, &opts.cli_features)?;
-
     let mut dsts = Vec::with_capacity(pkgs.len());
 
     if ws.root().join("Cargo.lock").exists() {
@@ -197,25 +184,24 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult<Option
     }
 
     for (pkg, cli_features) in pkgs {
-        let result = package_one(
-            ws,
-            pkg,
-            &PackageOpts {
-                gctx: opts.gctx,
-                list: opts.list,
-                check_metadata: opts.check_metadata,
-                allow_dirty: opts.allow_dirty,
-                verify: opts.verify,
-                jobs: opts.jobs.clone(),
-                keep_going: opts.keep_going,
-                to_package: ops::Packages::Default,
-                targets: opts.targets.clone(),
-                cli_features: cli_features,
-            },
-        )?;
+        let opts = PackageOpts {
+            to_package: ops::Packages::Default,
+            cli_features,
+            ..opts.clone()
+        };
+        let ar_files = prepare_archive(ws, pkg, &opts)?;
 
-        if !opts.list {
-            dsts.push(result.unwrap());
+        if opts.list {
+            for ar_file in ar_files {
+                drop_println!(ws.gctx(), "{}", ar_file.rel_str);
+            }
+        } else {
+            let tarball = create_package(ws, pkg, ar_files)?;
+            if opts.verify {
+                run_verify(ws, pkg, &tarball, &opts)
+                    .with_context(|| "failed to verify package tarball")?;
+            }
+            dsts.push(tarball);
         }
     }
 
@@ -227,6 +213,40 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult<Option
     }
 }
 
+/// Performs pre-archiving checks and builds a list of files to archive.
+fn prepare_archive(
+    ws: &Workspace<'_>,
+    pkg: &Package,
+    opts: &PackageOpts<'_>,
+) -> CargoResult<Vec<ArchiveFile>> {
+    let gctx = ws.gctx();
+    let mut src = PathSource::new(pkg.root(), pkg.package_id().source_id(), gctx);
+    src.update()?;
+
+    if opts.check_metadata {
+        check_metadata(pkg, gctx)?;
+    }
+
+    if !pkg.manifest().exclude().is_empty() && !pkg.manifest().include().is_empty() {
+        gctx.shell().warn(
+            "both package.include and package.exclude are specified; \
+             the exclude list will be ignored",
+        )?;
+    }
+    let src_files = src.list_files(pkg)?;
+
+    // Check (git) repository state, getting the current commit hash if not
+    // dirty.
+    let vcs_info = if !opts.allow_dirty {
+        // This will error if a dirty repo is found.
+        check_repo_state(pkg, &src_files, gctx)?
+    } else {
+        None
+    };
+
+    build_ar_list(ws, pkg, src_files, vcs_info)
+}
+
 /// Builds list of files to archive.
 fn build_ar_list(
     ws: &Workspace<'_>,
@@ -236,7 +256,6 @@ fn build_ar_list(
 ) -> CargoResult<Vec<ArchiveFile>> {
     let mut result = HashMap::new();
     let root = pkg.root();
-
     for src_file in &src_files {
         let rel_path = src_file.strip_prefix(&root)?;
         check_filename(rel_path, &mut ws.gctx().shell())?;

src/cargo/ops/registry/publish.rs

@@ -156,8 +156,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
             keep_going: opts.keep_going,
             cli_features,
         },
-    )?
-    .unwrap();
+    )?;
 
     if !opts.dry_run {
         let hash = cargo_util::Sha256::new()

tests/testsuite/alt_registry.rs

@@ -346,13 +346,13 @@ fn publish_with_registry_dependency() {
 See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
 [PACKAGING] foo v0.0.1 ([ROOT]/foo)
 [UPDATING] `alternative` index
+[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
 [VERIFYING] foo v0.0.1 ([ROOT]/foo)
 [DOWNLOADING] crates ...
 [DOWNLOADED] bar v0.0.1 (registry `alternative`)
 [COMPILING] bar v0.0.1 (registry `alternative`)
 [COMPILING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1)
 [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
-[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
 [UPLOADING] foo v0.0.1 ([ROOT]/foo)
 [UPLOADED] foo v0.0.1 to registry `alternative`
 [NOTE] waiting for `foo v0.0.1` to be available at registry `alternative`.
@@ -512,10 +512,10 @@ fn publish_to_alt_registry() {
 [WARNING] manifest has no description, license, license-file, documentation, homepage or repository.
 See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
 [PACKAGING] foo v0.0.1 ([ROOT]/foo)
+[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
 [VERIFYING] foo v0.0.1 ([ROOT]/foo)
 [COMPILING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1)
 [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
-[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
 [UPLOADING] foo v0.0.1 ([ROOT]/foo)
 [UPLOADED] foo v0.0.1 to registry `alternative`
 [NOTE] waiting for `foo v0.0.1` to be available at registry `alternative`.
@@ -591,13 +591,13 @@ fn publish_with_crates_io_dep() {
 See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
 [PACKAGING] foo v0.0.1 ([ROOT]/foo)
 [UPDATING] `dummy-registry` index
+[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
 [VERIFYING] foo v0.0.1 ([ROOT]/foo)
 [DOWNLOADING] crates ...
 [DOWNLOADED] bar v0.0.1 (registry `dummy-registry`)
 [COMPILING] bar v0.0.1
 [COMPILING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1)
 [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
-[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
 [UPLOADING] foo v0.0.1 ([ROOT]/foo)
 [UPLOADED] foo v0.0.1 to registry `alternative`
 [NOTE] waiting for `foo v0.0.1` to be available at registry `alternative`.

tests/testsuite/cargo_features.rs

@@ -664,10 +664,10 @@ fn publish_allowed() {
 [WARNING] [..]
 [..]
 [PACKAGING] a v0.0.1 [..]
+[PACKAGED] [..]
 [VERIFYING] a v0.0.1 [..]
 [COMPILING] a v0.0.1 [..]
 [FINISHED] [..]
-[PACKAGED] [..]
 [UPLOADING] a v0.0.1 [..]
 [UPLOADED] a v0.0.1 to registry `crates-io`
 [NOTE] waiting for `a v0.0.1` to be available at registry `crates-io`.

tests/testsuite/cross_publish.rs

@@ -46,10 +46,10 @@ fn simple_cross_package() {
         .with_stderr(
             "\
 [PACKAGING] foo v0.0.0 ([CWD])
+[PACKAGED] 4 files, [..] ([..] compressed)
 [VERIFYING] foo v0.0.0 ([CWD])
 [COMPILING] foo v0.0.0 ([CWD]/target/package/foo-0.0.0)
 [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
-[PACKAGED] 4 files, [..] ([..] compressed)
 ",
         )
         .run();
@@ -111,10 +111,10 @@ fn publish_with_target() {
             "\
 [UPDATING] crates.io index
 [PACKAGING] foo v0.0.0 ([CWD])
+[PACKAGED] [..]
 [VERIFYING] foo v0.0.0 ([CWD])
 [COMPILING] foo v0.0.0 ([CWD]/target/package/foo-0.0.0)
 [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
-[PACKAGED] [..]
 [UPLOADING] foo v0.0.0 ([CWD])
 [UPLOADED] foo v0.0.0 to registry `crates-io`
 [NOTE] waiting [..]

tests/testsuite/features_namespaced.rs

@@ -1055,11 +1055,11 @@ fn publish() {
             "\
 [UPDATING] [..]
 [PACKAGING] foo v0.1.0 [..]
+[PACKAGED] [..]
 [VERIFYING] foo v0.1.0 [..]
 [UPDATING] [..]
 [COMPILING] foo v0.1.0 [..]
 [FINISHED] [..]
-[PACKAGED] [..]
 [UPLOADING] foo v0.1.0 [..]
 [UPLOADED] foo v0.1.0 [..]
 [NOTE] waiting [..]

tests/testsuite/inheritable_workspace_fields.rs

@@ -164,10 +164,10 @@ fn inherit_own_workspace_fields() {
 [UPDATING] [..]
 [WARNING] [..]
 [..]
+[PACKAGED] [..]
 [VERIFYING] foo v1.2.3 [..]
 [COMPILING] foo v1.2.3 [..]
 [FINISHED] [..]
-[PACKAGED] [..]
 [UPLOADING] foo v1.2.3 [..]
 [UPLOADED] foo v1.2.3 to registry `crates-io`
 [NOTE] waiting for `foo v1.2.3` to be available at registry `crates-io`.
@@ -319,11 +319,11 @@ fn inherit_own_dependencies() {
 [..]
 [PACKAGING] bar v0.2.0 [..]
 [UPDATING] [..]
+[PACKAGED] [..]
 [VERIFYING] bar v0.2.0 [..]
 [COMPILING] dep v0.1.2
 [COMPILING] bar v0.2.0 [..]
 [FINISHED] [..]
-[PACKAGED] [..]
 [UPLOADING] bar v0.2.0 [..]
 [UPLOADED] bar v0.2.0 to registry `crates-io`
 [NOTE] waiting for `bar v0.2.0` to be available at registry `crates-io`.
@@ -478,11 +478,11 @@ fn inherit_own_detailed_dependencies() {
 [..]
 [PACKAGING] bar v0.2.0 [..]
 [UPDATING] [..]
+[PACKAGED] [..]
 [VERIFYING] bar v0.2.0 [..]
 [COMPILING] dep v0.1.2
 [COMPILING] bar v0.2.0 [..]
 [FINISHED] [..]
-[PACKAGED] [..]
 [UPLOADING] bar v0.2.0 [..]
 [UPLOADED] bar v0.2.0 to registry `crates-io`
 [NOTE] waiting for `bar v0.2.0` to be available at registry `crates-io`.
@@ -726,14 +726,14 @@ fn inherit_workspace_fields() {
 [UPDATING] [..]
 [WARNING] [..]
 [..]
+[PACKAGED] [..]
 [VERIFYING] bar v1.2.3 [..]
 [WARNING] [..]
 [..]
 [..]
 [..]
 [COMPILING] bar v1.2.3 [..]
 [FINISHED] [..]
-[PACKAGED] [..]
 [UPLOADING] bar v1.2.3 [..]
 [UPLOADED] bar v1.2.3 to registry `crates-io`
 [NOTE] waiting for `bar v1.2.3` to be available at registry `crates-io`.
@@ -892,11 +892,11 @@ fn inherit_dependencies() {
 [..]
 [PACKAGING] bar v0.2.0 [..]
 [UPDATING] [..]
+[PACKAGED] [..]
 [VERIFYING] bar v0.2.0 [..]
 [COMPILING] dep v0.1.2
 [COMPILING] bar v0.2.0 [..]
 [FINISHED] [..]
-[PACKAGED] [..]
 [UPLOADING] bar v0.2.0 [..]
 [UPLOADED] bar v0.2.0 to registry `crates-io`
 [NOTE] waiting for `bar v0.2.0` to be available at registry `crates-io`.