Unverified Commit 7875278a authored by Russell Cohen's avatar Russell Cohen Committed by GitHub
Browse files

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
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] 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._
parent a57a719c

tools/ci-build/publisher/src/subcommand/fix_manifests.rs

+161 −50
Original line number Diff line number Diff line
@@ -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<bool> {

        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<String, VersionWithMetadata>);

#[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<Item = (&str, &Version)> + '_ {

        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<PathBuf>) -> Result<Vec<Manifest>> {

    let mut result = Vec::new();

    for path in manifest_paths {
@@ -84,7 +141,7 @@ async fn read_manifests(fs: Fs, manifest_paths: Vec<PathBuf>) -> Result<Vec<Mani 
}



/// Returns a map of crate name to semver version number

fn package_versions(manifests: &[Manifest]) -> Result<BTreeMap<String, Version>> {

fn package_versions(manifests: &[Manifest]) -> Result<Versions> {

    let mut versions = BTreeMap::new();

    for manifest in manifests {

        // ignore workspace manifests
@@ -92,15 +149,7 @@ fn package_versions(manifests: &[Manifest]) -> Result<BTreeMap<String, Version>> 
            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<BTreeMap<String, Version>> 
                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<String, Version>,

    key: &str,

    metadata: &mut toml::Value,

) -> Result<usize> {

fn fix_dep_set(versions: &VersionView, key: &str, metadata: &mut Value) -> Result<usize> {

    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<String, Version>,

) -> Result<usize> {

fn update_dep(table: &mut Table, dep_name: &str, versions: &VersionView) -> Result<usize> {

    if !table.contains_key("path") {

        return Ok(0);

    }
@@ -169,9 +210,10 @@ fn update_dep( 
    }

}



fn fix_dep_sets(versions: &BTreeMap<String, Version>, metadata: &mut toml::Value) -> Result<usize> {

fn fix_dep_sets(versions: &VersionView, metadata: &mut toml::Value) -> Result<usize> {

    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,6 +244,14 @@ 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(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<bool> {

    if let Some(package) = metadata.as_table_mut().unwrap().get_mut("package") {

        info!(

            "Detected {}. Disallowing publish for {:?}.",
@@ -212,35 +262,35 @@ fn conditionally_disallow_publish( 
            .as_table_mut()

            .unwrap()

            .insert("publish".into(), toml::Value::Boolean(false));

            return Ok(true);

        return Some(true);

    }

    }

    Ok(false)

    None

}



async fn fix_manifests(

    fs: Fs,

    versions: &BTreeMap<String, Version>,

    versions: &Versions,

    manifests: &mut Vec<Manifest>,

    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<usize> {

    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<Item = &'a (&'a str, &'a str, bool)>) -> Versions {

        let map = versions

            .into_iter()

            .map(|(name, version, publish)| {

                let publish = *publish;

                (

                    name.to_string(),

                    VersionWithMetadata {

                        version: Version::parse(&version).unwrap(),

                        publish,

                    },

                )

            })

            .collect::<BTreeMap<_, _>>();



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

        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, &mut manifest.metadata).expect("success");

        fix_dep_sets(&versions.published(), &mut manifest.metadata).expect("success");



        let actual_deps = &manifest.metadata["dependencies"];

        assert_eq!(

tools/ci-build/publisher/src/subcommand/fix_manifests/validate.rs

+14 −6
Original line number Diff line number Diff line
@@ -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<String, Version>,

    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<String, Version> {

    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]