be more consistent about workspace members (#5921)

1. We have reasons for excluding some packages from the default
workspace members, but there was nothing checking that newly-added
packages were added to both. I added an additional check to `cargo xtask
check-workspace-deps` to ensure that `default-members` is equal to
`members` minus xtask and end-to-end-tests. This had the effect that the
tests in `api_identity` were not being run in CI!
2. Add `--workspace` to `cargo clippy` and `cargo doc` in CI to run
checks across the entire tree.
3. Fix clippy lints / doc errors that were not being caught in the
previous setup.

Author: iliana

.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

.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

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"

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"));

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::<BTreeSet<_>>();
+    let members = workspace.workspace_members.iter().collect::<BTreeSet<_>>();
+    let default_members =
+        workspace.workspace_default_members.iter().collect::<BTreeSet<_>>();
+    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

dev-tools/xtask/src/clippy.rs

@@ -42,6 +42,7 @@ pub fn run_cmd(args: ClippyArgs) -> Result<()> {
     command
         // Make sure we check everything.
         .arg("--all-targets")
+        .arg("--workspace")
         .arg("--")
         // For a list of lints, see
         // https://rust-lang.github.io/rust-clippy/master.

dev-tools/xtask/src/download.rs

@@ -218,7 +218,7 @@ async fn get_values_from_file<const N: usize>(
         .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('"');

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<Output> {
     for arg in &args {
         cmd.arg(arg);
     }
-    Ok(execute(cmd)?)
+    execute(cmd)
 }
 
 fn default_gateway_ip() -> Result<String> {
@@ -497,8 +495,8 @@ struct SledAgentConfig {
 impl SledAgentConfig {
     fn read(path: &Utf8Path) -> Result<Self> {
         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<Vec<Utf8PathBuf>> {
     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,

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)),

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<K, V, KE, VE>(
         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<TestResponse, anyhow::Error> {
         if let Some(error) = self.error {
             return Err(error);