diff --git a/dev-tools/ls-apis/src/bin/ls-apis.rs b/dev-tools/ls-apis/src/bin/ls-apis.rs index 05d03788758..1d16f5cf1bf 100644 --- a/dev-tools/ls-apis/src/bin/ls-apis.rs +++ b/dev-tools/ls-apis/src/bin/ls-apis.rs @@ -8,8 +8,8 @@ use anyhow::{Context, Result, bail}; use camino::Utf8PathBuf; use clap::{Args, Parser, Subcommand}; use omicron_ls_apis::{ - AllApiMetadata, ApiDependencyFilter, LoadArgs, ServerComponentName, - SystemApis, VersionedHow, + AllApiMetadata, ApiDependencyFilter, ApiMetadata, LoadArgs, + ServerComponentName, SystemApis, VersionedHow, }; use parse_display::{Display, FromStr}; @@ -108,17 +108,15 @@ fn run_adoc(apis: &SystemApis) -> Result<()> { let metadata = apis.api_metadata(); for api in metadata.apis() { - let Some(server_component) = - apis.api_producer(&api.client_package_name) - else { - continue; - }; println!("// DO NOT EDIT. This table is auto-generated. See above."); - println!("|{}", api.label); - println!("|{}", apis.adoc_label(server_component)?); println!("|{}", apis.adoc_label(&api.client_package_name)?); + println!("|"); + for server_component in apis.api_producers(&api.client_package_name) { + println!("* {}", apis.adoc_label(server_component)?); + } + println!("|"); for (c, _) in apis.api_consumers( &api.client_package_name, ApiDependencyFilter::default(), @@ -214,12 +212,10 @@ fn print_server_components<'a>( for s in server_components.into_iter() { let (repo_name, pkg_path) = apis.package_label(s)?; println!("{}{} ({}/{})", prefix, s, repo_name, pkg_path); - for api in metadata.apis().filter(|a| { - matches!( - apis.api_producer(&a.client_package_name), - Some (name) if name == s - ) - }) { + for api in metadata + .apis() + .filter(|a| apis.is_producer_of(s, &a.client_package_name)) + { println!( "{} exposes: {} (client = {})", prefix, api.label, api.client_package_name @@ -301,6 +297,18 @@ fn run_check(apis: &SystemApis) -> Result<()> { ); } + fn print_api_and_producers(api: &ApiMetadata, apis: &SystemApis) { + print!(" {} ({}", api.label, api.client_package_name,); + let mut producers = apis.api_producers(&api.client_package_name); + if let Some(producer) = producers.next() { + print!(", exposed by {producer}"); + for producer in producers { + print!(", {producer}") + } + } + println!(")"); + } + println!("\n"); println!("Server-managed APIs:\n"); for api in apis @@ -308,24 +316,14 @@ fn run_check(apis: &SystemApis) -> Result<()> { .apis() .filter(|f| f.deployed() && f.versioned_how == VersionedHow::Server) { - println!( - " {} ({}, exposed by {})", - api.label, - api.client_package_name, - apis.api_producer(&api.client_package_name).unwrap() - ); + print_api_and_producers(api, apis); } println!("\n"); println!("Client-managed API:\n"); for api in apis.api_metadata().apis().filter(|f| f.deployed()) { if let VersionedHow::Client(reason) = &api.versioned_how { - println!( - " {} ({}, exposed by {})", - api.label, - api.client_package_name, - apis.api_producer(&api.client_package_name).unwrap() - ); + print_api_and_producers(api, apis); println!(" reason: {}", reason); } } @@ -342,12 +340,7 @@ fn run_check(apis: &SystemApis) -> Result<()> { } else { println!("\n"); for api in unknown { - println!( - " {} ({}, exposed by {})", - api.label, - api.client_package_name, - apis.api_producer(&api.client_package_name).unwrap() - ); + print_api_and_producers(api, apis); } bail!("at least one API has unknown version strategy (see above)"); } diff --git a/dev-tools/ls-apis/src/lib.rs b/dev-tools/ls-apis/src/lib.rs index 4da58bfe892..403f7324e64 100644 --- a/dev-tools/ls-apis/src/lib.rs +++ b/dev-tools/ls-apis/src/lib.rs @@ -10,6 +10,7 @@ mod system_apis; mod workspaces; pub use api_metadata::AllApiMetadata; +pub use api_metadata::ApiMetadata; pub use api_metadata::VersionedHow; pub use system_apis::ApiDependencyFilter; 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 edcd3d019b8..d867fc96f20 100644 --- a/dev-tools/ls-apis/src/system_apis.rs +++ b/dev-tools/ls-apis/src/system_apis.rs @@ -51,8 +51,8 @@ pub struct SystemApis { BTreeMap>, >, - /// maps an API name to the server component that exposes that API - api_producers: BTreeMap, + /// maps an API name to the server component(s) that expose that API + api_producers: BTreeMap, /// source of developer-maintained API metadata api_metadata: AllApiMetadata, @@ -60,6 +60,8 @@ pub struct SystemApis { workspaces: Workspaces, } +type ApiProducerMap = BTreeMap>; + impl SystemApis { /// Load information about APIs in the system based on both developer- /// maintained metadata and Cargo-provided metadata @@ -122,14 +124,6 @@ impl SystemApis { } } - if !tracker.errors.is_empty() { - for e in tracker.errors { - eprintln!("error: {:#}", e); - } - - bail!("found at least one API exported by multiple servers"); - } - let (server_component_units, unit_server_components, api_producers) = ( tracker.server_component_units, tracker.unit_server_components, @@ -259,12 +253,15 @@ impl SystemApis { } /// Given the client package name for an API, return the name of the server - /// component that provides it - pub fn api_producer( - &self, + /// component(s) that provide it + pub fn api_producers<'apis>( + &'apis self, client: &ClientPackageName, - ) -> Option<&ServerComponentName> { - self.api_producers.get(client).map(|s| &s.0) + ) -> impl Iterator + 'apis { + self.api_producers + .get(client) + .into_iter() + .flat_map(|producers| producers.keys()) } /// Given the client package name for an API, return the list of server @@ -303,6 +300,20 @@ impl SystemApis { Ok(rv.into_iter()) } + /// Given the client package name for an API and the name of a server + /// component, returns `true` if the server is a producer of that API, or + /// `false` if it is not. + pub fn is_producer_of( + &self, + server: &ServerComponentName, + client: &ClientPackageName, + ) -> bool { + self.api_producers + .get(client) + .map(|producers| producers.contains_key(server)) + .unwrap_or(false) + } + /// Given the name of any package defined in one of our workspaces, return /// information used to construct a label /// @@ -351,14 +362,27 @@ impl SystemApis { for (client_pkg, _) in self.component_apis_consumed(server_pkg, filter)? { - let other_component = - self.api_producer(client_pkg).unwrap(); - let other_unit = self - .server_component_units - .get(other_component) - .unwrap(); - let other_node = nodes.get(other_unit).unwrap(); - graph.add_edge(*my_node, *other_node, client_pkg.clone()); + // Multiple server components may produce an API. However, + // if an API is produced by multiple server components + // within the same deployment unit, we would like to only + // create one edge per unit. Thus, use a BTreeSet here to + // de-duplicate the producing units. + let other_units: BTreeSet<_> = self + .api_producers(client_pkg) + .map(|other_component| { + self.server_component_units + .get(other_component) + .unwrap() + }) + .collect(); + for other_unit in other_units { + let other_node = nodes.get(other_unit).unwrap(); + graph.update_edge( + *my_node, + *other_node, + client_pkg.clone(), + ); + } } } } @@ -414,9 +438,10 @@ impl SystemApis { } } - let other_component = self.api_producer(client_pkg).unwrap(); - let other_node = nodes.get(other_component).unwrap(); - graph.add_edge(*my_node, *other_node, client_pkg); + for other_component in self.api_producers(client_pkg) { + let other_node = nodes.get(other_component).unwrap(); + graph.add_edge(*my_node, *other_node, client_pkg); + } } } @@ -526,105 +551,106 @@ impl SystemApis { } continue; } - let producer = self.api_producer(&api.client_package_name).unwrap(); - let apis_consumed: BTreeSet<_> = self - .component_apis_consumed(producer, filter)? - .map(|(client_pkgname, _dep_path)| client_pkgname) - .collect(); - let dependents: BTreeSet<_> = self - .api_consumers(&api.client_package_name, filter) - .unwrap() - .map(|(dependent, _dep_path)| dependent) - .collect(); - - if api.versioned_how == VersionedHow::Unknown { - // If we haven't determined how to manage versioning on this - // API, and it has no dependencies on "unknown" or - // client-managed APIs, then it can be made server-managed. - if !apis_consumed.iter().any(|client_pkgname| { - let api = self - .api_metadata - .client_pkgname_lookup(*client_pkgname) - .unwrap(); - api.versioned_how != VersionedHow::Server - }) { - dag_check.propose_server( - &api.client_package_name, - String::from( - "has no unknown or client-managed dependencies", - ), - ); - } else if apis_consumed.contains(&api.client_package_name) { - // If this thing depends on itself, it must be - // client-managed. - dag_check.propose_client( - &api.client_package_name, - String::from("depends on itself"), - ); - } else if dependents.is_empty() { - // If something has no consumers in deployed components, it - // can be server-managed. (These are generally debug APIs.) - dag_check.propose_server( - &api.client_package_name, - String::from( - "has no consumers among deployed components", - ), - ); - } + for producer in self.api_producers(&api.client_package_name) { + let apis_consumed: BTreeSet<_> = self + .component_apis_consumed(producer, filter)? + .map(|(client_pkgname, _dep_path)| client_pkgname) + .collect(); + let dependents: BTreeSet<_> = self + .api_consumers(&api.client_package_name, filter) + .unwrap() + .map(|(dependent, _dep_path)| dependent) + .collect(); - continue; - } - - let dependencies: BTreeMap<_, _> = apis_consumed - .iter() - .map(|dependency_clientpkg| { - ( - self.api_producer(dependency_clientpkg).unwrap(), - *dependency_clientpkg, - ) - }) - .collect(); - - // Look for one-step circular dependencies (i.e., API API A1 is - // produced by component C1, which uses API A2 produced by C2, which - // also uses A1). In such cases, either A1 or A2 must be - // client-managed (or both). - for other_pkgname in dependents { - if let Some(dependency_clientpkg) = - dependencies.get(other_pkgname) - { - let dependency_api = self - .api_metadata - .client_pkgname_lookup(*dependency_clientpkg) - .unwrap(); - - // If we're looking at a server-managed dependency and the - // other is unknown, then that one should be client-managed. - // - // Without loss of generality, we can ignore the reverse - // case (because we will catch that case when we're - // iterating over the dependency API). - if api.versioned_how == VersionedHow::Server - && dependency_api.versioned_how == VersionedHow::Unknown - { + if api.versioned_how == VersionedHow::Unknown { + // If we haven't determined how to manage versioning on this + // API, and it has no dependencies on "unknown" or + // client-managed APIs, then it can be made server-managed. + if !apis_consumed.iter().any(|client_pkgname| { + let api = self + .api_metadata + .client_pkgname_lookup(*client_pkgname) + .unwrap(); + api.versioned_how != VersionedHow::Server + }) { + dag_check.propose_server( + &api.client_package_name, + String::from( + "has no unknown or client-managed dependencies", + ), + ); + } else if apis_consumed.contains(&api.client_package_name) { + // If this thing depends on itself, it must be + // client-managed. dag_check.propose_client( - dependency_clientpkg, - format!( - "has cyclic dependency on {:?}, which is \ - server-managed", - api.client_package_name, + &api.client_package_name, + String::from("depends on itself"), + ); + } else if dependents.is_empty() { + // If something has no consumers in deployed components, it + // can be server-managed. (These are generally debug APIs.) + dag_check.propose_server( + &api.client_package_name, + String::from( + "has no consumers among deployed components", ), - ) + ); } - // If both are Unknown, tell the user to pick one. - if api.versioned_how == VersionedHow::Unknown - && dependency_api.versioned_how == VersionedHow::Unknown + continue; + } + + let dependencies: BTreeMap<_, _> = apis_consumed + .iter() + .flat_map(|dependency_clientpkg| { + self.api_producers(dependency_clientpkg) + .map(|p| (p, *dependency_clientpkg)) + }) + .collect(); + + // Look for one-step circular dependencies (i.e., API API A1 is + // produced by component C1, which uses API A2 produced by C2, which + // also uses A1). In such cases, either A1 or A2 must be + // client-managed (or both). + for other_pkgname in dependents { + if let Some(dependency_clientpkg) = + dependencies.get(other_pkgname) { - dag_check.propose_upick( - &api.client_package_name, - dependency_clientpkg, - ); + let dependency_api = self + .api_metadata + .client_pkgname_lookup(*dependency_clientpkg) + .unwrap(); + + // If we're looking at a server-managed dependency and the + // other is unknown, then that one should be client-managed. + // + // Without loss of generality, we can ignore the reverse + // case (because we will catch that case when we're + // iterating over the dependency API). + if api.versioned_how == VersionedHow::Server + && dependency_api.versioned_how + == VersionedHow::Unknown + { + dag_check.propose_client( + dependency_clientpkg, + format!( + "has cyclic dependency on {:?}, which is \ + server-managed", + api.client_package_name, + ), + ) + } + + // If both are Unknown, tell the user to pick one. + if api.versioned_how == VersionedHow::Unknown + && dependency_api.versioned_how + == VersionedHow::Unknown + { + dag_check.propose_upick( + &api.client_package_name, + dependency_clientpkg, + ); + } } } } @@ -758,11 +784,10 @@ struct ServerComponentsTracker<'a> { &'a BTreeMap>, // outputs (structures that we're building up) - errors: Vec, server_component_units: BTreeMap, unit_server_components: BTreeMap>, - api_producers: BTreeMap, + api_producers: BTreeMap, } impl<'a> ServerComponentsTracker<'a> { @@ -774,7 +799,6 @@ impl<'a> ServerComponentsTracker<'a> { ) -> ServerComponentsTracker<'a> { ServerComponentsTracker { known_server_packages, - errors: Vec::new(), server_component_units: BTreeMap::new(), unit_server_components: BTreeMap::new(), api_producers: BTreeMap::new(), @@ -826,19 +850,12 @@ impl<'a> ServerComponentsTracker<'a> { return; } - if let Some((previous, _)) = self.api_producers.insert( - api.client_package_name.clone(), - (server_pkgname.clone(), dep_path.clone()), - ) { - self.errors.push(anyhow!( - "API for client {} appears to be exported by multiple \ - components: at least {} and {} ({:?})", - api.client_package_name, - previous, - server_pkgname, - dep_path - )); - } + self.api_producers + .entry(api.client_package_name.clone()) + .or_default() + .entry(server_pkgname.clone()) + .or_default() + .push(dep_path.clone()); } /// Record that deployment unit package `dunit_pkgname` depends on package