From ec226c0223f56d45b51eb1ce7ad8195cc2f6db70 Mon Sep 17 00:00:00 2001 From: ysaito1001 Date: Tue, 17 Sep 2024 10:03:55 -0500 Subject: [PATCH] Address post-merge feedback on smithy-rs#3827 (#3834) ## Motivation and Context Primarily for @drganjoo providing feedback on smithy-rs#3827 (thanks), but anyone is welcome to review the changes. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --- buildSrc/src/main/kotlin/CrateSet.kt | 19 ++++--- tools/ci-build/sdk-lockfiles/Cargo.toml | 2 +- tools/ci-build/sdk-lockfiles/README.md | 38 ++++++++++---- tools/ci-build/sdk-lockfiles/src/audit.rs | 64 ++++++++++------------- 4 files changed, 68 insertions(+), 55 deletions(-) diff --git a/buildSrc/src/main/kotlin/CrateSet.kt b/buildSrc/src/main/kotlin/CrateSet.kt index 253bfa08ca..1924badc68 100644 --- a/buildSrc/src/main/kotlin/CrateSet.kt +++ b/buildSrc/src/main/kotlin/CrateSet.kt @@ -39,6 +39,8 @@ object CrateSet { } } + // If we make changes to `AWS_SDK_RUNTIME`, also update the list in + // https://github.com/smithy-lang/smithy-rs/blob/main/tools/ci-build/sdk-lockfiles/src/audit.rs#L22 val AWS_SDK_RUNTIME = listOf( "aws-config", @@ -79,13 +81,16 @@ object CrateSet { val AWS_SDK_SMITHY_RUNTIME = SMITHY_RUNTIME_COMMON - val SERVER_SMITHY_RUNTIME = - SMITHY_RUNTIME_COMMON + - listOf( - Crate("aws-smithy-http-server", UNSTABLE_VERSION_PROP_NAME), - Crate("aws-smithy-http-server-python", UNSTABLE_VERSION_PROP_NAME), - Crate("aws-smithy-http-server-typescript", UNSTABLE_VERSION_PROP_NAME), - ) + // If we make changes to `SERVER_SPECIFIC_SMITHY_RUNTIME`, also update the list in + // https://github.com/smithy-lang/smithy-rs/blob/main/tools/ci-build/sdk-lockfiles/src/audit.rs#L38 + private val SERVER_SPECIFIC_SMITHY_RUNTIME = + listOf( + Crate("aws-smithy-http-server", UNSTABLE_VERSION_PROP_NAME), + Crate("aws-smithy-http-server-python", UNSTABLE_VERSION_PROP_NAME), + Crate("aws-smithy-http-server-typescript", UNSTABLE_VERSION_PROP_NAME), + ) + + val SERVER_SMITHY_RUNTIME = SMITHY_RUNTIME_COMMON + SERVER_SPECIFIC_SMITHY_RUNTIME val ENTIRE_SMITHY_RUNTIME = (AWS_SDK_SMITHY_RUNTIME + SERVER_SMITHY_RUNTIME).toSortedSet(compareBy { it.name }) diff --git a/tools/ci-build/sdk-lockfiles/Cargo.toml b/tools/ci-build/sdk-lockfiles/Cargo.toml index d842d0129c..45aeada6fc 100644 --- a/tools/ci-build/sdk-lockfiles/Cargo.toml +++ b/tools/ci-build/sdk-lockfiles/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "sdk-lockfiles" -version = "0.1.0" +version = "0.1.1" authors = ["AWS Rust SDK Team "] description = """ A CLI tool to audit lockfiles for Smithy runtime crates, AWS runtime crates, `aws-config`, and the workspace containing diff --git a/tools/ci-build/sdk-lockfiles/README.md b/tools/ci-build/sdk-lockfiles/README.md index bb3325bf79..e83ad5ad5c 100644 --- a/tools/ci-build/sdk-lockfiles/README.md +++ b/tools/ci-build/sdk-lockfiles/README.md @@ -2,23 +2,41 @@ sdk-lockfiles ============= This CLI tool audits the `Cargo.lock` files in the `smithy-rs` repository. These lockfiles are used to ensure -reproducible builds. The `sdk-lockfiles` tool specifically audits the following lockfiles: -- The [lockfile](https://github.com/smithy-lang/smithy-rs/blob/main/rust-runtime/Cargo.lock) for Smithy runtime crates -- The [lockfile](https://github.com/smithy-lang/smithy-rs/blob/main/aws/rust-runtime/Cargo.lock) for AWS runtime crates -- The [lockfile](https://github.com/smithy-lang/smithy-rs/blob/main/aws/rust-runtime/aws-config/Cargo.lock) for the `aws-config` crate -- The [lockfile](https://github.com/smithy-lang/smithy-rs/blob/main/aws/sdk/Cargo.lock) for the workspace containing code-generated AWS SDK crates (*) +reproducible builds during our release process for both `smithy-rs` and `aws-sdk-rust`. When a crate dependency is not +pinned to a fixed version, it risks being affected by newer versions of that dependency published to crates.io, which +could potentially break the build. -Specifically, the tool ensures that the lockfile marked with (*) is a superset containing all dependencies listed in -the rest of the runtime lockfiles. If it detects a new dependency in the AWS SDK crates introduced by any of the runtime -lockfiles (unless the dependency is introduced by a server runtime crate), it will output a message similar to the -following: +We track the following lockfiles in the `smithy-rs` repository: +1. The [lockfile](https://github.com/smithy-lang/smithy-rs/blob/main/rust-runtime/Cargo.lock) for Smithy runtime crates +2. The [lockfile](https://github.com/smithy-lang/smithy-rs/blob/main/aws/rust-runtime/Cargo.lock) for AWS runtime crates +3. The [lockfile](https://github.com/smithy-lang/smithy-rs/blob/main/aws/rust-runtime/aws-config/Cargo.lock) for the `aws-config` crate +4. The [lockfile](https://github.com/smithy-lang/smithy-rs/blob/main/aws/sdk/Cargo.lock) for the workspace containing code-generated AWS SDK crates + +The first three lockfiles can be easily updated during development with a `cargo` command. However, the fourth lockfile +, known as the SDK lockfile, is generated by the code generator and is not checked into to the `smithy-rs` repository as +frequently as the first three runtime lockfiles. As a result, new dependencies added to any of the runtime lockfiles may +not be reflected in the SDK lockfile. + +The `sdk-lockfiles` tool ensures that the SDK lockfile is a superset containing all dependencies listed in the three +runtime lockfiles. If it detects a new dependency in the AWS SDK crates introduced by any of the runtime lockfiles it +will output a message similar to the following (unless the dependency is introduced by a server specific runtime crate): ``` $ sdk-lockfiles audit 2024-09-10T16:48:38.460518Z INFO sdk_lockfiles::audit: checking whether `rust-runtime/Cargo.lock` is covered by the SDK lockfile... 2024-09-10T16:48:38.489879Z INFO sdk_lockfiles::audit: checking whether `aws/rust-runtime/Cargo.lock` is covered by the SDK lockfile... 2024-09-10T16:48:38.490306Z INFO sdk_lockfiles::audit: checking whether `aws/rust-runtime/aws-config/Cargo.lock` is covered by the SDK lockfile... -`minicbor` (0.24.2), used by `rust-runtime/Cargo.lock`, is not contained in SDK lockfile! +`minicbor` (0.24.2), used by `rust-runtime/Cargo.lock`, is not contained in the SDK lockfile! Error: there are lockfile audit failures ``` This tool is intended for automated use. + +## Limitation +The `sdk-lockfiles` tool does not verify whether new dependencies introduced in [CargoDependency.kt](https://github.com/smithy-lang/smithy-rs/blob/main/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/CargoDependency.kt) +are included in the SDK lockfile. This is because dependencies in `CargoDependency.kt` are represented as a Kotlin data +class. Consequently, dependencies added via the code generator, `inlineable`, or `aws-inlineable` are not considered by +`sdk-lockfiles`. + +This limitation is acceptable for our operational purposes. Our release script always executes +`./gradlew aws:sdk:syncAwsSdkLockfile`, which ensures that any dependencies added in `CargoDependency.kt` are properly +reflected in the SDK lockfile. diff --git a/tools/ci-build/sdk-lockfiles/src/audit.rs b/tools/ci-build/sdk-lockfiles/src/audit.rs index 008f59990b..c47a0a2fe5 100644 --- a/tools/ci-build/sdk-lockfiles/src/audit.rs +++ b/tools/ci-build/sdk-lockfiles/src/audit.rs @@ -16,16 +16,14 @@ use std::env; use std::iter; use std::path::PathBuf; -// A list of AWS runtime crate must be in sync with +// A list of the names of AWS runtime crates (crate versions do not need to match) must be in sync with // https://github.com/smithy-lang/smithy-rs/blob/0f9b9aba386ea3063912a0464ba6a1fd7c596018/buildSrc/src/main/kotlin/CrateSet.kt#L42-L53 -// plus `aws-inlineable` const AWS_SDK_RUNTIMES: &[&str] = &[ "aws-config", "aws-credential-types", "aws-endpoint", "aws-http", "aws-hyper", - "aws-inlineable", "aws-runtime", "aws-runtime-api", "aws-sig-auth", @@ -33,8 +31,8 @@ const AWS_SDK_RUNTIMES: &[&str] = &[ "aws-types", ]; -// A list of server runtime crates must be in sync with -// https://github.com/smithy-lang/smithy-rs/blob/0f9b9aba386ea3063912a0464ba6a1fd7c596018/buildSrc/src/main/kotlin/CrateSet.kt#L85-L87 +// A list of the names of server specific runtime crates (crate versions do not need to match) must be in sync with +// https://github.com/smithy-lang/smithy-rs/blob/main/buildSrc/src/main/kotlin/CrateSet.kt#L42 const SERVER_SPECIFIC_RUNTIMES: &[&str] = &[ "aws-smithy-http-server", "aws-smithy-http-server-python", @@ -43,19 +41,22 @@ const SERVER_SPECIFIC_RUNTIMES: &[&str] = &[ fn new_dependency_for_aws_sdk(crate_name: &str) -> bool { AWS_SDK_RUNTIMES.contains(&crate_name) - || crate_name == "inlineable" || (crate_name.starts_with("aws-smithy-") && !SERVER_SPECIFIC_RUNTIMES.contains(&crate_name)) } // Recursively traverses a chain of dependencies originating from a potential new dependency. Returns true as soon as // it encounters a crate name that matches a runtime crate used by the AWS SDK. -fn visit(graph: &Graph, node_index: NodeIndex, visited: &mut BTreeSet) -> bool { +fn is_consumed_by_aws_sdk( + graph: &Graph, + node_index: NodeIndex, + visited: &mut BTreeSet, +) -> bool { if !visited.insert(node_index) { return false; } - let dependencies = graph + let consumers = graph .edges_directed( node_index, cargo_lock::dependency::graph::EdgeDirection::Incoming, @@ -63,14 +64,14 @@ fn visit(graph: &Graph, node_index: NodeIndex, visited: &mut BTreeSet .map(|edge| edge.source()) .collect::>(); - for dependency_node_index in dependencies.iter() { - let package = &graph[*dependency_node_index]; + for consumer_node_index in consumers.iter() { + let package = &graph[*consumer_node_index]; tracing::debug!("visiting `{}`", package.name.as_str()); if new_dependency_for_aws_sdk(package.name.as_str()) { tracing::debug!("it's a new dependency for the AWS SDK!"); return true; } - if visit(graph, *dependency_node_index, visited) { + if is_consumed_by_aws_sdk(graph, *consumer_node_index, visited) { return true; } } @@ -88,22 +89,17 @@ fn new_dependency(lockfile: &Lockfile, target: &str) -> bool { target ); let tree = lockfile.dependency_tree().unwrap(); - let indices: Vec<_> = [target.to_owned()] + let package = lockfile + .packages .iter() - .map(|dep| { - let package = lockfile - .packages - .iter() - .find(|pkg| pkg.name.as_str() == dep) - .unwrap(); - tree.nodes()[&package.into()] - }) - .collect(); + .find(|pkg| pkg.name.as_str() == target) + .expect("{target} must be in dependencies listed in `lockfile`"); + let indices = vec![tree.nodes()[&package.into()]]; for index in &indices { let mut visited: BTreeSet = BTreeSet::new(); tracing::debug!("traversing a dependency chain for `{}`...", target); - if visit(tree.graph(), *index, &mut visited) { + if is_consumed_by_aws_sdk(tree.graph(), *index, &mut visited) { return true; } } @@ -307,17 +303,17 @@ dependencies = [ ] [[package]] -name = "inlineable" -version = "0.1.0" +name = "aws-smithy-compression" +version = "0.0.1" dependencies = [ - "md-5" + "flate2" ] [[package]] -name = "md-5" -version = "0.10.6" +name = "flate2" +version = "1.0.33" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d89e7ee0cfbedfc4da3340218492196241d89eefb6dab27de5df917a6d2e78cf" +checksum = "324a1be68054ef05ad64b861cc9eaf1d623d2d8cb25b4bf2cb9cdd902b4bf253" [[package]] name = "minicbor" @@ -329,7 +325,7 @@ checksum = "5f8e213c36148d828083ae01948eed271d03f95f7e72571fa242d78184029af2" .unwrap(); assert_eq!( - vec!["md-5", "minicbor"], + vec!["flate2", "minicbor"], audit_runtime_lockfile_covered_by_sdk_lockfile( &runtime_lockfile, &sdk_dependency_set(), @@ -351,19 +347,13 @@ dependencies = [ "zeroize", ] -[[package]] -name = "aws-inlineable" -version = "0.1.0" -dependencies = [ - "ahash", - "lru" -] - [[package]] name = "aws-sigv4" version = "1.2.3" dependencies = [ + "ahash", "aws-credential-types", + "lru", "p256", ]