diff --git a/Cargo.lock b/Cargo.lock index 4fc5e5a215f..290e225a327 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8602,6 +8602,8 @@ dependencies = [ "petgraph 0.8.3", "serde", "subprocess", + "swrite", + "thiserror 2.0.18", "toml 0.8.23", ] diff --git a/dev-tools/ls-apis/Cargo.toml b/dev-tools/ls-apis/Cargo.toml index 6d7301ced3d..d62329d9c37 100644 --- a/dev-tools/ls-apis/Cargo.toml +++ b/dev-tools/ls-apis/Cargo.toml @@ -20,6 +20,8 @@ newtype_derive.workspace = true parse-display.workspace = true petgraph.workspace = true serde.workspace = true +swrite.workspace = true +thiserror.workspace = true toml.workspace = true omicron-workspace-hack.workspace = true diff --git a/dev-tools/ls-apis/src/api_metadata.rs b/dev-tools/ls-apis/src/api_metadata.rs index f4b1fd38467..02aee6a636b 100644 --- a/dev-tools/ls-apis/src/api_metadata.rs +++ b/dev-tools/ls-apis/src/api_metadata.rs @@ -8,6 +8,7 @@ use crate::ClientPackageName; use crate::ServerComponentName; use crate::ServerPackageName; use crate::cargo::DepPath; +use crate::errors::{ErrorAccumulator, LoadError}; use crate::workspaces::Workspaces; use anyhow::{Result, bail}; use iddqd::IdOrdItem; @@ -21,10 +22,8 @@ use std::collections::BTreeSet; /// Describes the APIs in the system /// -/// This is the programmatic interface to the `api-manifest.toml` file. -#[derive(Deserialize)] -#[serde(deny_unknown_fields)] -#[serde(try_from = "RawApiMetadata")] +/// This is the programmatic interface to the `api-manifest.toml` file. It is +/// built from a deserialized `RawApiMetadata` by `AllApiMetadata::from_raw`. pub struct AllApiMetadata { apis: BTreeMap, deployment_units: IdOrdMap, @@ -40,6 +39,138 @@ pub struct AllApiMetadata { } impl AllApiMetadata { + /// Validate a deserialized [`RawApiMetadata`] into an `AllApiMetadata`, + /// recording any problems into `errors`. + /// + /// Returns `None` if any problems were recorded. + pub(crate) fn from_raw( + raw: RawApiMetadata, + errors: &mut ErrorAccumulator, + ) -> Option { + let mut apis = BTreeMap::new(); + + for api in raw.apis { + // Keep the first definition of any client package name; record each + // duplicate so the manifest author can resolve it. + if apis.contains_key(&api.client_package_name) { + errors.push(LoadError::DuplicateClientPackage { + name: api.client_package_name.clone(), + }); + continue; + } + apis.insert(api.client_package_name.clone(), api); + } + + let mut deployment_units = IdOrdMap::new(); + let mut server_components: IdOrdMap = IdOrdMap::new(); + for raw_unit in raw.deployment_units { + // Build this unit's list of component names (packages first, then + // embedded components) while registering each component in + // `server_components`. + let mut component_names = Vec::new(); + + for pkg in &raw_unit.packages { + component_names.push(pkg.clone()); + register_server_component( + &mut server_components, + ServerComponent { + name: pkg.clone(), + deployment_unit: raw_unit.name.clone(), + lifecycle: Lifecycle::SteadyState, + kind: ServerComponentKind::TopLevel, + }, + errors, + ); + } + + for embedded in &raw_unit.embedded_components { + if !raw_unit.packages.contains(&embedded.inside) { + errors.push(LoadError::EmbeddedComponentInsideMissing { + embedded_component: embedded.name.clone(), + inside: embedded.inside.clone(), + deployment_unit: raw_unit.name.clone(), + }); + continue; + } + component_names.push(embedded.name.clone()); + register_server_component( + &mut server_components, + ServerComponent { + name: embedded.name.clone(), + deployment_unit: raw_unit.name.clone(), + lifecycle: embedded.lifecycle, + kind: ServerComponentKind::Embedded { + inside: embedded.inside.clone(), + }, + }, + errors, + ); + } + + let info = + DeploymentUnitInfo { name: raw_unit.name, component_names }; + if let Err(e) = deployment_units.insert_unique(info) { + errors.push(LoadError::DuplicateDeploymentUnit { + name: e.new_item().name.clone(), + }); + } + } + + let mut dependency_rules = BTreeMap::new(); + for rule in raw.dependency_filter_rules { + if !apis.contains_key(&rule.client) { + errors.push(LoadError::UnknownDependencyRuleClient { + client: rule.client.clone(), + }); + continue; + } + + dependency_rules + .entry(rule.client.clone()) + .or_insert_with(Vec::new) + .push(rule); + } + + let mut ignored_non_clients = BTreeSet::new(); + for client_pkg in raw.ignored_non_clients { + if !ignored_non_clients.insert(client_pkg.clone()) { + errors.push(LoadError::DuplicateIgnoredNonClient { + client: client_pkg, + }); + } + } + + // Validate that IDU-only edges reference only known server components + // and APIs. + for edge in &raw.intra_deployment_unit_only_edges { + if server_components.get(&edge.server).is_none() { + errors.push(LoadError::IduUnknownServerComponent { + server: edge.server.clone(), + }); + } + + if !apis.contains_key(&edge.client) { + errors.push(LoadError::IduUnknownClient { + client: edge.client.clone(), + }); + } + } + + if errors.has_errors() { + return None; + } + + Some(AllApiMetadata { + apis, + deployment_units, + server_components, + dependency_rules, + ignored_non_clients, + intra_deployment_unit_only_edges: raw + .intra_deployment_unit_only_edges, + }) + } + /// Iterate over the distinct APIs described by the metadata pub fn apis(&self) -> impl Iterator { self.apis.values() @@ -162,7 +293,7 @@ impl AllApiMetadata { /// the transformation to `AllApiMetadata`. #[derive(Deserialize)] #[serde(deny_unknown_fields)] -struct RawApiMetadata { +pub(crate) struct RawApiMetadata { apis: Vec, deployment_units: Vec, dependency_filter_rules: Vec, @@ -170,8 +301,8 @@ struct RawApiMetadata { intra_deployment_unit_only_edges: Vec, } -/// Registers a server component, failing if a component with the same name has -/// already been registered. +/// Registers a server component, recording an error if a component with the +/// same name has already been registered. /// /// `server_components` is keyed by component name and spans every deployment /// unit, so this enforces that each component name appears exactly once across @@ -181,8 +312,11 @@ struct RawApiMetadata { fn register_server_component( server_components: &mut IdOrdMap, component: ServerComponent, -) -> Result<()> { + errors: &mut ErrorAccumulator, +) { if let Err(error) = server_components.insert_unique(component) { + // On conflict, the first-registered component stays in the map and the + // duplicate is dropped, so validation can keep going. let new = error.new_item(); // `IdOrdMap` is keyed by component name alone, so the new component // conflicts with exactly one previously-registered component. @@ -190,153 +324,17 @@ fn register_server_component( "a duplicate-key conflict has exactly one conflicting item", ); if previous.deployment_unit == new.deployment_unit { - bail!( - "server component {:?} appears more than once in \ - deployment unit {}; each component must appear exactly \ - once across `packages` and `embedded_components`", - new.name, - new.deployment_unit, - ); - } - bail!( - "server component {:?} appears in multiple deployment unit \ - entries ({} and {}); each component must appear exactly once \ - across `packages` and `embedded_components`", - new.name, - previous.deployment_unit, - new.deployment_unit, - ); - } - Ok(()) -} - -impl TryFrom for AllApiMetadata { - type Error = anyhow::Error; - - fn try_from(raw: RawApiMetadata) -> anyhow::Result { - let mut apis = BTreeMap::new(); - - for api in raw.apis { - if let Some(previous) = - apis.insert(api.client_package_name.clone(), api) - { - bail!( - "duplicate client package name in API metadata: {}", - &previous.client_package_name, - ); - } - } - - let mut deployment_units = IdOrdMap::new(); - let mut server_components: IdOrdMap = IdOrdMap::new(); - for raw_unit in raw.deployment_units { - // Build this unit's list of component names (packages first, then - // embedded components) while registering each component in - // `server_components`. - let mut component_names = Vec::new(); - - for pkg in &raw_unit.packages { - component_names.push(pkg.clone()); - register_server_component( - &mut server_components, - ServerComponent { - name: pkg.clone(), - deployment_unit: raw_unit.name.clone(), - lifecycle: Lifecycle::SteadyState, - kind: ServerComponentKind::TopLevel, - }, - )?; - } - - for embedded in &raw_unit.embedded_components { - if !raw_unit.packages.contains(&embedded.inside) { - bail!( - "embedded component {:?} has `inside = {:?}`, but \ - no such package exists in deployment unit {:?}'s \ - `packages` list", - embedded.name, - embedded.inside, - raw_unit.name, - ); - } - component_names.push(embedded.name.clone()); - register_server_component( - &mut server_components, - ServerComponent { - name: embedded.name.clone(), - deployment_unit: raw_unit.name.clone(), - lifecycle: embedded.lifecycle, - kind: ServerComponentKind::Embedded { - inside: embedded.inside.clone(), - }, - }, - )?; - } - - let info = - DeploymentUnitInfo { name: raw_unit.name, component_names }; - if let Err(e) = deployment_units.insert_unique(info) { - bail!( - "duplicate deployment unit name in API metadata: {}", - e.new_item().name, - ); - } - } - - let mut dependency_rules = BTreeMap::new(); - for rule in raw.dependency_filter_rules { - if !apis.contains_key(&rule.client) { - bail!( - "dependency rule references unknown client: {:?}", - rule.client - ); - } - - dependency_rules - .entry(rule.client.clone()) - .or_insert_with(Vec::new) - .push(rule); - } - - let mut ignored_non_clients = BTreeSet::new(); - for client_pkg in raw.ignored_non_clients { - if !ignored_non_clients.insert(client_pkg.clone()) { - bail!( - "entry in ignored_non_clients appeared twice: {:?}", - &client_pkg - ); - } - } - - // Validate that IDU-only edges reference only known server components - // and APIs. - for edge in &raw.intra_deployment_unit_only_edges { - if server_components.get(&edge.server).is_none() { - bail!( - "intra_deployment_unit_only_edges: \ - unknown server component {:?}", - edge.server - ); - } - - if !apis.contains_key(&edge.client) { - bail!( - "intra_deployment_unit_only_edges: \ - unknown client {:?}", - edge.client, - ); - } + errors.push(LoadError::ServerComponentRepeatedInUnit { + component: new.name.clone(), + deployment_unit: new.deployment_unit.clone(), + }); + } else { + errors.push(LoadError::ServerComponentDeclaredInMultipleUnits { + component: new.name.clone(), + first: previous.deployment_unit.clone(), + second: new.deployment_unit.clone(), + }); } - - Ok(AllApiMetadata { - apis, - deployment_units, - server_components, - dependency_rules, - ignored_non_clients, - intra_deployment_unit_only_edges: raw - .intra_deployment_unit_only_edges, - }) } } @@ -765,3 +763,99 @@ impl IntraDeploymentUnitOnlyEdge { self.server == *server && self.client == *client } } + +#[cfg(test)] +mod tests { + use super::*; + + /// A manifest with three independent, simultaneous defects: + /// + /// 1. two APIs sharing a client package name, + /// 2. an embedded component whose `inside` names no package in its unit, + /// 3. a dependency rule referencing an unknown client. + /// + /// All three are caught by `from_raw` alone, so tests built on this + /// manifest never reach the point at which they run `cargo metadata` and + /// analyze the Omicron repo itself. + const THREE_DEFECTS_TOML: &str = r#" +ignored_non_clients = [] +intra_deployment_unit_only_edges = [] + +[[apis]] +client_package_name = "dup-client" +label = "Dup One" +server_package_name = "server-one" +versioned_how = "unknown" + +[[apis]] +client_package_name = "dup-client" +label = "Dup Two" +server_package_name = "server-two" +versioned_how = "unknown" + +[[deployment_units]] +name = "Unit One" +packages = ["server-one"] + +[[deployment_units.embedded_components]] +name = "embedded-comp" +inside = "nonexistent-package" + +[[dependency_filter_rules]] +ancestor = "some-ancestor" +client = "unknown-client" +note = "why this rule exists" +"#; + + #[test] + fn from_raw_collects_multiple_errors() { + let raw: RawApiMetadata = + toml::from_str(THREE_DEFECTS_TOML).expect("test manifest parses"); + + let mut errors = ErrorAccumulator::new(); + let metadata = AllApiMetadata::from_raw(raw, &mut errors); + assert!( + metadata.is_none(), + "from_raw should return None when it records errors", + ); + + let collected = errors + .take_load_errors() + .expect("from_raw recorded errors, so this is non-empty"); + match collected.errors() { + [ + LoadError::DuplicateClientPackage { name }, + LoadError::EmbeddedComponentInsideMissing { + embedded_component, + inside, + deployment_unit, + }, + LoadError::UnknownDependencyRuleClient { client }, + ] => { + assert_eq!(name.as_str(), "dup-client"); + assert_eq!(embedded_component.as_str(), "embedded-comp"); + assert_eq!(inside.as_str(), "nonexistent-package"); + assert_eq!(deployment_unit.to_string(), "Unit One"); + assert_eq!(client.as_str(), "unknown-client"); + } + other => panic!("unexpected set of load errors: {other:#?}"), + } + } + + #[test] + fn load_errors_display_lists_every_error() { + let raw: RawApiMetadata = + toml::from_str(THREE_DEFECTS_TOML).expect("test manifest parses"); + + let mut errors = ErrorAccumulator::new(); + assert!(AllApiMetadata::from_raw(raw, &mut errors).is_none()); + let collected = errors + .take_load_errors() + .expect("from_raw recorded errors, so this is non-empty"); + + expectorate::assert_contents( + "tests/output/load_errors_display.txt", + &collected.to_string(), + ); + } +} diff --git a/dev-tools/ls-apis/src/errors.rs b/dev-tools/ls-apis/src/errors.rs new file mode 100644 index 00000000000..cc7a95e37ee --- /dev/null +++ b/dev-tools/ls-apis/src/errors.rs @@ -0,0 +1,367 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Collecting load-time errors so we can report many at once. +//! +//! Loading API metadata runs a sequence of independent validation passes (parse +//! the manifest, cross-check it against the Cargo dependency tree, walk +//! deployment units, and so on). Each pass records its errors into a shared +//! [`ErrorAccumulator`] and keeps going wherever it sensibly can, so a single +//! run can surface many problems at once. The caller gates between passes that +//! depend on each other -- there's no point validating the dependency graph if +//! the manifest itself didn't parse -- and reports everything collected so far +//! via [`LoadErrors`]. +//! +//! Errors are modeled as a structured [`LoadError`] enum, so callers can match +//! on the specific problem and each variant's data is captured explicitly. + +use crate::ClientPackageName; +use crate::ServerComponentName; +use crate::ServerPackageName; +use crate::plural; +use camino::Utf8PathBuf; +use indent_write::indentable::Indentable; +use itertools::Itertools; +use omicron_deployment_graph::DeploymentUnitName; +use std::error::Error; +use std::fmt; +use swrite::{SWrite as _, swrite}; + +/// A single problem encountered while loading API metadata. +/// +/// Variants are grouped by the validation pass that produces them. Passes +/// record these into an `ErrorAccumulator`, so a single run can surface every +/// problem at once. +#[derive(Debug, thiserror::Error)] +pub enum LoadError { + // + // Fatal load failures. + // + /// The API manifest file could not be read or parsed. + #[error("reading API manifest {path}")] + ReadManifest { + path: Utf8PathBuf, + #[source] + source: anyhow::Error, + }, + + /// Cargo workspace metadata could not be loaded. + #[error("loading Cargo workspace metadata")] + LoadWorkspaces { + #[source] + source: anyhow::Error, + }, + + // + // Manifest validation (`AllApiMetadata::from_raw`). + // + /// The same client package name was listed for more than one API. + #[error("duplicate client package name in API metadata: {name}")] + DuplicateClientPackage { name: ClientPackageName }, + + /// An embedded component's `inside` does not name a package in the same + /// unit. + #[error( + "embedded component {embedded_component:?} has `inside = {inside:?}`, \ + but no such package exists in deployment unit \ + {deployment_unit:?}'s `packages` list" + )] + EmbeddedComponentInsideMissing { + embedded_component: ServerComponentName, + inside: ServerComponentName, + deployment_unit: DeploymentUnitName, + }, + + /// Two deployment units share a name. + #[error("duplicate deployment unit name in API metadata: {name}")] + DuplicateDeploymentUnit { name: DeploymentUnitName }, + + /// A server component is listed twice within one deployment unit. + #[error( + "server component {component:?} appears more than once in deployment \ + unit {deployment_unit}; each component must appear exactly once across \ + `packages` and `embedded_components`" + )] + ServerComponentRepeatedInUnit { + component: ServerComponentName, + deployment_unit: DeploymentUnitName, + }, + + /// A server component is declared in two different deployment units' + /// entries. + #[error( + "server component {component:?} appears in multiple deployment unit \ + entries ({first} and {second}); each component must appear exactly \ + once across `packages` and `embedded_components`" + )] + ServerComponentDeclaredInMultipleUnits { + component: ServerComponentName, + first: DeploymentUnitName, + second: DeploymentUnitName, + }, + + /// A dependency filter rule names a client that isn't a known API. + #[error("dependency rule references unknown client: {client:?}")] + UnknownDependencyRuleClient { client: ClientPackageName }, + + /// The same entry appears twice in `ignored_non_clients`. + #[error("entry in ignored_non_clients appeared twice: {client:?}")] + DuplicateIgnoredNonClient { client: ClientPackageName }, + + /// An IDU-only edge names a server component that doesn't exist. + #[error( + "intra_deployment_unit_only_edges: unknown server component {server:?}" + )] + IduUnknownServerComponent { server: ServerComponentName }, + + /// An IDU-only edge names a client that isn't a known API. + #[error("intra_deployment_unit_only_edges: unknown client {client:?}")] + IduUnknownClient { client: ClientPackageName }, + + // + // Workspace consistency (`Workspaces::load`). + // + /// A workspace exposes a Progenitor client that the manifest doesn't list. + #[error( + "workspace {workspace}: found Progenitor-based client package missing \ + from API manifest: {client}" + )] + ClientPackageMissingFromManifest { + workspace: String, + client: ClientPackageName, + }, + + /// The manifest lists a client package that no workspace exposes. + #[error("API manifest refers to unknown client package: {client}")] + UnknownClientPackageInManifest { client: ClientPackageName }, + + // + // Deployment-unit walk (`SystemApis::load`). + // + /// Failed to resolve the workspace for an embedded component's `inside` + /// package. + #[error( + "resolving workspace for embedded component {embedded_component:?} \ + via its `inside` package {inside:?}" + )] + ResolveEmbeddedComponentWorkspace { + embedded_component: ServerComponentName, + inside: ServerComponentName, + #[source] + source: anyhow::Error, + }, + + /// An embedded component's package wasn't found in its expected workspace. + #[error( + "embedded component {embedded_component:?} not found in its expected \ + workspace\n \ + expected workspace: {workspace:?} (the workspace of its `inside` \ + package {inside:?})\n \ + deployment unit: {deployment_unit:?}\n \ + hint: check `name` for typos, or that the package exists in that workspace" + )] + EmbeddedComponentNotInWorkspace { + embedded_component: ServerComponentName, + workspace: String, + inside: ServerComponentName, + deployment_unit: DeploymentUnitName, + }, + + /// Failed to resolve the workspace for a server component. + #[error("resolving workspace for server component {component:?}")] + ResolveServerComponentWorkspace { + component: ServerComponentName, + #[source] + source: anyhow::Error, + }, + + /// Failed to walk the Cargo dependencies of a server component. + #[error("walking Cargo dependencies of server component {component:?}")] + WalkDependencies { + component: ServerComponentName, + #[source] + source: anyhow::Error, + }, + + /// An embedded component is declared `inside` a package that doesn't + /// depend on it, so omitting it from that package's dependency walk did + /// nothing. + #[error( + "embedded component {embedded_component:?} is declared `inside` a \ + package that doesn't depend on it\n \ + deployment unit: {deployment_unit:?}\n \ + `inside` package: {inside:?} (no normal or build Cargo dependency on \ + this embedded component)\n \ + hint: remove the stale `embedded_components` entry, fix its `inside` \ + field, or restore the dependency if it was removed or made dev-only" + )] + StaleEmbeddedComponent { + embedded_component: ServerComponentName, + deployment_unit: DeploymentUnitName, + inside: ServerComponentName, + }, + + // + // API producer/consumer validation (`SystemApis::load`). + // + /// An API restricts its consumers to one that isn't a known server + /// component. + #[error( + "api {client} specifies unknown consumer: {consumer} (with expected \ + reason: {reason})" + )] + ApiUnknownRestrictedConsumer { + client: ClientPackageName, + consumer: ServerComponentName, + reason: String, + }, + + /// A deployed API has no producer in any deployment unit. + #[error( + "found no producer for API with client package name {client:?} in any \ + deployment unit (should have been one that contains server package \ + {server:?})" + )] + NoProducerForApi { client: ClientPackageName, server: ServerPackageName }, + + /// An API marked as having no deployed producer nonetheless has one. + #[error( + "metadata says there should be no deployed producer for API with \ + client package name {client:?}, but found one(s): {}", + .producers.iter().format(", ") + )] + UnexpectedProducerForApi { + client: ClientPackageName, + producers: Vec, + }, + + // + // Intra-deployment-unit-only edge validation (`SystemApis::load`). + // + /// An IDU-only edge's server is absent from the tracked server components. + /// (Validated earlier, so this indicates an internal inconsistency.) + #[error( + "internal error: intra_deployment_unit_only specifies server \ + {server:?} that does not exist in server components" + )] + IduServerNotTracked { server: ServerComponentName }, + + /// An IDU-only edge's client doesn't correspond to a known API. + /// (Validated earlier, so this indicates an internal inconsistency.) + #[error( + "internal error: intra_deployment_unit_only specifies client \ + {client:?} that does not correspond to a known API" + )] + IduClientNotTracked { client: ClientPackageName }, + + /// An IDU-only edge names a non-deployed client API that has no producer in + /// any deployment unit, so the edge can never be satisfied. (A *deployed* + /// API in this state is reported as [`LoadError::NoProducerForApi`] + /// instead, so the same misconfiguration is never reported twice.) + #[error( + "intra_deployment_unit_only specifies server {server:?} with client \ + {client:?}, but {client:?} has no producer in any deployment unit" + )] + IduClientWithoutProducer { + server: ServerComponentName, + client: ClientPackageName, + }, + + /// None of an IDU-only edge's client's producers live in the same + /// deployment unit as its server. + #[error( + "intra_deployment_unit_only specifies server {server:?} in deployment \ + unit {deployment_unit:?}, but none of the producers of client \ + {client:?} are in that deployment unit: {}", + .producers.iter().format(", ") + )] + IduProducersNotInServerUnit { + server: ServerComponentName, + deployment_unit: DeploymentUnitName, + client: ClientPackageName, + producers: Vec, + }, +} + +/// Collects [`LoadError`]s encountered while loading system API metadata. +#[derive(Debug, Default)] +pub(crate) struct ErrorAccumulator { + errors: Vec, +} + +impl ErrorAccumulator { + pub(crate) fn new() -> ErrorAccumulator { + ErrorAccumulator::default() + } + + /// Records an error and keep going. + pub(crate) fn push(&mut self, error: LoadError) { + self.errors.push(error); + } + + /// Returns whether any error has been recorded so far. + pub(crate) fn has_errors(&self) -> bool { + !self.errors.is_empty() + } + + /// If any errors have been recorded, drains them into a [`LoadErrors`]. + /// Otherwise, returns `None`. + pub(crate) fn take_load_errors(&mut self) -> Option { + if self.errors.is_empty() { + None + } else { + Some(LoadErrors { errors: std::mem::take(&mut self.errors) }) + } + } +} + +/// The aggregate of every [`LoadError`] collected while loading API metadata. +/// +/// This is the error type returned by [`crate::SystemApis::load`]. Its +/// `Display` lists each underlying problem on its own line (including the source +/// chain of any wrapped error), rather than collapsing them to just the first. +#[derive(Debug)] +pub struct LoadErrors { + errors: Vec, +} + +impl LoadErrors { + /// The individual errors that were collected. + /// + /// This is always non-empty: a `LoadErrors` is only ever built from at + /// least one recorded error. + pub fn errors(&self) -> &[LoadError] { + &self.errors + } +} + +impl fmt::Display for LoadErrors { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let count = self.errors.len(); + writeln!( + f, + "found {} {} while loading API metadata:", + count, + plural::errors_str(count), + )?; + for error in &self.errors { + // Render the error and its source chain, one cause per line, then + // indent any continuation lines (chain causes, or a multi-line + // message) so they align under the bullet's text. + let mut rendered = error.to_string(); + let mut source = error.source(); + while let Some(cause) = source { + swrite!(rendered, "\ncaused by: {cause}"); + source = cause.source(); + } + writeln!(f, " - {}", rendered.indented_skip_initial(" "))?; + } + Ok(()) + } +} + +// `LoadErrors` is a list of errors, so it deliberately does not expose a single +// `source`. Use the Display impl to print out all errors, and `errors()` to +// iterate over them. +impl Error for LoadErrors {} diff --git a/dev-tools/ls-apis/src/lib.rs b/dev-tools/ls-apis/src/lib.rs index 8ecb59307c8..2ccd554a856 100644 --- a/dev-tools/ls-apis/src/lib.rs +++ b/dev-tools/ls-apis/src/lib.rs @@ -6,6 +6,7 @@ mod api_metadata; mod cargo; +mod errors; pub mod plural; mod system_apis; mod workspaces; @@ -16,6 +17,8 @@ pub use api_metadata::ApiExpectedConsumer; pub use api_metadata::ApiMetadata; pub use api_metadata::ServerComponent; pub use api_metadata::VersionedHow; +pub use errors::LoadError; +pub use errors::LoadErrors; pub use system_apis::ApiDependencyFilter; pub use system_apis::FailedConsumerCheck; pub use system_apis::SystemApis; diff --git a/dev-tools/ls-apis/src/system_apis.rs b/dev-tools/ls-apis/src/system_apis.rs index f50b32ee55b..2af58313bba 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -15,9 +15,13 @@ use crate::api_metadata::ApiExpectedConsumer; use crate::api_metadata::ApiExpectedConsumers; use crate::api_metadata::ApiMetadata; use crate::api_metadata::Evaluation; +use crate::api_metadata::RawApiMetadata; use crate::api_metadata::ServerComponentKind; use crate::api_metadata::VersionedHow; use crate::cargo::DepPath; +use crate::errors::ErrorAccumulator; +use crate::errors::LoadError; +use crate::errors::LoadErrors; use crate::parse_toml_file; use crate::workspaces::Workspaces; use anyhow::Result; @@ -27,7 +31,6 @@ use cargo_metadata::PackageId; use iddqd::IdOrdItem; use iddqd::IdOrdMap; use iddqd::id_upcast; -use itertools::Itertools; use omicron_deployment_graph::DagEdge; use omicron_deployment_graph::DagEdgesFile; use omicron_deployment_graph::DeploymentUnitName; @@ -126,25 +129,41 @@ impl<'a> IdOrdItem for FilteredApiConsumer<'a> { impl SystemApis { /// Load information about APIs in the system based on both developer- /// maintained metadata and Cargo-provided metadata - pub fn load(args: LoadArgs) -> Result { - // Load the API manifest. - let api_metadata: AllApiMetadata = - parse_toml_file(&args.api_manifest_path)?; - // Load Cargo metadata and validate it against the manifest. - let (workspaces, warnings) = Workspaces::load(&api_metadata)?; - if !warnings.is_empty() { - // We treat these warnings as fatal here. - for e in warnings { - eprintln!("error: {:#}", e); + pub fn load(args: LoadArgs) -> Result { + let mut errors = ErrorAccumulator::new(); + + // Parse the API manifest. If the file can't be read or parsed, there's + // nothing else to validate, so report that and exit early. + let raw: RawApiMetadata = match parse_toml_file(&args.api_manifest_path) + { + Ok(raw) => raw, + Err(source) => { + errors.push(LoadError::ReadManifest { + path: args.api_manifest_path.clone(), + source, + }); + return Err(errors + .take_load_errors() + .expect("the manifest read error was just recorded")); } + }; - bail!( - "found inconsistency between API manifest ({}) and \ - information found from the Cargo dependency tree \ - (see above)", - &args.api_manifest_path - ); - } + // Validate the manifest. Every later pass assumes valid metadata, so + // we stop early if the manifest had any problems. + let Some(api_metadata) = AllApiMetadata::from_raw(raw, &mut errors) + else { + return Err(errors.take_load_errors().expect( + "from_raw returns None only after recording an error", + )); + }; + + // Load Cargo metadata and cross-check it against the manifest. + let Some(workspaces) = Workspaces::load(&api_metadata, &mut errors) + else { + return Err(errors.take_load_errors().expect( + "Workspaces::load returns None only after recording an error", + )); + }; // Create an index of server package names, mapping each one to the list // of APIs that it produces. @@ -180,25 +199,37 @@ impl SystemApis { // the `inside` binary, so it necessarily lives in the // same workspace as `inside`. Resolve its package within // that workspace specifically. - let (workspace, _) = - workspaces.find_package_workspace(inside)?; - let embedded_pkg = workspace - .find_workspace_package(component.name()) - .ok_or_else(|| { - anyhow!( - "embedded component {:?} not found in its \ - expected workspace\n \ - expected workspace: {:?} (the workspace \ - of its `inside` package {:?})\n \ - deployment unit: {:?}\n \ - hint: check `name` for typos, or that the \ - package exists in that workspace", - component.name(), - workspace.name(), - inside, - component.deployment_unit(), - ) - })?; + let workspace = + match workspaces.find_package_workspace(inside) { + Ok((workspace, _)) => workspace, + Err(source) => { + errors.push( + LoadError::ResolveEmbeddedComponentWorkspace { + embedded_component: component + .name() + .clone(), + inside: inside.clone(), + source, + }, + ); + continue; + } + }; + let Some(embedded_pkg) = + workspace.find_workspace_package(component.name()) + else { + errors.push( + LoadError::EmbeddedComponentNotInWorkspace { + embedded_component: component.name().clone(), + workspace: workspace.name().to_owned(), + inside: inside.clone(), + deployment_unit: component + .deployment_unit() + .clone(), + }, + ); + continue; + }; omitted_pkgid_embedded.insert(&embedded_pkg.id, component); omitted_nodes .entry(inside) @@ -230,8 +261,20 @@ impl SystemApis { match component.kind() { ServerComponentKind::TopLevel => { - let (workspace, server_pkg) = workspaces - .find_package_workspace(component_name)?; + let (workspace, server_pkg) = match workspaces + .find_package_workspace(component_name) + { + Ok(found) => found, + Err(source) => { + errors.push( + LoadError::ResolveServerComponentWorkspace { + component: component_name.clone(), + source, + }, + ); + continue; + } + }; let dep_path = DepPath::for_pkg(server_pkg.id.clone()); tracker.found_package( component_name, @@ -242,10 +285,18 @@ impl SystemApis { let omitted = omitted_nodes.get(component_name).expect( "every server component has an omitted_nodes entry", ); - let outcome = workspace - .walk_required_deps_recursively( - server_pkg, omitted, - )?; + let outcome = match workspace + .walk_required_deps_recursively(server_pkg, omitted) + { + Ok(outcome) => outcome, + Err(source) => { + errors.push(LoadError::WalkDependencies { + component: component_name.clone(), + source, + }); + continue; + } + }; for pkg_outcome in &outcome.found { tracker.found_package( component_name, @@ -269,22 +320,13 @@ impl SystemApis { "every omitted package ID was \ registered with its embedded component", ); - bail!( - "embedded component {:?} is declared \ - `inside` a package that doesn't depend on \ - it\n \ - deployment unit: {:?}\n \ - `inside` package: {:?} (no normal or build \ - Cargo dependency on this embedded \ - component)\n \ - hint: remove the stale \ - `embedded_components` entry, fix its \ - `inside` field, or restore the dependency \ - if it was removed or made dev-only", - embedded.name(), - embedded.deployment_unit(), - component_name, - ); + errors.push(LoadError::StaleEmbeddedComponent { + embedded_component: embedded.name().clone(), + deployment_unit: embedded + .deployment_unit() + .clone(), + inside: component_name.clone(), + }); } } ServerComponentKind::Embedded { .. } => { @@ -300,6 +342,13 @@ impl SystemApis { let (unit_server_components, api_producers) = (tracker.unit_server_components, tracker.api_producers); + // The remaining passes interpret the producer/consumer graph we just + // built. If constructing the graph hit any problems, it is incomplete + // and further checks would report spurious errors, so bail out. + if let Some(load_errors) = errors.take_load_errors() { + return Err(load_errors); + } + // Ensure that if restricted_to_consumers is defined, all consumers // listed are specified by at least one deployment unit. for api in api_metadata.apis() { @@ -311,12 +360,12 @@ impl SystemApis { .server_component(&consumer.name) .is_none() { - bail!( - "api {} specifies unknown consumer: {} \ - (with expected reason: {})", - api.client_package_name, - consumer.name, - consumer.reason, + errors.push( + LoadError::ApiUnknownRestrictedConsumer { + client: api.client_package_name.clone(), + consumer: consumer.name.clone(), + reason: consumer.reason.clone(), + }, ); } } @@ -331,20 +380,32 @@ impl SystemApis { let mut deps_tracker = ClientDependenciesTracker::new(&api_metadata); for server_component in api_metadata.server_components() { let server_pkgname = server_component.name(); - let (workspace, pkg) = - workspaces.find_package_workspace(server_pkgname)?; + let (workspace, pkg) = match workspaces + .find_package_workspace(server_pkgname) + { + Ok(found) => found, + Err(source) => { + errors.push(LoadError::ResolveServerComponentWorkspace { + component: server_pkgname.clone(), + source, + }); + continue; + } + }; let omitted = omitted_nodes .get(server_pkgname) .expect("every server component has an omitted_nodes entry"); - let outcome = workspace - .walk_required_deps_recursively(pkg, omitted) - .with_context(|| { - format!( - "iterating dependencies of workspace {:?} package {:?}", - workspace.name(), - server_pkgname - ) - })?; + let outcome = + match workspace.walk_required_deps_recursively(pkg, omitted) { + Ok(outcome) => outcome, + Err(source) => { + errors.push(LoadError::WalkDependencies { + component: server_pkgname.clone(), + source, + }); + continue; + } + }; for pkg_outcome in &outcome.found { deps_tracker.found_dependency( server_pkgname, @@ -363,22 +424,16 @@ impl SystemApis { let found_producer = api_producers.get(&api.client_package_name); if api.deployed() { if found_producer.is_none() { - bail!( - "error: found no producer for API with client package \ - name {:?} in any deployment unit (should have been \ - one that contains server package {:?})", - api.client_package_name, - api.server_package_name, - ); + errors.push(LoadError::NoProducerForApi { + client: api.client_package_name.clone(), + server: api.server_package_name.clone(), + }); } } else if let Some(found) = found_producer { - bail!( - "error: metadata says there should be no deployed \ - producer for API with client package name {:?}, but found \ - one: {:?}", - api.client_package_name, - found - ); + errors.push(LoadError::UnexpectedProducerForApi { + client: api.client_package_name.clone(), + producers: found.keys().cloned().collect(), + }); } // Do any of the expected consumers of this API not actually use it? @@ -408,32 +463,66 @@ impl SystemApis { // Validate that the IDU-only edges' components belong to the same // deployment unit. + // + // Each `(server, client)` edge produces one of the following outcomes. + // `from_raw` already validates that both `server` and `client` are + // known, so the two "internal" rows are unreachable in practice. + // + // Row 3 fires only for non-deployed APIs: a *deployed* API with no + // producer is already reported as `NoProducerForApi` in the + // producer/consumer pass above, and we report each misconfiguration + // exactly once. + // + // server known? client known? has producer? in server's unit? | outcome + // + // (1) no - - - | IduServerNotTracked (internal) + // (2) yes no - - | IduClientNotTracked (internal) + // (3) yes yes no - | IduClientWithoutProducer (non-deployed only) + // (4) yes yes yes no | IduProducersNotInServerUnit + // (5) yes yes yes yes | ok (edge satisfied) for edge in api_metadata.intra_deployment_unit_only_edges() { let server = &edge.server; let Some(server_unit) = api_metadata .server_component(server) .map(|c| c.deployment_unit()) else { - // This was validated earlier, but there's not an easy way to - // express this in the type system, so we just handle it - // gracefully. - bail!( - "internal error: intra_deployment_unit_only specifies \ - server {:?} that does not exist in server components", - server, - ); + // Row 1 above. + errors.push(LoadError::IduServerNotTracked { + server: server.clone(), + }); + continue; }; let client = &edge.client; + let Some(client_api) = api_metadata.client_pkgname_lookup(client) + else { + // Row 2 above. + // + // (This is checked against the full set of known APIs, not + // against `api_producers`, which only contains APIs that have a + // producer.) + errors.push(LoadError::IduClientNotTracked { + client: client.clone(), + }); + continue; + }; + let Some(producers) = api_producers.get(client) else { - // This was validated earlier, but there's not an easy way to - // express this in the type system, so we just handle it - // gracefully. - bail!( - "internal error: intra_deployment_unit_only specifies \ - client {:?} that does not correspond to a known API", - client, - ); + // Row 3 above. + // + // A known API with no producer is a real error in the manifest: + // the edge names an API that nothing produces, so it can never + // be satisfied. A *deployed* API in this state was already + // reported as `NoProducerForApi` above, so to report each + // misconfiguration exactly once we only fire here for + // non-deployed APIs, which that pass deliberately ignores. + if !client_api.deployed() { + errors.push(LoadError::IduClientWithoutProducer { + server: server.clone(), + client: client.clone(), + }); + } + continue; }; if !producers.iter().any(|(p, _)| { @@ -442,18 +531,21 @@ impl SystemApis { .map(|c| c.deployment_unit() == server_unit) .unwrap_or(false) }) { - bail!( - "error: intra_deployment_unit_only specifies server \ - {:?} in deployment unit {:?}, but none of the producers \ - of client {:?} are in that deployment_unit: {}", - server, - server_unit, - client, - producers.keys().map(|p| p.as_str()).join(", "), - ); + // Row 4 above. + errors.push(LoadError::IduProducersNotInServerUnit { + server: server.clone(), + deployment_unit: server_unit.clone(), + client: client.clone(), + producers: producers.keys().cloned().collect(), + }); } } + // Report everything collected by the final group of validation passes. + if let Some(load_errors) = errors.take_load_errors() { + return Err(load_errors); + } + Ok(SystemApis { unit_server_components, apis_consumed, diff --git a/dev-tools/ls-apis/src/workspaces.rs b/dev-tools/ls-apis/src/workspaces.rs index f3cb89b19d9..710f4b09131 100644 --- a/dev-tools/ls-apis/src/workspaces.rs +++ b/dev-tools/ls-apis/src/workspaces.rs @@ -7,6 +7,7 @@ use crate::ClientPackageName; use crate::api_metadata::AllApiMetadata; use crate::cargo::Workspace; +use crate::errors::{ErrorAccumulator, LoadError}; use anyhow::{Context, Result, anyhow, ensure}; use camino::Utf8Path; use cargo_metadata::CargoOpt; @@ -26,13 +27,63 @@ impl Workspaces { /// Use `cargo metadata` to load workspace metadata for all the workspaces /// that we care about /// - /// The data found is validated against `api_metadata`. + /// The data found is validated against `api_metadata`. Any inconsistencies + /// between the API metadata and the Cargo metadata are recorded into + /// `errors`. /// - /// On success, returns `(workspaces, warnings)`, where `warnings` is a list - /// of inconsistencies between API metadata and Cargo metadata. + /// Returns `None` if the workspaces couldn't be loaded at all (e.g., a + /// `cargo metadata` invocation failed) or if an inconsistency was + /// recorded. pub fn load( api_metadata: &AllApiMetadata, - ) -> Result<(Workspaces, Vec)> { + errors: &mut ErrorAccumulator, + ) -> Option { + // If `cargo metadata` won't run, there's no `Workspaces` to return, so + // record that as a single fatal error. + let workspaces = match Self::load_workspace_map(api_metadata) { + Ok(workspaces) => workspaces, + Err(source) => { + errors.push(LoadError::LoadWorkspaces { source }); + return None; + } + }; + + // Validate the metadata against what we found in the workspaces. + let mut client_pkgnames_unused: BTreeSet<_> = + api_metadata.client_pkgnames().collect(); + for (_, workspace) in &workspaces { + for client_pkgname in workspace.client_packages() { + if api_metadata.client_pkgname_lookup(client_pkgname).is_some() + { + // It's possible that we will find multiple references + // to the same client package name. That's okay. + client_pkgnames_unused.remove(client_pkgname); + } else { + errors.push(LoadError::ClientPackageMissingFromManifest { + workspace: workspace.name().to_owned(), + client: client_pkgname.clone(), + }); + } + } + } + + for c in client_pkgnames_unused { + errors.push(LoadError::UnknownClientPackageInManifest { + client: c.clone(), + }); + } + + if errors.has_errors() { + return None; + } + + Some(Workspaces { workspaces }) + } + + /// Load every workspace we care about via `cargo metadata`. + fn load_workspace_map( + api_metadata: &AllApiMetadata, + ) -> Result> { // First, load information about the "omicron" workspace. This is the // current workspace so we don't need to provide the path to it. let ignored_non_clients = api_metadata.ignored_non_clients(); @@ -128,36 +179,7 @@ impl Workspaces { )?, ); - // Validate the metadata against what we found in the workspaces. - let mut client_pkgnames_unused: BTreeSet<_> = - api_metadata.client_pkgnames().collect(); - let mut warnings = Vec::new(); - for (_, workspace) in &workspaces { - for client_pkgname in workspace.client_packages() { - if api_metadata.client_pkgname_lookup(client_pkgname).is_some() - { - // It's possible that we will find multiple references - // to the same client package name. That's okay. - client_pkgnames_unused.remove(client_pkgname); - } else { - warnings.push(anyhow!( - "workspace {}: found Progenitor-based client package \ - missing from API manifest: {}", - workspace.name(), - client_pkgname - )); - } - } - } - - for c in client_pkgnames_unused { - warnings.push(anyhow!( - "API manifest refers to unknown client package: {}", - c - )); - } - - Ok((Workspaces { workspaces }, warnings)) + Ok(workspaces) } /// Given the name of a workspace package from one of our workspaces, return diff --git a/dev-tools/ls-apis/tests/output/load_errors_display.txt b/dev-tools/ls-apis/tests/output/load_errors_display.txt new file mode 100644 index 00000000000..7adfd5887a7 --- /dev/null +++ b/dev-tools/ls-apis/tests/output/load_errors_display.txt @@ -0,0 +1,4 @@ +found 3 errors while loading API metadata: + - duplicate client package name in API metadata: dup-client + - embedded component "embedded-comp" has `inside = "nonexistent-package"`, but no such package exists in deployment unit "Unit One"'s `packages` list + - dependency rule references unknown client: "unknown-client"