From 9cb10630078f212986e92a40926289c892cbf980 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Fri, 12 Jul 2024 04:56:33 +0000 Subject: [PATCH] review: xtask updates on options/parameters around install-version, ci Includes: - xtask documentation on main readme --- .github/buildomat/jobs/check-features.sh | 14 +++--- .github/workflows/rust.yml | 4 +- README.adoc | 18 ++++++++ dev-tools/xtask/src/check_features.rs | 57 +++++++++++++++++------- 4 files changed, 64 insertions(+), 29 deletions(-) diff --git a/.github/buildomat/jobs/check-features.sh b/.github/buildomat/jobs/check-features.sh index 7e8ffc79859..3b79baa22d9 100644 --- a/.github/buildomat/jobs/check-features.sh +++ b/.github/buildomat/jobs/check-features.sh @@ -6,8 +6,7 @@ #: rust_toolchain = true #: output_rules = [] -# Run cargo check on illumos with feature-specifics like `no-default-features` -# or `feature-powerset`. +# Run the check-features `xtask` on illumos, testing compilation of feature combinations. set -o errexit set -o pipefail @@ -28,13 +27,10 @@ source ./env.sh banner prerequisites ptime -m bash ./tools/install_builder_prerequisites.sh -y -banner check -export CARGO_INCREMENTAL=0 -ptime -m cargo check --workspace --bins --tests --no-default-features -RUSTDOCFLAGS="--document-private-items -D warnings" ptime -m cargo doc --workspace --no-deps --no-default-features - # # Check the feature set with the `cargo xtask check-features` command. # -banner hack -ptime -m timeout 2h cargo xtask check-features --version "$CARGO_HACK_VERSION" --exclude-features image-trampoline,image-standard +banner hack-check +export CARGO_INCREMENTAL=0 +ptime -m timeout 2h cargo xtask check-features --ci --install-version "$CARGO_HACK_VERSION" +RUSTDOCFLAGS="--document-private-items -D warnings" ptime -m cargo doc --workspace --no-deps --no-default-features diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index cc307fec595..7ee558edbea 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -106,15 +106,13 @@ jobs: run: cat "$GITHUB_ENV" - name: Install Pre-Requisites run: ./tools/install_builder_prerequisites.sh -y - - name: Run `cargo check` for no-default-features - run: cargo check --workspace --bins --tests --no-default-features # Uses manifest for install - uses: taiki-e/install-action@v2 with: tool: cargo-hack@${{ env.CARGO_HACK_VERSION }} - name: Run Check on Features (Feature-Powerset, No-Dev-Deps) timeout-minutes: 120 # 2 hours - run: cargo xtask check-features --no-install --exclude-features image-trampoline,image-standard + run: cargo xtask check-features --ci # This is just a test build of docs. Publicly available docs are built via # the separate "rustdocs" repo. diff --git a/README.adoc b/README.adoc index f0e3a883439..2ea3896f359 100644 --- a/README.adoc +++ b/README.adoc @@ -112,6 +112,24 @@ cargo nextest run We check that certain system library dependencies are not leaked outside of their intended binaries via `cargo xtask verify-libraries` in CI. If you are adding a new dependency on a illumos/helios library it is recommended that you update xref:.cargo/xtask.toml[] with an allow list of where you expect the dependency to show up. For example some libraries such as `libnvme.so.1` are only available in the global zone and therefore will not be present in any other zone. This check is here to help us catch any leakage before we go to deploy on a rack. You can inspect a compiled binary in the target directory for what it requires by using `elfedit` - for example `elfedit -r -e 'dyn:tag NEEDED' /path/to/omicron/target/debug/sled-agent`. +=== Checking feature flag combinations + +To ensure that varying combinations of features compile, run `cargo xtask check-features`, which executes the https://github.com/taiki-e/cargo-hack[`cargo hack`] subcommand under the hood. This `xtask` is run in CI using the `--ci` parameter , which automatically exludes certain `image-*` features that purposefully cause compiler errors if set. + +If you don't have `cargo hack` installed locally, run the the `xtask` with the `install-version ` option, which will install it into your user's `.cargo` directory: + +[source,text] +---- +$ cargo xtask check-features --install-version 0.6.28 +---- + +To limit the max number of simultaneous feature flags combined for checking, run the `xtask` with the `--depth ` flag: + +[source,text] +---- +$ cargo xtask check-features --depth 2 +---- + === Rust packages in Omicron NOTE: The term "package" is overloaded: most programming languages and operating systems have their own definitions of a package. On top of that, Omicron bundles up components into our own kind of "package" that gets delivered via the install and update systems. These are described in the `package-manifest.toml` file in the root of the repo. In this section, we're just concerned with Rust packages. diff --git a/dev-tools/xtask/src/check_features.rs b/dev-tools/xtask/src/check_features.rs index 5f690e08502..9b5883e7c1d 100644 --- a/dev-tools/xtask/src/check_features.rs +++ b/dev-tools/xtask/src/check_features.rs @@ -6,52 +6,74 @@ use anyhow::{bail, Context, Result}; use clap::Parser; -use std::process::Command; +use std::{collections::HashSet, process::Command}; /// The default version of `cargo-hack` to install. /// We use a patch-floating version to avoid breaking the build when a new /// version is released (locally). const FLOAT_VERSION: &str = "~0.6.28"; +const CI_EXCLUDED_FEATURES: [&str; 2] = ["image-trampoline", "image-standard"]; + #[derive(Parser)] pub struct Args { + /// Run in CI mode, with a default set of features excluded. + #[clap(long, default_value_t = false)] + ci: bool, /// Features to exclude from the check. - #[clap(long)] + #[clap(long, value_name = "FEATURES")] exclude_features: Option>, /// Depth of the feature powerset to check. - #[clap(long)] + #[clap(long, value_name = "NUM")] depth: Option, /// Error format passed to `cargo hack check`. #[clap(long, value_name = "FMT")] message_format: Option, - /// Do not install `cargo-hack` before running the check. - #[clap(long, default_value_t = false)] - no_install: bool, /// Version of `cargo-hack` to install. - #[clap(long)] - version: Option, + #[clap(long, value_name = "VERSION")] + install_version: Option, } /// Run `cargo hack check`. pub fn run_cmd(args: Args) -> Result<()> { - if !args.no_install { - install_cargo_hack(args.version).unwrap(); + // Install `cargo-hack` if the `install-version` was specified. + if let Some(version) = args.install_version { + install_cargo_hack(Some(version))?; } let cargo = std::env::var("CARGO").unwrap_or_else(|_| String::from("cargo")); let mut command = Command::new(&cargo); + // Add the `hack check` subcommand. command.args(&["hack", "check"]); - if let Some(features) = args.exclude_features { - let ex = format!("--exclude-features={}", features.join(",")); - command.arg(ex); + // Add the `--exclude-features` flag if we are running in CI mode. + if args.ci { + let ex = if let Some(mut features) = args.exclude_features { + // Extend the list of features to exclude with the CI defaults. + features.extend( + CI_EXCLUDED_FEATURES.into_iter().map(|s| s.to_string()), + ); + + // Remove duplicates. + let excludes = features.into_iter().collect::>(); + + excludes.into_iter().collect::>().join(",") + } else { + CI_EXCLUDED_FEATURES.join(",") + }; + + command.args(["--exclude-features", &ex]); + } else { + // Add "only" the `--exclude-features` flag if it was provided. + if let Some(features) = args.exclude_features { + command.args(["--exclude-features", &features.join(",")]); + } } if let Some(depth) = args.depth { - let depth = format!("depth={}", depth); - command.arg(depth); + command.args(&["--depth", &depth.to_string()]); } // Pass along the `--message-format` flag if it was provided. @@ -62,11 +84,12 @@ pub fn run_cmd(args: Args) -> Result<()> { command // Make sure we check everything. .arg("--workspace") + // We want to check the binaries. .arg("--bins") // We want to check the feature powerset. .arg("--feature-powerset") - .arg("--no-dev-deps") - .arg("--exclude-no-default-features"); + // We will not check the dev-dependencies, which should covered by tests. + .arg("--no-dev-deps"); eprintln!( "running: {:?} {}",