Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Separate Registry CLI tools from AWSSDKSwiftCLI #1620

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

jbelkins
Copy link
Contributor

@jbelkins jbelkins commented Jul 10, 2024

Issue #

#1621

Description of changes

Having the package manifest generator, located in AWSSDKSwiftCLI, depend on the SDK itself has proven problematic:

  • The AWS_SWIFT_SDK_USE_LOCAL_DEPS env var, which must be set in most situations in development & CI, prevents the AWSSDKSwiftCLI package from building with stable aws-sdk-swift as a dependency.
  • Using the local copy of aws-sdk-swift causes problems when AWSSDKSwiftCLI is run to update the manifest, but the manifest being updated is also part of the code being built.

The problem is addressed by separating the Registry CLI tools into a separate Swift package, allowing the aws-sdk-swift dependency to be removed from AWSSDKSwiftCLI. AWSSDKSwiftCLI publishes its reusable AWSCLIUtils as a library, so it may be used by the Registry CLI as a dependency.

New/existing dependencies impact assessment, if applicable

No new dependencies were added to this change.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jbelkins jbelkins changed the title chore: Use local aws-sdk-swift with SwiftCLI tools chore: Separate Registry CLI tools from AWSSDKSwiftCLI Jul 10, 2024
@jbelkins jbelkins marked this pull request as ready for review July 10, 2024 02:46
@@ -7,12 +7,14 @@ let package = Package(
platforms: [
.macOS(.v13)
],
products: [
.library(name: "AWSCLIUtils", targets: ["AWSCLIUtils"]),
],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This product is published so AWSCLIUtils can be shared with other packages (like the new SPRCLI package.)

.product(name: "AWSS3", package: "aws-sdk-swift"),
.product(name: "AWSCloudFront", package: "aws-sdk-swift"),
]
),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the SPR targets in AWSSDKSwiftCLI, along with the SDK dependency, are moved to the new SPRCLI package defined below.

dependencies: [
.package(url: "https://github.com/apple/swift-argument-parser", from: "1.2.0"),
.package(url: "https://github.com/awslabs/aws-sdk-swift", from: "0.46.0"),
.package(path: "../AWSSDKSwiftCLI"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AWSCLIUtils are accessed by adding the local copy of AWSSDKSwiftCLI as a dependency.

.product(name: "AWSS3", package: "aws-sdk-swift"),
.product(name: "AWSCloudFront", package: "aws-sdk-swift"),
]
),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three targets above were copied in from AWSSDKSwiftCLI.

@@ -12,7 +12,6 @@ rm -rf Tests/Services/*
./gradlew --stop

# Regenerate the SDK Package.swift with all services
unset AWS_SWIFT_SDK_USE_LOCAL_DEPS
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Un-setting this env var is no longer needed when building AWSSDKSwiftCLI since it does not depend on the SDK anymore.

@jbelkins jbelkins merged commit 764400b into main Jul 10, 2024
29 checks passed
@jbelkins jbelkins deleted the jbe/cli_tools_use_local_sdk branch July 10, 2024 02:55
@sichanyoo sichanyoo self-requested a review July 10, 2024 16:21
Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants