Speedup manifest splicing by removing workspace discovery logic (#3570)

The original implementation required users to specify all manifests
belonging to the workspace, and had a validation pass that walked the
repo to verify the list was in sync. This PR swaps that for a `cargo
metadata` call, which lists the manifests making up the workspace. We
can then call `ctx.watch` on them to invalidate the extension when they
are modified (which was previously impossible if they were not passed to
the extension as labels). This also means users only need to specify the
workspace-root Cargo.toml now :)

There is still an edge-case where a newly-added Cargo.toml won't trigger
invalidation if it's covered by one of the wildcard `members` patterns,
but that was already a pre-existing issue. In practice, we'd expect
invalidation in that case to be triggered by a Cargo.lock change, or to
be unnecessary if the crate does not add new deps to crate resolution.

Fixes #3425
Fixes #3515
Fixes #3518

Obsoletes #3387

Co-authored-by: Daniel Wagner-Hall <[email protected]>

Author: dzbarsky

crate_universe/extensions.bzl

@@ -628,6 +628,9 @@ def _generate_hub_and_spokes(
         splicing_manifest = splicing_manifest,
     )
 
+    # The workspace root when one is explicitly provided.
+    nonhermetic_root_bazel_workspace_dir = module_ctx.path(Label("@@//:MODULE.bazel")).dirname
+
     # If re-pinning is enabled, gather additional inputs for the generator
     kwargs = dict()
     if repin:
@@ -658,8 +661,10 @@ def _generate_hub_and_spokes(
             "metadata": splice_outputs.metadata,
         })
 
-    # The workspace root when one is explicitly provided.
-    nonhermetic_root_bazel_workspace_dir = module_ctx.path(Label("@@//:MODULE.bazel")).dirname
+        for path_to_track in splice_outputs.extra_paths_to_track:
+            # We can only watch paths in our workspace.
+            if path_to_track.startswith(str(nonhermetic_root_bazel_workspace_dir)):
+                module_ctx.watch(path_to_track)
 
     paths_to_track_file = tag_path.get_child("paths_to_track.json")
     warnings_output_file = tag_path.get_child("warnings_output.json")

crate_universe/private/crates_repository.bzl

@@ -76,6 +76,8 @@ def _crates_repository_impl(repository_ctx):
         repin_instructions = repository_ctx.attr.repin_instructions,
     )
 
+    nonhermetic_root_bazel_workspace_dir = repository_ctx.workspace_root
+
     # If re-pinning is enabled, gather additional inputs for the generator
     kwargs = dict()
     if repin:
@@ -92,6 +94,11 @@ def _crates_repository_impl(repository_ctx):
             repository_name = repository_ctx.name,
         )
 
+        for path_to_track in splice_outputs.extra_paths_to_track:
+            # We can only watch paths in our workspace.
+            if path_to_track.startswith(str(nonhermetic_root_bazel_workspace_dir)):
+                repository_ctx.watch(path_to_track)
+
         kwargs.update({
             "metadata": splice_outputs.metadata,
         })
@@ -109,7 +116,7 @@ def _crates_repository_impl(repository_ctx):
         lockfile_path = lockfiles.bazel,
         cargo_lockfile_path = lockfiles.cargo,
         repository_dir = repository_ctx.path("."),
-        nonhermetic_root_bazel_workspace_dir = repository_ctx.workspace_root,
+        nonhermetic_root_bazel_workspace_dir = nonhermetic_root_bazel_workspace_dir,
         paths_to_track_file = paths_to_track_file,
         warnings_output_file = warnings_output_file,
         skip_cargo_lockfile_overwrite = repository_ctx.attr.skip_cargo_lockfile_overwrite,

crate_universe/private/splicing_utils.bzl

