From 7875278a2b1cd0be0b2fe72b3810f8d1d26abd60 Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Thu, 20 Jul 2023 16:43:17 -0400 Subject: [PATCH] don't suppress unpublished examples (#2855) ## Motivation and Context The publish tool doesn't consider unpublished crates when computing the version tree for publish. This causes errors for examples that use shared crates. ## Description This adds support for considering unpublished crates when publishing. It allows unpublished crates to have unpublished dependencies but published crates can only have unpublished crates as dev dependencies. ## Testing - [x] build these exact changes into the docker image and test ## Checklist - [ ] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --- .../publisher/src/subcommand/fix_manifests.rs | 211 +++++++++++++----- .../src/subcommand/fix_manifests/validate.rs | 20 +- 2 files changed, 175 insertions(+), 56 deletions(-) diff --git a/tools/ci-build/publisher/src/subcommand/fix_manifests.rs b/tools/ci-build/publisher/src/subcommand/fix_manifests.rs index 910f5fd51..2d34c1aba 100644 --- a/tools/ci-build/publisher/src/subcommand/fix_manifests.rs +++ b/tools/ci-build/publisher/src/subcommand/fix_manifests.rs @@ -21,7 +21,7 @@ use std::ffi::OsStr; use std::path::{Path, PathBuf}; use toml::value::Table; use toml::Value; -use tracing::info; +use tracing::{debug, info}; mod validate; @@ -72,6 +72,63 @@ struct Manifest { metadata: toml::Value, } +impl Manifest { + /// Returns the `publish` setting for a given crate + fn publish(&self) -> Result { + let value = self.metadata.get("package").and_then(|v| v.get("publish")); + match value { + None => Ok(true), + Some(value) => value + .as_bool() + .ok_or(anyhow::Error::msg("unexpected publish setting")), + } + } +} + +struct Versions(BTreeMap); +#[derive(Copy, Clone)] +enum FilterType { + AllCrates, + PublishedOnly, +} +struct VersionView<'a>(&'a Versions, FilterType); +impl VersionView<'_> { + fn get(&self, crate_name: &str) -> Option<&Version> { + let version = match (self.1, self.0 .0.get(crate_name)) { + (FilterType::AllCrates, version) => version, + (FilterType::PublishedOnly, v @ Some(VersionWithMetadata { publish: true, .. })) => v, + _ => None, + }; + version.map(|v| &v.version) + } + + fn all_crates(&self) -> Self { + VersionView(self.0, FilterType::AllCrates) + } +} + +impl Versions { + fn published(&self) -> VersionView { + VersionView(self, FilterType::PublishedOnly) + } + + fn published_crates(&self) -> impl Iterator + '_ { + self.0 + .iter() + .filter(|(_, v)| v.publish) + .map(|(k, v)| (k.as_str(), &v.version)) + } + + fn get(&self, crate_name: &str) -> Option<&Version> { + self.0.get(crate_name).map(|v| &v.version) + } +} + +struct VersionWithMetadata { + version: Version, + publish: bool, +} + async fn read_manifests(fs: Fs, manifest_paths: Vec) -> Result> { let mut result = Vec::new(); for path in manifest_paths { @@ -84,7 +141,7 @@ async fn read_manifests(fs: Fs, manifest_paths: Vec) -> Result Result> { +fn package_versions(manifests: &[Manifest]) -> Result { let mut versions = BTreeMap::new(); for manifest in manifests { // ignore workspace manifests @@ -92,15 +149,7 @@ fn package_versions(manifests: &[Manifest]) -> Result> Some(package) => package, None => continue, }; - // ignore non-publishable crates - if let Some(Value::Boolean(false)) = manifest - .metadata - .get("package") - .expect("checked above") - .get("publish") - { - continue; - } + let publish = manifest.publish()?; let name = package .get("name") .and_then(|name| name.as_str()) @@ -114,16 +163,12 @@ fn package_versions(manifests: &[Manifest]) -> Result> anyhow::Error::msg(format!("{:?} is missing a package version", manifest.path)) })?; let version = parse_version(&manifest.path, version)?; - versions.insert(name.into(), version); + versions.insert(name.into(), VersionWithMetadata { version, publish }); } - Ok(versions) + Ok(Versions(versions)) } -fn fix_dep_set( - versions: &BTreeMap, - key: &str, - metadata: &mut toml::Value, -) -> Result { +fn fix_dep_set(versions: &VersionView, key: &str, metadata: &mut Value) -> Result { let mut changed = 0; if let Some(dependencies) = metadata.as_table_mut().unwrap().get_mut(key) { if let Some(dependencies) = dependencies.as_table_mut() { @@ -143,11 +188,7 @@ fn fix_dep_set( Ok(changed) } -fn update_dep( - table: &mut Table, - dep_name: &str, - versions: &BTreeMap, -) -> Result { +fn update_dep(table: &mut Table, dep_name: &str, versions: &VersionView) -> Result { if !table.contains_key("path") { return Ok(0); } @@ -169,9 +210,10 @@ fn update_dep( } } -fn fix_dep_sets(versions: &BTreeMap, metadata: &mut toml::Value) -> Result { +fn fix_dep_sets(versions: &VersionView, metadata: &mut toml::Value) -> Result { let mut changed = fix_dep_set(versions, "dependencies", metadata)?; - changed += fix_dep_set(versions, "dev-dependencies", metadata)?; + // allow dev dependencies to be unpublished + changed += fix_dep_set(&versions.all_crates(), "dev-dependencies", metadata)?; changed += fix_dep_set(versions, "build-dependencies", metadata)?; Ok(changed) } @@ -202,45 +244,53 @@ fn conditionally_disallow_publish( // is not being run from CI, and disallow publish in that case. Also disallow // publishing of examples. if !is_github_actions || is_example { - if let Some(package) = metadata.as_table_mut().unwrap().get_mut("package") { - info!( - "Detected {}. Disallowing publish for {:?}.", - if is_example { "example" } else { "local build" }, - manifest_path, - ); - package - .as_table_mut() - .unwrap() - .insert("publish".into(), toml::Value::Boolean(false)); - return Ok(true); + if let Some(value) = set_publish_false(manifest_path, metadata, is_example) { + return Ok(value); } } Ok(false) } +fn set_publish_false(manifest_path: &Path, metadata: &mut Value, is_example: bool) -> Option { + if let Some(package) = metadata.as_table_mut().unwrap().get_mut("package") { + info!( + "Detected {}. Disallowing publish for {:?}.", + if is_example { "example" } else { "local build" }, + manifest_path, + ); + package + .as_table_mut() + .unwrap() + .insert("publish".into(), toml::Value::Boolean(false)); + return Some(true); + } + None +} + async fn fix_manifests( fs: Fs, - versions: &BTreeMap, + versions: &Versions, manifests: &mut Vec, mode: Mode, ) -> Result<()> { for manifest in manifests { let package_changed = conditionally_disallow_publish(&manifest.path, &mut manifest.metadata)?; - let dependencies_changed = fix_dep_sets(versions, &mut manifest.metadata)?; - if package_changed || dependencies_changed > 0 { + let num_deps_changed = fix_manifest(versions, manifest)?; + if package_changed || num_deps_changed > 0 { let contents = "# Code generated by software.amazon.smithy.rust.codegen.smithy-rs. DO NOT EDIT.\n" .to_string() + &toml::to_string(&manifest.metadata).with_context(|| { format!("failed to serialize to toml for {:?}", manifest.path) })?; + match mode { Mode::Execute => { fs.write_file(&manifest.path, contents.as_bytes()).await?; info!( "Changed {} dependencies in {:?}.", - dependencies_changed, manifest.path + num_deps_changed, manifest.path ); } Mode::Check => { @@ -255,10 +305,73 @@ async fn fix_manifests( Ok(()) } +fn fix_manifest(versions: &Versions, manifest: &mut Manifest) -> Result { + let mut view = versions.published(); + if !manifest.publish()? { + debug!(package = ?&manifest.path, "package has publishing disabled, allowing unpublished crates to be used"); + view = view.all_crates(); + } + fix_dep_sets(&view, &mut manifest.metadata) +} + #[cfg(test)] mod tests { use super::*; + fn make_versions<'a>(versions: impl Iterator) -> Versions { + let map = versions + .into_iter() + .map(|(name, version, publish)| { + let publish = *publish; + ( + name.to_string(), + VersionWithMetadata { + version: Version::parse(&version).unwrap(), + publish, + }, + ) + }) + .collect::>(); + + Versions(map) + } + + #[test] + fn unpublished_deps_cant_be_deps() { + let manifest = br#" + [package] + name = "test" + version = "1.2.0" + + [build-dependencies] + build_something = "1.3" + local_build_something = { path = "../local_build_something", version = "0.4.0-different" } + + [dev-dependencies] + dev_something = "1.1" + local_dev_something = { path = "../local_dev_something" } + + [dependencies] + something = "1.0" + local_something = { path = "../local_something" } + "#; + let metadata = toml::from_slice(manifest).unwrap(); + let mut manifest = Manifest { + path: "test".into(), + metadata, + }; + let versions = &[ + ("local_build_something", "0.2.0", true), + ("local_dev_something", "0.1.0", false), + ("local_something", "1.1.3", false), + ]; + let versions = make_versions(versions.iter()); + fix_manifest(&versions, &mut manifest).expect_err("depends on unpublished local something"); + set_publish_false(&manifest.path, &mut manifest.metadata, false).unwrap(); + fix_manifest(&versions, &mut manifest) + .expect("now it will work, the crate isn't published"); + } + #[test] fn test_fix_dep_sets() { let manifest = br#" @@ -283,16 +396,14 @@ mod tests { path: "test".into(), metadata, }; - let versions = vec![ - ("local_build_something", "0.2.0"), - ("local_dev_something", "0.1.0"), - ("local_something", "1.1.3"), - ] - .into_iter() - .map(|e| (e.0.to_string(), Version::parse(e.1).unwrap())) - .collect(); - - fix_dep_sets(&versions, &mut manifest.metadata).expect("success"); + let versions = &[ + ("local_build_something", "0.2.0", true), + ("local_dev_something", "0.1.0", false), + ("local_something", "1.1.3", true), + ]; + let versions = make_versions(versions.iter()); + + fix_dep_sets(&versions.published(), &mut manifest.metadata).expect("success"); let actual_deps = &manifest.metadata["dependencies"]; assert_eq!( diff --git a/tools/ci-build/publisher/src/subcommand/fix_manifests/validate.rs b/tools/ci-build/publisher/src/subcommand/fix_manifests/validate.rs index 53202468f..40aa8f609 100644 --- a/tools/ci-build/publisher/src/subcommand/fix_manifests/validate.rs +++ b/tools/ci-build/publisher/src/subcommand/fix_manifests/validate.rs @@ -5,10 +5,10 @@ use crate::fs::Fs; use crate::package::discover_and_validate_package_batches; +use crate::subcommand::fix_manifests::Versions; use anyhow::{anyhow, bail, Result}; use semver::Version; use smithy_rs_tool_common::package::PackageCategory; -use std::collections::BTreeMap; use std::path::Path; use tracing::info; @@ -17,7 +17,7 @@ use tracing::info; /// For now, this validates: /// - `aws-smithy-` prefixed versions match `aws-` (NOT `aws-sdk-`) prefixed versions pub(super) fn validate_before_fixes( - versions: &BTreeMap, + versions: &Versions, disable_version_number_validation: bool, ) -> Result<()> { // Later when we only generate independently versioned SDK crates, this flag can become permanent. @@ -30,7 +30,7 @@ pub(super) fn validate_before_fixes( .get("aws-smithy-types") .ok_or_else(|| anyhow!("`aws-smithy-types` crate missing"))?; - for (name, version) in versions { + for (name, version) in versions.published_crates() { let category = PackageCategory::from_package_name(name); if category == PackageCategory::SmithyRuntime || category == PackageCategory::AwsRuntime { confirm_version(name, expected_runtime_version, version)?; @@ -63,14 +63,22 @@ pub(super) async fn validate_after_fixes(location: &Path) -> Result<()> { #[cfg(test)] mod test { use super::*; + use crate::subcommand::fix_manifests::VersionWithMetadata; + use std::collections::BTreeMap; use std::str::FromStr; - fn versions(versions: &[(&'static str, &'static str)]) -> BTreeMap { + fn versions(versions: &[(&'static str, &'static str)]) -> Versions { let mut map = BTreeMap::new(); for (name, version) in versions { - map.insert((*name).into(), Version::from_str(version).unwrap()); + map.insert( + (*name).into(), + VersionWithMetadata { + version: Version::from_str(version).unwrap(), + publish: true, + }, + ); } - map + Versions(map) } #[track_caller] -- GitLab