diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a66be51e75..cbe706bb70 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -1103,6 +1103,16 @@ jobs: steps: - name: Checkout code uses: actions/checkout@v4 + with: + # actions/checkout fetches only one commit by default; a depth of 0 removes + # that limit and fetches the full history. The `check-affected` + # cargo-make task needs that history to find HEAD's common ancestor with + # origin/main. + fetch-depth: 0 + + # The container can see the mounted workspace as owned by a different UID. + - name: Configure Git safe directory + run: git config --global --add safe.directory "$GITHUB_WORKSPACE" # PostgreSQL is required because sqlx proc macros (sqlx::query!) connect to # a local database at compile time to validate SQL queries and return types. @@ -1114,11 +1124,13 @@ jobs: sudo -u postgres psql -c "ALTER USER root WITH SUPERUSER;" createdb root - - name: Run clippy - run: cargo make --no-workspace clippy-flow - - - name: Run carbide lints - run: cargo make carbide-lints + # `check-affected` selects packages and replaces separate CI steps for + # Clippy, carbide-lints, and isolated default-feature builds. Formatting, + # workspace dependency, license, and ban checks remain global below. + - name: Run affected Rust checks + env: + AFFECTED_BASE: origin/main + run: cargo make --no-workspace check-affected - name: Check TOML formatting run: taplo fmt --check || echo "Please format toml files" @@ -1135,9 +1147,6 @@ jobs: - name: Check bans run: cargo make --no-workspace check-bans - - name: Check isolated package builds - run: cargo xtask check-isolated-package-builds - - name: Check repository is clean run: bash scripts/check-repo-clean.sh "Core pre-build checks" diff --git a/AGENTS.md b/AGENTS.md index 6d4872d182..387cf33403 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -124,14 +124,27 @@ cargo make format-nightly # Also sort imports > `carbide-lints`. The stable toolchain pinned in `rust-toolchain.toml` is used > for everything else. -### Top-level Makefile (rest-api entrypoint) +#### Affected-package CI + +When reviewing changes that add, remove, rename, or repurpose shared Rust build +inputs, verify that `is_global_path()` in +`crates/xtask/src/affected_packages.rs` remains up to date. Currently matched +shared inputs include the root Cargo files and `Cargo.lock`, cargo-make files, +Rust toolchain configuration, `.cargo/`, CI configuration, custom lint and xtask +code, and `include/`. Add any newly introduced or repurposed shared generated or +configuration directories to the predicate. If a changed path cannot be mapped +safely to exactly one workspace package, affected-package selection must fall +back to the full workspace. + +### Top-level Makefile entrypoints A top-level [`Makefile`](Makefile) at the repo root provides a thin -discoverable entrypoint for the `rest-api/` Go services. It just -delegates to `rest-api/Makefile`. +discoverable entrypoint for selected Core workflows and the `rest-api/` Go +services. ```bash -make help # default goal: list rest-* targets +make help # default goal: list available targets +make core/check-affected # check affected Rust packages and workspace dependents make rest-build # build rest-api Go binaries make rest-test # run rest-api unit tests make rest-lint # lint rest-api @@ -142,8 +155,8 @@ make rest-kind-reset # spin up the local kind dev cluster (~10 min) make rest-api/ # pass any target through to rest-api/Makefile ``` -Core (Rust) tasks are not in this Makefile; use cargo and `cargo make` -directly as documented above. +Other Core (Rust) tasks use cargo and `cargo make` directly as documented +above. ## Coding Conventions diff --git a/Makefile b/Makefile index 13453e83b0..f544807cde 100644 --- a/Makefile +++ b/Makefile @@ -14,11 +14,11 @@ # See the License for the specific language governing permissions and # limitations under the License. # -# Top-level Makefile for the rest-api/ Go services. +# Top-level Makefile for Core and rest-api workflows. # -# Thin discoverable entrypoint that delegates to rest-api/Makefile. -# rest-api/Makefile continues to work directly; this file is an -# additive convenience layer. +# Thin discoverable entrypoint that delegates Core tasks to cargo-make and Rest +# tasks to rest-api/Makefile. Both underlying entrypoints continue to work +# directly; this file is an additive convenience layer. # # Run `make help` (default goal) for the inventory of targets. @@ -26,6 +26,8 @@ SHELL := /bin/bash .DEFAULT_GOAL := help +AFFECTED_BASE ?= origin/main + # ============================================================================= # Help (default goal) # ============================================================================= @@ -38,12 +40,23 @@ help: ## Show this help and exit (default goal) @echo "Container images (build from a clean clone):" @grep -E '^images[a-zA-Z0-9_-]*:.*## ' $(MAKEFILE_LIST) | awk 'BEGIN{FS=":.*?## "} {printf " %-26s %s\n", $$1, $$2}' @echo "" + @echo "Core (Rust):" + @grep -E '^core/check-affected:.*## ' $(MAKEFILE_LIST) | awk 'BEGIN{FS=":.*?## "} {printf " %-26s %s\n", $$1, $$2}' + @echo "" @echo "Rest (Go services in rest-api/):" @grep -E '^rest-[a-zA-Z0-9_-]+:.*## ' $(MAKEFILE_LIST) | awk 'BEGIN{FS=":.*?## "} {printf " %-26s %s\n", $$1, $$2}' @echo " rest-api/ Pass any target through to rest-api/Makefile" @echo "" @echo " cat rest-api/Makefile See all rest-api/ targets directly" +# ============================================================================= +# Core (Rust; delegate to cargo-make) +# ============================================================================= + +.PHONY: core/check-affected +core/check-affected: ## Run Clippy, custom lints, and isolated builds for affected Rust packages + AFFECTED_BASE="$(AFFECTED_BASE)" cargo make --no-workspace check-affected + # ============================================================================= # Getting started (build host setup) # ============================================================================= diff --git a/Makefile.toml b/Makefile.toml index 57472c941d..914a2f5bd2 100644 --- a/Makefile.toml +++ b/Makefile.toml @@ -828,6 +828,50 @@ workspace = false description = "Check that workspace packages build independently with default features" script = ["cargo xtask check-isolated-package-builds"] +[tasks.check-affected] +workspace = false +description = "Run compilation checks for changed Rust packages and their workspace dependents" +script = ''' +set -eu + +base="${AFFECTED_BASE:-origin/main}" +affected_packages="$(cargo xtask affected-packages --base "${base}")" + +if [ -z "${affected_packages}" ]; then + echo "No affected Rust packages were selected" >&2 + exit 1 +fi + +echo "Affected Rust packages:" +printf ' %s\n' "${affected_packages}" + +set -- +while IFS= read -r package; do + [ -n "${package}" ] || continue + set -- "$@" -p "${package}" +done <), +} + +#[derive(Debug, PartialEq, Eq)] +enum FullWorkspaceReason { + GlobalPath(String), + UnsafePath(String), + UnmappedPath(String), + EmptyDiff, + MalformedDiff, + IncompleteDependencyGraph, +} + +impl fmt::Display for FullWorkspaceReason { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::GlobalPath(path) => write!(formatter, "global path changed: {path}"), + Self::UnsafePath(path) => { + write!( + formatter, + "changed path is not a safe relative path: {path}" + ) + } + Self::UnmappedPath(path) => write!( + formatter, + "changed path does not map to a workspace package: {path}" + ), + Self::EmptyDiff => formatter.write_str("no changed paths found"), + Self::MalformedDiff => { + formatter.write_str("Git diff output was malformed or non-UTF-8") + } + Self::IncompleteDependencyGraph => { + formatter.write_str("Cargo dependency graph was incomplete") + } + } + } +} + +#[derive(Debug, PartialEq, Eq)] +enum ChangedPaths { + Paths(Vec), + MalformedDiff, +} + +pub fn run(base: &str) -> eyre::Result<()> { + let metadata = workspace_metadata()?; + let workspace_packages = workspace_packages(&metadata)?; + let all_package_selectors = all_package_selectors(&workspace_packages); + let changed_paths = changed_paths(&metadata.workspace_root, base)?; + + let selection = match changed_paths { + ChangedPaths::MalformedDiff => Selection::All(FullWorkspaceReason::MalformedDiff), + ChangedPaths::Paths(changed_paths) => reverse_dependencies(&metadata, &workspace_packages) + .map(|reverse_dependencies| { + select_affected(&changed_paths, &workspace_packages, &reverse_dependencies) + }) + .unwrap_or(Selection::All( + FullWorkspaceReason::IncompleteDependencyGraph, + )), + }; + + let package_selectors = match selection { + Selection::All(reason) => { + eprintln!("affected-packages: selecting full workspace: {reason}"); + all_package_selectors + } + Selection::Packages(package_selectors) => package_selectors, + }; + + for package_selector in package_selectors { + println!("{package_selector}"); + } + + Ok(()) +} + +fn workspace_metadata() -> eyre::Result { + let metadata = MetadataCommand::new() + .features(CargoOpt::AllFeatures) + .other_options(vec!["--locked".to_string()]) + .exec() + .context("failed to run cargo metadata")?; + Ok(metadata) +} + +fn workspace_packages(metadata: &Metadata) -> eyre::Result> { + metadata + .workspace_members + .iter() + .map(|member| { + let package = metadata + .packages + .iter() + .find(|package| &package.id == member) + .with_context(|| { + format!("workspace member {member} missing from cargo metadata") + })?; + let manifest_dir = package + .manifest_path + .parent() + .context("workspace package manifest has no parent directory")?; + let root = manifest_dir + .strip_prefix(&metadata.workspace_root) + .with_context(|| { + format!( + "package {} is outside workspace root {}", + package.name, + metadata.workspace_root.display() + ) + })? + .to_path_buf(); + + Ok(WorkspacePackage { + id: member.clone(), + cargo_selector: format!("{}@{}", package.name, package.version), + root, + }) + }) + .collect() +} + +fn reverse_dependencies( + metadata: &Metadata, + workspace_packages: &[WorkspacePackage], +) -> Option>> { + let resolve = metadata.resolve.as_ref()?; + let workspace_ids = workspace_packages + .iter() + .map(|package| package.id.clone()) + .collect::>(); + let mut reverse_dependencies = workspace_ids + .iter() + .cloned() + .map(|id| (id, BTreeSet::new())) + .collect::>(); + + for node in &resolve.nodes { + if !workspace_ids.contains(&node.id) { + continue; + } + + for dependency in &node.dependencies { + if workspace_ids.contains(dependency) { + reverse_dependencies + .get_mut(dependency)? + .insert(node.id.clone()); + } + } + } + + // A missing workspace node means the graph is incomplete. Fall back to all. + if workspace_ids + .iter() + .any(|id| !resolve.nodes.iter().any(|node| &node.id == id)) + { + return None; + } + + Some(reverse_dependencies) +} + +fn select_affected( + changed_paths: &[String], + workspace_packages: &[WorkspacePackage], + reverse_dependencies: &HashMap>, +) -> Selection { + if changed_paths.is_empty() { + return Selection::All(FullWorkspaceReason::EmptyDiff); + } + if let Some(path) = changed_paths.iter().find(|path| is_global_path(path)) { + return Selection::All(FullWorkspaceReason::GlobalPath(path.clone())); + } + + let mut affected = BTreeSet::new(); + for changed_path in changed_paths { + let Some(path) = safe_relative_path(changed_path) else { + return Selection::All(FullWorkspaceReason::UnsafePath(changed_path.clone())); + }; + let Some(package) = owning_package(path, workspace_packages) else { + return Selection::All(FullWorkspaceReason::UnmappedPath(changed_path.clone())); + }; + affected.insert(package.id.clone()); + } + + let mut queue = affected.iter().cloned().collect::>(); + while let Some(package) = queue.pop_front() { + let Some(dependents) = reverse_dependencies.get(&package) else { + return Selection::All(FullWorkspaceReason::IncompleteDependencyGraph); + }; + for dependent in dependents { + if affected.insert(dependent.clone()) { + queue.push_back(dependent.clone()); + } + } + } + + let selectors_by_id = workspace_packages + .iter() + .map(|package| (&package.id, &package.cargo_selector)) + .collect::>(); + let package_selectors = affected + .iter() + .map(|id| selectors_by_id.get(id).map(|selector| (*selector).clone())) + .collect::>>(); + + package_selectors + .map(Selection::Packages) + .unwrap_or(Selection::All( + FullWorkspaceReason::IncompleteDependencyGraph, + )) +} + +fn owning_package<'a>( + path: &Path, + workspace_packages: &'a [WorkspacePackage], +) -> Option<&'a WorkspacePackage> { + workspace_packages + .iter() + .filter(|package| path.starts_with(&package.root)) + .max_by_key(|package| package.root.components().count()) +} + +fn safe_relative_path(path: &str) -> Option<&Path> { + let path = Path::new(path); + if path + .components() + .all(|component| matches!(component, Component::Normal(_))) + { + Some(path) + } else { + None + } +} + +fn is_global_path(path: &str) -> bool { + const GLOBAL_FILES: &[&str] = &[ + "Cargo.toml", + "Cargo.lock", + "Makefile", + "Makefile.toml", + "Makefile-build.toml", + "Makefile-package.toml", + "rust-toolchain", + "rust-toolchain.toml", + ]; + const GLOBAL_DIRECTORIES: &[&str] = &[ + ".cargo/", + ".github/", + ".gitlab/", + "crates/xtask/", + "include/", + "lints/", + ]; + + GLOBAL_FILES.contains(&path) + || path.starts_with(".gitlab-ci") + || GLOBAL_DIRECTORIES + .iter() + .any(|directory| path.starts_with(directory)) +} + +fn all_package_selectors(workspace_packages: &[WorkspacePackage]) -> BTreeSet { + workspace_packages + .iter() + .map(|package| package.cargo_selector.clone()) + .collect() +} + +fn changed_paths(workspace_root: &Path, base: &str) -> eyre::Result { + let merge_base = git_output(workspace_root, ["merge-base", base, "HEAD"])?; + let merge_base = String::from_utf8(merge_base.stdout) + .context("git merge-base returned a non-UTF-8 revision")?; + let merge_base = merge_base.trim(); + if merge_base.is_empty() { + bail!("git merge-base returned an empty revision"); + } + + let diff = git_output( + workspace_root, + [ + "diff", + "--name-status", + "-z", + "--find-renames", + merge_base, + "HEAD", + "--", + ], + )?; + + // Malformed or non-UTF-8 Git output is uncertain, so select every package. + Ok(parsed_changed_paths(&diff.stdout)) +} + +fn parsed_changed_paths(output: &[u8]) -> ChangedPaths { + parse_name_status(output) + .map(ChangedPaths::Paths) + .unwrap_or(ChangedPaths::MalformedDiff) +} + +fn git_output(workspace_root: &Path, args: [&str; N]) -> eyre::Result { + let output = Command::new("git") + .args(args) + .current_dir(workspace_root) + .output() + .context("failed to run git")?; + if !output.status.success() { + bail!( + "git command failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + Ok(output) +} + +fn parse_name_status(output: &[u8]) -> Option> { + if output.is_empty() { + return Some(Vec::new()); + } + + let fields = output + .strip_suffix(&[0])? + .split(|byte| *byte == 0) + .collect::>(); + let mut paths = Vec::new(); + let mut index = 0; + + while index < fields.len() { + let status = std::str::from_utf8(fields[index]).ok()?; + index += 1; + if status.is_empty() || index >= fields.len() { + return None; + } + + let path_count = if status.starts_with('R') || status.starts_with('C') { + 2 + } else { + 1 + }; + if index + path_count > fields.len() { + return None; + } + + for field in &fields[index..index + path_count] { + let path = std::str::from_utf8(field).ok()?; + if path.is_empty() { + return None; + } + paths.push(path.to_string()); + } + index += path_count; + } + + Some(paths) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn real_workspace_metadata_enables_every_workspace_feature() { + // This is intentionally integration-style: synthetic dependency graphs + // cannot prove that `CargoOpt::AllFeatures` reaches Cargo and activates + // optional workspace dependency edges in the production metadata call. + let metadata = workspace_metadata().unwrap(); + let resolve = metadata.resolve.as_ref().unwrap(); + + for member in &metadata.workspace_members { + let package = metadata + .packages + .iter() + .find(|package| &package.id == member) + .unwrap(); + let node = resolve + .nodes + .iter() + .find(|node| &node.id == member) + .unwrap(); + let missing_features = package + .features + .keys() + .filter(|feature| !node.features.contains(feature)) + .collect::>(); + + assert!( + missing_features.is_empty(), + "package {} is missing resolved features: {missing_features:?}", + package.name + ); + } + } + + fn package(id: &str, name: &str, root: &str) -> WorkspacePackage { + WorkspacePackage { + id: PackageId { + repr: id.to_string(), + }, + cargo_selector: format!("{name}@0.1.0"), + root: PathBuf::from(root), + } + } + + fn fixture() -> ( + Vec, + HashMap>, + ) { + let packages = vec![ + package("a", "alpha", "crates/alpha"), + package("b", "beta", "crates/beta"), + package("c", "gamma", "crates/gamma"), + package("nested", "nested-tests", "crates/alpha/tests"), + ]; + let reverse_dependencies = HashMap::from([ + ( + packages[0].id.clone(), + BTreeSet::from([packages[1].id.clone()]), + ), + ( + packages[1].id.clone(), + BTreeSet::from([packages[2].id.clone()]), + ), + (packages[2].id.clone(), BTreeSet::new()), + (packages[3].id.clone(), BTreeSet::new()), + ]); + (packages, reverse_dependencies) + } + + fn selected_names(selection: Selection) -> Vec { + match selection { + Selection::Packages(packages) => packages.into_iter().collect(), + Selection::All(reason) => panic!("expected a package selection, got: {reason}"), + } + } + + #[test] + fn maps_paths_to_the_deepest_package_and_adds_transitive_dependents() { + let (packages, reverse_dependencies) = fixture(); + + assert_eq!( + selected_names(select_affected( + &["crates/alpha/src/lib.rs".to_string()], + &packages, + &reverse_dependencies, + )), + ["alpha@0.1.0", "beta@0.1.0", "gamma@0.1.0"] + ); + assert_eq!( + selected_names(select_affected( + &["crates/alpha/tests/src/lib.rs".to_string()], + &packages, + &reverse_dependencies, + )), + ["nested-tests@0.1.0"] + ); + } + + #[test] + fn package_manifest_changes_select_the_package() { + let (packages, reverse_dependencies) = fixture(); + + assert_eq!( + selected_names(select_affected( + &["crates/beta/Cargo.toml".to_string()], + &packages, + &reverse_dependencies, + )), + ["beta@0.1.0", "gamma@0.1.0"] + ); + } + + #[test] + fn renamed_paths_select_both_owners() { + let (packages, reverse_dependencies) = fixture(); + + assert_eq!( + selected_names(select_affected( + &[ + "crates/alpha/src/moved.rs".to_string(), + "crates/beta/src/moved.rs".to_string(), + ], + &packages, + &reverse_dependencies, + )), + ["alpha@0.1.0", "beta@0.1.0", "gamma@0.1.0"] + ); + } + + #[test] + fn global_unmapped_unsafe_and_empty_changes_select_all_with_a_reason() { + let (packages, reverse_dependencies) = fixture(); + let paths = [ + ( + "Cargo.toml", + FullWorkspaceReason::GlobalPath("Cargo.toml".to_string()), + ), + ( + "Cargo.lock", + FullWorkspaceReason::GlobalPath("Cargo.lock".to_string()), + ), + ( + "rust-toolchain.toml", + FullWorkspaceReason::GlobalPath("rust-toolchain.toml".to_string()), + ), + ( + ".cargo/config.toml", + FullWorkspaceReason::GlobalPath(".cargo/config.toml".to_string()), + ), + ( + "lints/carbide-lints/src/lib.rs", + FullWorkspaceReason::GlobalPath("lints/carbide-lints/src/lib.rs".to_string()), + ), + ( + "crates/xtask/src/main.rs", + FullWorkspaceReason::GlobalPath("crates/xtask/src/main.rs".to_string()), + ), + ( + ".github/workflows/ci.yaml", + FullWorkspaceReason::GlobalPath(".github/workflows/ci.yaml".to_string()), + ), + ( + "Makefile.toml", + FullWorkspaceReason::GlobalPath("Makefile.toml".to_string()), + ), + ( + "docs/readme.md", + FullWorkspaceReason::UnmappedPath("docs/readme.md".to_string()), + ), + ( + "crates/deleted-package/Cargo.toml", + FullWorkspaceReason::UnmappedPath("crates/deleted-package/Cargo.toml".to_string()), + ), + ( + "../Cargo.toml", + FullWorkspaceReason::UnsafePath("../Cargo.toml".to_string()), + ), + ]; + + for (path, reason) in paths { + assert_eq!( + select_affected(&[path.to_string()], &packages, &reverse_dependencies), + Selection::All(reason), + "path {path} should select the full workspace" + ); + } + assert_eq!( + select_affected(&[], &packages, &reverse_dependencies), + Selection::All(FullWorkspaceReason::EmptyDiff) + ); + } + + #[test] + fn incomplete_dependency_graph_selects_all_with_a_reason() { + let (packages, mut reverse_dependencies) = fixture(); + reverse_dependencies.remove(&packages[0].id); + + assert_eq!( + select_affected( + &["crates/alpha/src/lib.rs".to_string()], + &packages, + &reverse_dependencies, + ), + Selection::All(FullWorkspaceReason::IncompleteDependencyGraph) + ); + } + + #[test] + fn deleted_file_in_an_existing_package_selects_its_owner() { + let (packages, reverse_dependencies) = fixture(); + + assert_eq!( + selected_names(select_affected( + &["crates/beta/src/deleted.rs".to_string()], + &packages, + &reverse_dependencies, + )), + ["beta@0.1.0", "gamma@0.1.0"] + ); + } + + #[test] + fn parses_modified_deleted_and_renamed_git_records() { + let output = b"M\0crates/alpha/src/lib.rs\0D\0crates/beta/src/old.rs\0R100\0crates/alpha/src/from.rs\0crates/gamma/src/to.rs\0"; + + assert_eq!( + parse_name_status(output).unwrap(), + [ + "crates/alpha/src/lib.rs", + "crates/beta/src/old.rs", + "crates/alpha/src/from.rs", + "crates/gamma/src/to.rs", + ] + ); + assert!(parse_name_status(b"R100\0only-one-path\0").is_none()); + assert!(parse_name_status(b"M\0missing-trailing-nul").is_none()); + } + + #[test] + fn malformed_diff_is_distinct_from_an_empty_diff() { + assert_eq!( + parsed_changed_paths(b"M\0missing-trailing-nul"), + ChangedPaths::MalformedDiff + ); + assert_eq!(parsed_changed_paths(b""), ChangedPaths::Paths(Vec::new())); + } + + #[test] + fn full_workspace_reasons_are_actionable() { + let reasons = [ + ( + FullWorkspaceReason::GlobalPath("Cargo.lock".to_string()), + "global path changed: Cargo.lock", + ), + ( + FullWorkspaceReason::UnsafePath("../Cargo.toml".to_string()), + "changed path is not a safe relative path: ../Cargo.toml", + ), + ( + FullWorkspaceReason::UnmappedPath("docs/readme.md".to_string()), + "changed path does not map to a workspace package: docs/readme.md", + ), + (FullWorkspaceReason::EmptyDiff, "no changed paths found"), + ( + FullWorkspaceReason::MalformedDiff, + "Git diff output was malformed or non-UTF-8", + ), + ( + FullWorkspaceReason::IncompleteDependencyGraph, + "Cargo dependency graph was incomplete", + ), + ]; + + for (reason, message) in reasons { + assert_eq!(reason.to_string(), message); + } + } +} diff --git a/crates/xtask/src/isolated_package_builds.rs b/crates/xtask/src/isolated_package_builds.rs index 0240592628..70aa970fd9 100644 --- a/crates/xtask/src/isolated_package_builds.rs +++ b/crates/xtask/src/isolated_package_builds.rs @@ -14,14 +14,24 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +use std::collections::BTreeSet; use std::process::Command; use cargo_metadata::MetadataCommand; use eyre::{Context, ContextCompat, bail}; -/// Checks that every workspace package builds independently with default features. -pub fn check() -> eyre::Result<()> { - let packages = workspace_packages()?; +#[derive(Debug, PartialEq, Eq)] +struct WorkspacePackage { + name: String, + cargo_selector: String, +} + +/// Checks workspace packages independently with default features. +/// +/// An empty package selection preserves the original full-workspace behavior. +pub fn check(selected_packages: &[String]) -> eyre::Result<()> { + let workspace_packages = workspace_packages()?; + let packages = select_packages(&workspace_packages, selected_packages)?; let mut failures = Vec::new(); for package in packages { @@ -50,9 +60,40 @@ pub fn check() -> eyre::Result<()> { bail!("one or more isolated package builds failed") } -fn workspace_packages() -> eyre::Result> { +fn select_packages( + workspace_packages: &[WorkspacePackage], + selected_packages: &[String], +) -> eyre::Result> { + if selected_packages.is_empty() { + return Ok(workspace_packages + .iter() + .map(|package| package.cargo_selector.clone()) + .collect()); + } + + let selected_packages = selected_packages + .iter() + .map(String::as_str) + .collect::>(); + + let mut cargo_selectors = BTreeSet::new(); + for selected_package in selected_packages { + let Some(workspace_package) = workspace_packages.iter().find(|package| { + package.name == selected_package || package.cargo_selector == selected_package + }) else { + bail!("unknown workspace package: {selected_package}"); + }; + cargo_selectors.insert(workspace_package.cargo_selector.clone()); + } + + Ok(cargo_selectors.into_iter().collect()) +} + +fn workspace_packages() -> eyre::Result> { + // Metadata runs before the checks, so prevent it from repairing a stale lockfile. let metadata = MetadataCommand::new() .no_deps() + .other_options(vec!["--locked".to_string()]) .exec() .context("failed to run cargo metadata")?; @@ -64,7 +105,10 @@ fn workspace_packages() -> eyre::Result> { .packages .iter() .find(|package| &package.id == member) - .map(|package| package.name.clone()) + .map(|package| WorkspacePackage { + name: package.name.clone(), + cargo_selector: format!("{}@{}", package.name, package.version), + }) .with_context(|| format!("workspace member {member} missing from cargo metadata")) }) .collect::>>() @@ -73,3 +117,41 @@ fn workspace_packages() -> eyre::Result> { fn cargo() -> String { std::env::var("CARGO").unwrap_or_else(|_| "cargo".to_string()) } + +#[cfg(test)] +mod tests { + use super::{WorkspacePackage, select_packages}; + + fn package(name: &str, version: &str) -> WorkspacePackage { + WorkspacePackage { + name: name.to_string(), + cargo_selector: format!("{name}@{version}"), + } + } + + #[test] + fn empty_selection_uses_every_workspace_package() { + let workspace = vec![package("beta", "2.0.0"), package("alpha", "1.0.0")]; + + assert_eq!( + select_packages(&workspace, &[]).unwrap(), + ["beta@2.0.0", "alpha@1.0.0"] + ); + } + + #[test] + fn explicit_selection_is_validated_sorted_and_deduplicated() { + let workspace = vec![package("alpha", "1.0.0"), package("beta", "2.0.0")]; + let selected = vec![ + "beta".to_string(), + "alpha@1.0.0".to_string(), + "beta".to_string(), + ]; + + assert_eq!( + select_packages(&workspace, &selected).unwrap(), + ["alpha@1.0.0", "beta@2.0.0"] + ); + assert!(select_packages(&workspace, &["missing".to_string()]).is_err()); + } +} diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs index e19972c9b4..1a7dfa6e32 100644 --- a/crates/xtask/src/main.rs +++ b/crates/xtask/src/main.rs @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +mod affected_packages; mod isolated_package_builds; mod workspace_deps; @@ -22,6 +23,11 @@ use clap::Parser; #[derive(Parser, Debug)] #[clap(name = "xtask")] enum Xtask { + #[clap( + name = "affected-packages", + about = "List changed workspace packages and their transitive workspace dependents" + )] + AffectedPackages(AffectedPackages), #[clap( name = "check-workspace-deps", about = "Check for any dependency versions defined in crate-level Cargo.toml's instead of the workspace root" @@ -31,7 +37,29 @@ enum Xtask { name = "check-isolated-package-builds", about = "Check that each workspace package builds independently with its default features" )] - IsolatedPackageBuilds, + IsolatedPackageBuilds(IsolatedPackageBuilds), +} + +#[derive(Parser, Debug)] +struct AffectedPackages { + #[clap( + long, + value_name = "REVISION", + default_value = "origin/main", + help = "Revision whose merge-base with HEAD is used to find changed paths" + )] + base: String, +} + +#[derive(Parser, Debug)] +struct IsolatedPackageBuilds { + #[clap( + short = 'p', + long = "package", + value_name = "PACKAGE", + help = "Workspace package to check (repeatable; defaults to all packages)" + )] + packages: Vec, } #[derive(Parser, Debug)] @@ -46,9 +74,37 @@ struct CheckWorkspaceDeps { fn main() -> eyre::Result<()> { match Xtask::parse() { + Xtask::AffectedPackages(AffectedPackages { base }) => affected_packages::run(&base), Xtask::CheckWorkspaceDeps(CheckWorkspaceDeps { fix }) => { workspace_deps::check(fix)?.report_and_exit() } - Xtask::IsolatedPackageBuilds => isolated_package_builds::check(), + Xtask::IsolatedPackageBuilds(IsolatedPackageBuilds { packages }) => { + isolated_package_builds::check(&packages) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn affected_packages_base(arguments: &[&str]) -> String { + let command = Xtask::try_parse_from(arguments).unwrap(); + let Xtask::AffectedPackages(AffectedPackages { base }) = command else { + panic!("expected affected-packages command"); + }; + base + } + + #[test] + fn affected_packages_base_defaults_to_origin_main_and_accepts_an_override() { + assert_eq!( + affected_packages_base(&["xtask", "affected-packages"]), + "origin/main" + ); + assert_eq!( + affected_packages_base(&["xtask", "affected-packages", "--base", "upstream/main",]), + "upstream/main" + ); } }