From 7ec0e8881dfe540d0f7d30cd6c90791d9b25a5ee Mon Sep 17 00:00:00 2001 From: iliana etaoin Date: Thu, 20 Jun 2024 16:47:56 +0000 Subject: [PATCH 1/5] add --workspace --keep-going to xtask clippy --- api_identity/src/lib.rs | 18 ++++++------------ dev-tools/xtask/src/clippy.rs | 2 ++ dev-tools/xtask/src/download.rs | 2 +- dev-tools/xtask/src/virtual_hardware.rs | 12 +++++------- end-to-end-tests/src/helpers/icmp.rs | 2 +- 5 files changed, 15 insertions(+), 21 deletions(-) diff --git a/api_identity/src/lib.rs b/api_identity/src/lib.rs index 4d933ed3c0..f90b1e3a89 100644 --- a/api_identity/src/lib.rs +++ b/api_identity/src/lib.rs @@ -60,12 +60,9 @@ mod test { #[test] fn test_identity() { - let ret = do_object_identity( - quote! { - struct Foo { identity: IdentityMetadata } - } - .into(), - ); + let ret = do_object_identity(quote! { + struct Foo { identity: IdentityMetadata } + }); let expected = quote! { impl ObjectIdentity for Foo { @@ -80,12 +77,9 @@ mod test { #[test] fn test_identity_no_field() { - let ret = do_object_identity( - quote! { - struct Foo {} - } - .into(), - ); + let ret = do_object_identity(quote! { + struct Foo {} + }); let error = ret.unwrap_err(); assert!(error.to_string().starts_with("deriving ObjectIdentity")); diff --git a/dev-tools/xtask/src/clippy.rs b/dev-tools/xtask/src/clippy.rs index 8c454fdebf..7adf3e9476 100644 --- a/dev-tools/xtask/src/clippy.rs +++ b/dev-tools/xtask/src/clippy.rs @@ -42,6 +42,8 @@ pub fn run_cmd(args: ClippyArgs) -> Result<()> { command // Make sure we check everything. .arg("--all-targets") + .arg("--workspace") + .arg("--keep-going") .arg("--") // For a list of lints, see // https://rust-lang.github.io/rust-clippy/master. diff --git a/dev-tools/xtask/src/download.rs b/dev-tools/xtask/src/download.rs index ce227b7c4d..f7583d19a0 100644 --- a/dev-tools/xtask/src/download.rs +++ b/dev-tools/xtask/src/download.rs @@ -218,7 +218,7 @@ async fn get_values_from_file( .context("Failed to read {path}")?; for line in content.lines() { let line = line.trim(); - let Some((key, value)) = line.split_once("=") else { + let Some((key, value)) = line.split_once('=') else { continue; }; let value = value.trim_matches('"'); diff --git a/dev-tools/xtask/src/virtual_hardware.rs b/dev-tools/xtask/src/virtual_hardware.rs index 5384433f55..d28c3d9037 100644 --- a/dev-tools/xtask/src/virtual_hardware.rs +++ b/dev-tools/xtask/src/virtual_hardware.rs @@ -235,9 +235,7 @@ fn unload_xde_driver() -> Result<()> { .context("Invalid modinfo output")? .lines() .find_map(|line| { - let mut cols = line.trim().splitn(2, ' '); - let id = cols.next()?; - let desc = cols.next()?; + let (id, desc) = line.trim().split_once(' ')?; if !desc.contains("xde") { return None; } @@ -419,7 +417,7 @@ fn run_scadm_command(args: Vec<&str>) -> Result { for arg in &args { cmd.arg(arg); } - Ok(execute(cmd)?) + execute(cmd) } fn default_gateway_ip() -> Result { @@ -497,8 +495,8 @@ struct SledAgentConfig { impl SledAgentConfig { fn read(path: &Utf8Path) -> Result { let config = std::fs::read_to_string(path)?; - Ok(toml::from_str(&config) - .context("Could not parse sled agent config as toml")?) + toml::from_str(&config) + .context("Could not parse sled agent config as toml") } } @@ -605,7 +603,7 @@ fn swap_list() -> Result> { let mut cmd = Command::new(SWAP); cmd.arg("-l"); - let output = cmd.output().context(format!("Could not start swap"))?; + let output = cmd.output().context("Could not start swap")?; if !output.status.success() { if let Ok(stderr) = String::from_utf8(output.stderr) { // This is an exceptional case - if there are no swap devices, diff --git a/end-to-end-tests/src/helpers/icmp.rs b/end-to-end-tests/src/helpers/icmp.rs index 36b9bc5675..40256db8d9 100644 --- a/end-to-end-tests/src/helpers/icmp.rs +++ b/end-to-end-tests/src/helpers/icmp.rs @@ -150,7 +150,7 @@ impl Pinger4 { "{:.3}", (t.sum.as_micros() as f32 / 1000.0 - / t.rx_count as f32) + / f32::from(t.rx_count)) ) }, format!("{:.3}", (t.high.as_micros() as f32 / 1000.0)), From 8bea28743a729df0be9995732187de28748aa177 Mon Sep 17 00:00:00 2001 From: iliana etaoin Date: Thu, 20 Jun 2024 17:26:55 +0000 Subject: [PATCH 2/5] enforce some (any) policy for default-members --- Cargo.toml | 13 +++++++-- dev-tools/xtask/src/check_workspace_deps.rs | 32 ++++++++++++++++++++- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 69269a1de4..2e6e556519 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -87,6 +87,7 @@ members = [ ] default-members = [ + "api_identity", "bootstore", "certificates", "clients/bootstrap-agent-client", @@ -113,6 +114,8 @@ default-members = [ # hakari to not work as well and build times to be longer. # See omicron#4392. "dns-server", + # Do not include end-to-end-tests in the list of default members, as its + # tests only work on a deployed control plane. "gateway-cli", "gateway-test-utils", "gateway", @@ -128,18 +131,21 @@ default-members = [ "nexus-config", "nexus/authz-macros", "nexus/auth", - "nexus/macros-common", - "nexus/metrics-producer-gc", - "nexus/networking", "nexus/db-fixed-data", "nexus/db-macros", "nexus/db-model", "nexus/db-queries", "nexus/defaults", "nexus/inventory", + "nexus/macros-common", + "nexus/metrics-producer-gc", + "nexus/networking", "nexus/reconfigurator/execution", "nexus/reconfigurator/planning", "nexus/reconfigurator/preparation", + "nexus/test-interface", + "nexus/test-utils-macros", + "nexus/test-utils", "nexus/types", "oximeter/collector", "oximeter/db", @@ -166,6 +172,7 @@ default-members = [ "wicket-dbg", "wicket", "wicketd", + "workspace-hack", "zone-setup", ] resolver = "2" diff --git a/dev-tools/xtask/src/check_workspace_deps.rs b/dev-tools/xtask/src/check_workspace_deps.rs index 76e405ce1a..94de557a46 100644 --- a/dev-tools/xtask/src/check_workspace_deps.rs +++ b/dev-tools/xtask/src/check_workspace_deps.rs @@ -8,7 +8,7 @@ use anyhow::{bail, Context, Result}; use camino::Utf8Path; use cargo_toml::{Dependency, Manifest}; use fs_err as fs; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; const WORKSPACE_HACK_PACKAGE_NAME: &str = "omicron-workspace-hack"; @@ -116,6 +116,36 @@ pub fn run_cmd() -> Result<()> { nerrors += 1; } + // Check that `default-members` is configured correctly. + let non_default = workspace + .packages + .iter() + .filter_map(|package| { + [ + // Including xtask causes hakari to not work as well and build + // times to be longer (omicron#4392). + "xtask", + // The tests here should not be run by default, as they require + // a running control plane. + "end-to-end-tests", + ] + .contains(&package.name.as_str()) + .then_some(&package.id) + }) + .collect::>(); + let members = workspace.workspace_members.iter().collect::>(); + let default_members = + workspace.workspace_default_members.iter().collect::>(); + for package in members.difference(&default_members) { + if !non_default.contains(package) { + eprintln!( + "error: package {:?} not in default-members", + package.repr + ); + nerrors += 1; + } + } + eprintln!( "check-workspace-deps: errors: {}, warnings: {}", nerrors, nwarnings From 9df256f3f38880389d716b741f6c789c35886fbc Mon Sep 17 00:00:00 2001 From: iliana etaoin Date: Thu, 20 Jun 2024 18:51:48 +0000 Subject: [PATCH 3/5] fix a doc error --- nexus/test-utils/src/http_testing.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs index 90c7dd43bc..1a85d7094c 100644 --- a/nexus/test-utils/src/http_testing.rs +++ b/nexus/test-utils/src/http_testing.rs @@ -216,8 +216,8 @@ impl<'a> RequestBuilder<'a> { /// Add header and value to check for at execution time /// - /// Behaves like header() rather than expect_allowed_headers() in that it - /// takes one header at a time rather than a whole set. + /// Behaves like header() in that it takes one header at a time rather than + /// a whole set. pub fn expect_response_header( mut self, name: K, @@ -291,8 +291,9 @@ impl<'a> RequestBuilder<'a> { /// response, and make the response available to the caller /// /// This function checks the returned status code (if [`Self::expect_status()`] - /// was used), allowed headers (if [`Self::expect_allowed_headers()`] was used), and - /// various other properties of the response. + /// was used), allowed headers (if [`Self::expect_websocket_handshake()`] or + /// [`Self::expect_console_asset()`] was used), and various other properties + /// of the response. pub async fn execute(self) -> Result { if let Some(error) = self.error { return Err(error); From 298aaed0bedabad5ff06b8a66b3c5cd4d3d42778 Mon Sep 17 00:00:00 2001 From: iliana etaoin Date: Thu, 20 Jun 2024 18:52:02 +0000 Subject: [PATCH 4/5] also add --workspace --keep-going to cargo doc --- .github/buildomat/jobs/clippy.sh | 2 +- .github/workflows/rust.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/buildomat/jobs/clippy.sh b/.github/buildomat/jobs/clippy.sh index cff9c45a1b..e9265d6253 100755 --- a/.github/buildomat/jobs/clippy.sh +++ b/.github/buildomat/jobs/clippy.sh @@ -30,4 +30,4 @@ ptime -m bash ./tools/install_builder_prerequisites.sh -y banner clippy export CARGO_INCREMENTAL=0 ptime -m cargo xtask clippy -ptime -m cargo doc +RUSTDOCFLAGS="-Dwarnings" ptime -m cargo doc --workspace --keep-going diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 724f88e7a3..5eaf081406 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -108,4 +108,4 @@ jobs: - name: Install Pre-Requisites run: ./tools/install_builder_prerequisites.sh -y - name: Test build documentation - run: RUSTDOCFLAGS="-Dwarnings" cargo doc + run: RUSTDOCFLAGS="-Dwarnings" cargo doc --workspace --keep-going From 2c8b77b11f0e64c6edae6a226ee7f75a054dd863 Mon Sep 17 00:00:00 2001 From: iliana etaoin Date: Thu, 20 Jun 2024 20:13:40 +0000 Subject: [PATCH 5/5] --keep-goin't --- .github/buildomat/jobs/clippy.sh | 2 +- .github/workflows/rust.yml | 2 +- dev-tools/xtask/src/clippy.rs | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/buildomat/jobs/clippy.sh b/.github/buildomat/jobs/clippy.sh index e9265d6253..7da32ec331 100755 --- a/.github/buildomat/jobs/clippy.sh +++ b/.github/buildomat/jobs/clippy.sh @@ -30,4 +30,4 @@ ptime -m bash ./tools/install_builder_prerequisites.sh -y banner clippy export CARGO_INCREMENTAL=0 ptime -m cargo xtask clippy -RUSTDOCFLAGS="-Dwarnings" ptime -m cargo doc --workspace --keep-going +RUSTDOCFLAGS="-Dwarnings" ptime -m cargo doc --workspace diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 5eaf081406..1888481ba2 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -108,4 +108,4 @@ jobs: - name: Install Pre-Requisites run: ./tools/install_builder_prerequisites.sh -y - name: Test build documentation - run: RUSTDOCFLAGS="-Dwarnings" cargo doc --workspace --keep-going + run: RUSTDOCFLAGS="-Dwarnings" cargo doc --workspace diff --git a/dev-tools/xtask/src/clippy.rs b/dev-tools/xtask/src/clippy.rs index 7adf3e9476..7924a05574 100644 --- a/dev-tools/xtask/src/clippy.rs +++ b/dev-tools/xtask/src/clippy.rs @@ -43,7 +43,6 @@ pub fn run_cmd(args: ClippyArgs) -> Result<()> { // Make sure we check everything. .arg("--all-targets") .arg("--workspace") - .arg("--keep-going") .arg("--") // For a list of lints, see // https://rust-lang.github.io/rust-clippy/master.