From c81eaf076960f877b042c3e35a1c31744d0752f3 Mon Sep 17 00:00:00 2001 From: Graeme Coupar Date: Tue, 3 Sep 2024 15:22:48 +0100 Subject: [PATCH 1/7] improve the output now i have basic GHA in place From cf6e6294ff9f35ae671818a11cd93ac8b939eec7 Mon Sep 17 00:00:00 2001 From: Graeme Coupar Date: Tue, 3 Sep 2024 15:38:59 +0100 Subject: [PATCH 2/7] iterating on this thing --- src/main.rs | 48 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/src/main.rs b/src/main.rs index 26c9f17..603a512 100644 --- a/src/main.rs +++ b/src/main.rs @@ -32,17 +32,25 @@ fn main() { let determinator_set = determinator.compute(); - // determinator_set.affected_set contains the workspace packages directly or indirectly affected - // by the change. - let cargo_test_targets = determinator_set + let cargo_package_specs = determinator_set .affected_set .packages(DependencyDirection::Forward) .filter(|package| package.has_test_targets()) + .filter(|package| { + // TODO: Make this configurable somehow... + package.name() != "grafbase-docker-tests" + }) .flat_map(|package| ["-p", package.name()]) .collect::>() .join(" "); - let cargo_build_targets = determinator_set + let changed_packages = determinator_set + .affected_set + .packages(DependencyDirection::Forward) + .map(|package| package.name().to_string()) + .collect(); + + let cargo_bin_specs = determinator_set .affected_set .root_packages(DependencyDirection::Forward) .flat_map(|package| { @@ -55,18 +63,42 @@ fn main() { .collect::>() .join(" "); + let changed_binaries = determinator_set + .affected_set + .packages(DependencyDirection::Forward) + .flat_map(|package| { + package + .binary_targets() + .into_iter() + .map(|target| target.name().to_string()) + .collect::>() + }) + .collect::>(); + let report = Report { - cargo_test_targets, - cargo_build_targets, + cargo_package_specs, + cargo_bin_specs, + changed_packages, + changed_binaries, }; println!("{}", serde_json::to_string(&report).unwrap()) } #[derive(serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "kebab-case")] pub struct Report { - cargo_test_targets: String, - cargo_build_targets: String, + /// A string that can be passed to cargo to limit its builds to changed packages + cargo_package_specs: String, + + /// A string that can be passed to cargo build to limit only to changed binaries + cargo_bin_specs: String, + + /// The full list of packages that have changed, useful for other CI filtering purposes + changed_packages: Vec, + + /// The full list of binaries that have changed, useful for other CI filtering purposes + changed_binaries: Vec, } trait PackageMetadataExt { From 02c3a8857d99d8d52a223ad657270aa6dcdb58f5 Mon Sep 17 00:00:00 2001 From: Graeme Coupar Date: Wed, 4 Sep 2024 11:14:46 +0100 Subject: [PATCH 3/7] add support for docker & non docker platforms --- src/main.rs | 47 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/src/main.rs b/src/main.rs index 603a512..266ed38 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,6 +6,9 @@ use guppy::{ CargoMetadata, }; +// TODO: Make this configurable via a file once I've finished iterating +const TESTS_THAT_NEED_DOCKER: &[&str] = &["integration-tests", "grafbase-gateway"]; + fn main() { let args = std::env::args().collect::>(); @@ -32,7 +35,31 @@ fn main() { let determinator_set = determinator.compute(); - let cargo_package_specs = determinator_set + let cargo_build_specs = determinator_set + .affected_set + .packages(DependencyDirection::Forward) + .filter(|package| { + // TODO: Make this configurable somehow... + package.name() != "grafbase-docker-tests" + }) + .flat_map(|package| ["-p", package.name()]) + .collect::>() + .join(" "); + + let cargo_test_specs = determinator_set + .affected_set + .packages(DependencyDirection::Forward) + .filter(|package| package.has_test_targets()) + .filter(|package| { + // TODO: Make this configurable somehow... + package.name() != "grafbase-docker-tests" + }) + .filter(|package| !TESTS_THAT_NEED_DOCKER.contains(&package.name())) + .flat_map(|package| ["-p", package.name()]) + .collect::>() + .join(" "); + + let cargo_docker_test_specs = determinator_set .affected_set .packages(DependencyDirection::Forward) .filter(|package| package.has_test_targets()) @@ -76,7 +103,9 @@ fn main() { .collect::>(); let report = Report { - cargo_package_specs, + cargo_build_specs, + cargo_test_specs, + cargo_docker_test_specs, cargo_bin_specs, changed_packages, changed_binaries, @@ -88,8 +117,18 @@ fn main() { #[derive(serde::Serialize, serde::Deserialize)] #[serde(rename_all = "kebab-case")] pub struct Report { - /// A string that can be passed to cargo to limit its builds to changed packages - cargo_package_specs: String, + /// A string that can be passed to cargo build to limit to changed packages + cargo_build_specs: String, + + /// A string that can be passed to cargo test to limit to changed packages + /// + /// This one should be used for platforms that do not support docker + cargo_test_specs: String, + + /// A string that can be passed to cargo test to limit to changed packages + /// + /// This one should be used for platforms that support docker + cargo_docker_test_specs: String, /// A string that can be passed to cargo build to limit only to changed binaries cargo_bin_specs: String, From 3eedbf0a4fc98039984d3c021e60d26f71b8b7f0 Mon Sep 17 00:00:00 2001 From: Graeme Coupar Date: Thu, 5 Sep 2024 13:55:32 +0100 Subject: [PATCH 4/7] tidy things up, add config support --- Cargo.lock | 59 ++++++++++++++++- Cargo.toml | 3 +- src/config.rs | 28 ++++++++ src/guppy_ext.rs | 62 +++++++++++++++++ src/main.rs | 168 ++++++++++++++++------------------------------- 5 files changed, 205 insertions(+), 115 deletions(-) create mode 100644 src/config.rs create mode 100644 src/guppy_ext.rs diff --git a/Cargo.lock b/Cargo.lock index f460b43..757a35e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -133,7 +133,7 @@ dependencies = [ "petgraph", "rayon", "serde", - "toml", + "toml 0.5.11", ] [[package]] @@ -214,7 +214,7 @@ dependencies = [ "smallvec", "static_assertions", "target-spec", - "toml", + "toml 0.5.11", ] [[package]] @@ -229,7 +229,7 @@ dependencies = [ "guppy-workspace-hack", "semver", "serde", - "toml", + "toml 0.5.11", ] [[package]] @@ -445,6 +445,15 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_spanned" +version = "0.6.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eb5b1b31579f3811bf615c144393417496f152e12ac8b7663bf664f4a815306d" +dependencies = [ + "serde", +] + [[package]] name = "smallvec" version = "1.13.2" @@ -517,6 +526,40 @@ dependencies = [ "serde", ] +[[package]] +name = "toml" +version = "0.8.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1ed1f98e3fdc28d6d910e6737ae6ab1a93bf1985935a1193e68f93eeb68d24e" +dependencies = [ + "serde", + "serde_spanned", + "toml_datetime", + "toml_edit", +] + +[[package]] +name = "toml_datetime" +version = "0.6.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0dd7358ecb8fc2f8d014bf86f6f638ce72ba252a2c3a2572f2a795f1d23efb41" +dependencies = [ + "serde", +] + +[[package]] +name = "toml_edit" +version = "0.22.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "583c44c02ad26b0c3f3066fe629275e50627026c51ac2e595cca4c230ce1ce1d" +dependencies = [ + "indexmap 2.5.0", + "serde", + "serde_spanned", + "toml_datetime", + "winnow", +] + [[package]] name = "unicode-ident" version = "1.0.12" @@ -544,6 +587,16 @@ dependencies = [ "itertools 0.13.0", "serde", "serde_json", + "toml 0.8.19", +] + +[[package]] +name = "winnow" +version = "0.6.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "68a9bda4691f099d435ad181000724da8e5899daa10713c2d432552b9ccd3a6f" +dependencies = [ + "memchr", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 18b5187..f4dfd24 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,4 +8,5 @@ determinator = "0.12" guppy = "0.17" itertools = "0.13" serde = "1" -serde_json = "1" \ No newline at end of file +serde_json = "1" +toml = "0.8" \ No newline at end of file diff --git a/src/config.rs b/src/config.rs new file mode 100644 index 0000000..8e75c08 --- /dev/null +++ b/src/config.rs @@ -0,0 +1,28 @@ +use std::fs::read_to_string; + +pub fn load() -> Config { + do_load().unwrap_or_default() +} + +fn do_load() -> Option { + let path = std::env::var("WHAT_RUST_CHANGED_CONFIG").ok()?; + + eprintln!("Loading config from {path}"); + + let data = read_to_string(path).expect("to be able to read named config file"); + + toml::from_str(&data).expect("invalid config format") +} + +#[derive(serde::Serialize, serde::Deserialize, Default)] +pub struct Config { + /// Packages that shouldn't automatically be included in test runs + pub ignore_test_packages: Vec, + + /// Packages that require docker for their tests and should be put + /// into a separate section of the output + pub docker_test_packages: Vec, + + #[serde(flatten)] + pub determinator_rules: determinator::rules::DeterminatorRules, +} diff --git a/src/guppy_ext.rs b/src/guppy_ext.rs new file mode 100644 index 0000000..f23a8e7 --- /dev/null +++ b/src/guppy_ext.rs @@ -0,0 +1,62 @@ +use guppy::graph::{BuildTarget, BuildTargetKind}; + +pub trait PackageMetadataExt { + fn has_test_targets(&self) -> bool; + fn binary_targets(&self) -> Vec>; +} + +impl PackageMetadataExt for guppy::graph::PackageMetadata<'_> { + fn has_test_targets(&self) -> bool { + let package_root_path = self + .manifest_path() + .parent() + .expect("all packages to have manifests with one parent"); + + self.build_targets() + .filter(|target| { + matches!( + target.kind(), + BuildTargetKind::Binary | BuildTargetKind::LibraryOrExample(_) + ) + }) + .any(|target| { + let relative_path = target + .path() + .strip_prefix(package_root_path) + .expect("targets to live inside package"); + + let Some(root_folder) = relative_path.components().next() else { + return false; + }; + + let root_folder = root_folder.as_str(); + + // Unfortunately doesn't seem to be any way to tell whether something rooted in + // src actually has tests or not, so best to just assume they do + root_folder == "tests" || root_folder == "src" + }) + } + + fn binary_targets(&self) -> Vec> { + let package_root_path = self + .manifest_path() + .parent() + .expect("all packages to have manifests with one parent"); + + self.build_targets() + .filter(|target| matches!(target.kind(), BuildTargetKind::Binary)) + .filter(|target| { + let relative_path = target + .path() + .strip_prefix(package_root_path) + .expect("targets to live inside package"); + + let Some(root_folder) = relative_path.components().next() else { + return false; + }; + + root_folder.as_str() == "src" && relative_path.file_name() == Some("main.rs") + }) + .collect() + } +} diff --git a/src/main.rs b/src/main.rs index 266ed38..14561d7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,20 +1,48 @@ #![allow(unstable_name_collisions)] -use determinator::{rules::DeterminatorRules, Determinator}; -use guppy::{ - graph::{BuildTarget, BuildTargetKind, DependencyDirection}, - CargoMetadata, -}; +mod config; +mod guppy_ext; -// TODO: Make this configurable via a file once I've finished iterating -const TESTS_THAT_NEED_DOCKER: &[&str] = &["integration-tests", "grafbase-gateway"]; +use determinator::Determinator; +use guppy::{graph::DependencyDirection, CargoMetadata}; +use guppy_ext::PackageMetadataExt; + +#[derive(serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "kebab-case")] +pub struct Report { + /// A string that can be passed to cargo build to limit to changed packages + cargo_build_specs: String, + + /// A string that can be passed to cargo test to limit to changed packages + /// + /// This one should be used for platforms that do not support docker + cargo_test_specs: String, + + /// A string that can be passed to cargo test to limit to changed packages + /// + /// This one should be used for platforms that support docker + cargo_docker_test_specs: String, + + /// A string that can be passed to cargo build to limit only to changed binaries + cargo_bin_specs: String, + + /// The full list of packages that have changed, useful for other CI filtering purposes + changed_packages: Vec, + + /// The full list of binaries that have changed, useful for other CI filtering purposes + changed_binaries: Vec, +} fn main() { let args = std::env::args().collect::>(); + let config = config::load(); + + eprintln!("Comparing metadata from {} to {}", &args[1], &args[2]); + // guppy accepts `cargo metadata` JSON output. Use a pre-existing fixture for these examples. let old_metadata = - CargoMetadata::parse_json(std::fs::read_to_string(dbg!(&args[1])).unwrap()).unwrap(); + CargoMetadata::parse_json(std::fs::read_to_string(&args[1]).unwrap()).unwrap(); let old = old_metadata.build_graph().unwrap(); let new_metadata = CargoMetadata::parse_json(std::fs::read_to_string(&args[2]).unwrap()).unwrap(); @@ -22,26 +50,22 @@ fn main() { let mut determinator = Determinator::new(&old, &new); - // The determinator supports custom rules read from a TOML file. - // let rules = - // DeterminatorRules::parse(include_str!("../../../fixtures/guppy/path-rules.toml")).unwrap(); + determinator.set_rules(&config.determinator_rules).unwrap(); - determinator - .set_rules(DeterminatorRules::default_rules()) - .unwrap(); + eprintln!("Changed files:"); + for path in &args[3..] { + eprintln!("- {path}"); + } + eprintln!(); // The determinator expects a list of changed files to be passed in. - determinator.add_changed_paths(dbg!(&args[3..])); + determinator.add_changed_paths(&args[3..]); let determinator_set = determinator.compute(); let cargo_build_specs = determinator_set .affected_set .packages(DependencyDirection::Forward) - .filter(|package| { - // TODO: Make this configurable somehow... - package.name() != "grafbase-docker-tests" - }) .flat_map(|package| ["-p", package.name()]) .collect::>() .join(" "); @@ -51,10 +75,17 @@ fn main() { .packages(DependencyDirection::Forward) .filter(|package| package.has_test_targets()) .filter(|package| { - // TODO: Make this configurable somehow... - package.name() != "grafbase-docker-tests" + !config + .ignore_test_packages + .iter() + .any(|name| package.name() == name) + }) + .filter(|package| { + !config + .docker_test_packages + .iter() + .any(|name| package.name() == name) }) - .filter(|package| !TESTS_THAT_NEED_DOCKER.contains(&package.name())) .flat_map(|package| ["-p", package.name()]) .collect::>() .join(" "); @@ -64,8 +95,10 @@ fn main() { .packages(DependencyDirection::Forward) .filter(|package| package.has_test_targets()) .filter(|package| { - // TODO: Make this configurable somehow... - package.name() != "grafbase-docker-tests" + !config + .ignore_test_packages + .iter() + .any(|name| package.name() == name) }) .flat_map(|package| ["-p", package.name()]) .collect::>() @@ -113,90 +146,3 @@ fn main() { println!("{}", serde_json::to_string(&report).unwrap()) } - -#[derive(serde::Serialize, serde::Deserialize)] -#[serde(rename_all = "kebab-case")] -pub struct Report { - /// A string that can be passed to cargo build to limit to changed packages - cargo_build_specs: String, - - /// A string that can be passed to cargo test to limit to changed packages - /// - /// This one should be used for platforms that do not support docker - cargo_test_specs: String, - - /// A string that can be passed to cargo test to limit to changed packages - /// - /// This one should be used for platforms that support docker - cargo_docker_test_specs: String, - - /// A string that can be passed to cargo build to limit only to changed binaries - cargo_bin_specs: String, - - /// The full list of packages that have changed, useful for other CI filtering purposes - changed_packages: Vec, - - /// The full list of binaries that have changed, useful for other CI filtering purposes - changed_binaries: Vec, -} - -trait PackageMetadataExt { - fn has_test_targets(&self) -> bool; - fn binary_targets(&self) -> Vec>; -} - -impl PackageMetadataExt for guppy::graph::PackageMetadata<'_> { - fn has_test_targets(&self) -> bool { - let package_root_path = self - .manifest_path() - .parent() - .expect("all packages to have manifests with one parent"); - - self.build_targets() - .filter(|target| { - matches!( - target.kind(), - BuildTargetKind::Binary | BuildTargetKind::LibraryOrExample(_) - ) - }) - .any(|target| { - let relative_path = target - .path() - .strip_prefix(package_root_path) - .expect("targets to live inside package"); - - let Some(root_folder) = relative_path.components().next() else { - return false; - }; - - let root_folder = root_folder.as_str(); - - // Unfortunately doesn't seem to be any way to tell whether something rooted in - // src actually has tests or not, so best to just assume they do - root_folder == "tests" || root_folder == "src" - }) - } - - fn binary_targets(&self) -> Vec> { - let package_root_path = self - .manifest_path() - .parent() - .expect("all packages to have manifests with one parent"); - - self.build_targets() - .filter(|target| matches!(target.kind(), BuildTargetKind::Binary)) - .filter(|target| { - let relative_path = target - .path() - .strip_prefix(package_root_path) - .expect("targets to live inside package"); - - let Some(root_folder) = relative_path.components().next() else { - return false; - }; - - root_folder.as_str() == "src" && relative_path.file_name() == Some("main.rs") - }) - .collect() - } -} From 7be3c923231520257cef4ebd05391e08ab105b77 Mon Sep 17 00:00:00 2001 From: Graeme Coupar Date: Thu, 5 Sep 2024 14:05:29 +0100 Subject: [PATCH 5/7] add CI --- .github/workflows/release.yaml | 35 +++++++++++++++++++++++++++++++++ .github/workflows/rust.yml | 36 ++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 .github/workflows/release.yaml create mode 100644 .github/workflows/rust.yml diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml new file mode 100644 index 0000000..a89b721 --- /dev/null +++ b/.github/workflows/release.yaml @@ -0,0 +1,35 @@ +name: Create release + +on: + push: + tags: + - "v*" + +jobs: + create-release: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Create Release + uses: taiki-e/create-gh-release-action@v1 + with: + token: ${{ secrets.GITHUB_TOKEN }} + + upload-artifacts: + strategy: + matrix: + os: + - ubuntu-latest + - macos-latest + - windows-latest + + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@v4 + + - name: Upload binaries + uses: taiki-e/upload-rust-binary-action@v1 + with: + bin: what-rust-changed + token: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml new file mode 100644 index 0000000..c116c03 --- /dev/null +++ b/.github/workflows/rust.yml @@ -0,0 +1,36 @@ +name: Build + +on: + push: + branches: [main] + pull_request: + branches: [main] + +env: + COLUMNS: 250 + CARGO_INCREMENTAL: 0 + RUSTFLAGS: "-W rust-2021-compatibility -D warnings" + RUST_BACKTRACE: short + NEXTEST_PROFILE: ci + CI: 1 + +jobs: + build-rust: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + + - uses: dtolnay/rust-toolchain@1.80.1 + with: + components: rustfmt + + - uses: Swatinem/rust-cache@v2 + + - name: Check formatting + shell: cargo + run: cargo fmt --check + + - name: Build + shell: cargo + run: cargo build From 531c5fe1667396905d147a85cf76fc5971be6dd3 Mon Sep 17 00:00:00 2001 From: Graeme Coupar Date: Thu, 5 Sep 2024 14:19:17 +0100 Subject: [PATCH 6/7] more config tweaks --- src/config.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/config.rs b/src/config.rs index 8e75c08..7cde844 100644 --- a/src/config.rs +++ b/src/config.rs @@ -15,6 +15,7 @@ fn do_load() -> Option { } #[derive(serde::Serialize, serde::Deserialize, Default)] +#[serde(rename_all = "kebab-case")] pub struct Config { /// Packages that shouldn't automatically be included in test runs pub ignore_test_packages: Vec, From 0f9c658411280aebe3003ae7e5a55bd8549f8211 Mon Sep 17 00:00:00 2001 From: Graeme Coupar Date: Thu, 5 Sep 2024 15:42:12 +0100 Subject: [PATCH 7/7] fix ci --- .github/workflows/rust.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index c116c03..7e2f1e5 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -28,9 +28,9 @@ jobs: - uses: Swatinem/rust-cache@v2 - name: Check formatting - shell: cargo + shell: bash run: cargo fmt --check - name: Build - shell: cargo + shell: bash run: cargo build