@@ -188,7 +188,14 @@ def splice_workspace_manifest(
     if not spliced_metadata.exists:
         fail("Metadata file does not exist: " + str(spliced_metadata))
 
+    extra_paths_to_track_path = repository_ctx.path(output_dir.get_child("extra_paths_to_track"))
+    if extra_paths_to_track_path.exists:
+        extra_paths_to_track = repository_ctx.read(extra_paths_to_track_path).split("\n")
+    else:
+        extra_paths_to_track = []
+
     return struct(
         metadata = spliced_metadata,
         cargo_lock = spliced_lockfile,
+        extra_paths_to_track = extra_paths_to_track,
     )

crate_universe/private/srcs.bzl

@@ -28,7 +28,6 @@ CARGO_BAZEL_SRCS = [
     Label("//crate_universe:src/metadata/cargo_tree_rustc_wrapper.sh"),
     Label("//crate_universe:src/metadata/dependency.rs"),
     Label("//crate_universe:src/metadata/metadata_annotation.rs"),
-    Label("//crate_universe:src/metadata/workspace_discoverer.rs"),
     Label("//crate_universe:src/rendering.rs"),
     Label("//crate_universe:src/rendering/template_engine.rs"),
     Label("//crate_universe:src/rendering/templates/module_bzl.j2"),

crate_universe/src/cli/splice.rs

@@ -5,13 +5,16 @@ use std::path::PathBuf;
 use anyhow::Context;
 use camino::Utf8PathBuf;
 use clap::Parser;
+use itertools::Itertools;
 
 use crate::cli::Result;
 use crate::config::Config;
 use crate::metadata::{
     write_metadata, Cargo, CargoUpdateRequest, Generator, MetadataGenerator, TreeResolver,
 };
-use crate::splicing::{generate_lockfile, Splicer, SplicingManifest, WorkspaceMetadata};
+use crate::splicing::{
+    generate_lockfile, Splicer, SplicerKind, SplicingManifest, WorkspaceMetadata,
+};
 
 /// Command line options for the `splice` subcommand
 #[derive(Parser, Debug)]
@@ -81,13 +84,14 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {
     };
 
     // Generate a splicer for creating a Cargo workspace manifest
-    let splicer = Splicer::new(splicing_dir, splicing_manifest)?;
+    let splicer = Splicer::new(splicing_dir.clone(), splicing_manifest)?;
+    let prepared_splicer = splicer.prepare()?;
 
     let cargo = Cargo::new(opt.cargo, opt.rustc.clone());
 
     // Splice together the manifest
-    let manifest_path = splicer
-        .splice_workspace()
+    let manifest_path = prepared_splicer
+        .splice(&splicing_dir)
         .with_context(|| format!("Failed to splice workspace {}", opt.repository_name))?;
 
     // Generate a lockfile
@@ -121,8 +125,8 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {
 
     // Write metadata to the workspace for future reuse
     let (cargo_metadata, _) = Generator::new()
-        .with_cargo(cargo)
-        .with_rustc(opt.rustc)
+        .with_cargo(cargo.clone())
+        .with_rustc(opt.rustc.clone())
         .generate(manifest_path.as_path_buf())
         .context("Failed to generate cargo metadata")?;
 
@@ -147,5 +151,22 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {
     std::fs::copy(cargo_lockfile_path, output_dir.join("Cargo.lock"))
         .context("Failed to copy lockfile")?;
 
+    if let SplicerKind::Workspace { path, .. } = prepared_splicer {
+        let metadata = cargo.metadata_command_with_options(
+            path.as_std_path(),
+            vec![String::from("--no-deps")],
+        )?.exec().with_context(|| {
+                format!(
+                    "Error spawning cargo in child process to compute crate paths for workspace '{}'",
+                    path
+                )
+            })?;
+        let contents = metadata
+            .packages
+            .iter()
+            .map(|package| package.manifest_path.clone())
+            .join("\n");
+        std::fs::write(opt.output_dir.join("extra_paths_to_track"), contents)?;
+    }
     Ok(())
 }

crate_universe/src/metadata.rs

@@ -4,7 +4,6 @@ mod cargo_bin;
 mod cargo_tree_resolver;
 mod dependency;
 mod metadata_annotation;
-mod workspace_discoverer;
 
 use std::env;
 use std::fs;
@@ -21,7 +20,6 @@ pub(crate) use self::cargo_bin::*;
 pub(crate) use self::cargo_tree_resolver::*;
 pub(crate) use self::dependency::*;
 pub(crate) use self::metadata_annotation::*;
-pub(crate) use self::workspace_discoverer::*;
 
 // TODO: This should also return a set of [crate-index::IndexConfig]s for packages in metadata.packages
 /// A Trait for generating metadata (`cargo metadata` output and a lock file) from a Cargo manifest.