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

feat: Add Swift Package Registry tools and config #771

Merged
merged 19 commits into from
Jul 9, 2024
Merged

feat: Add Swift Package Registry tools and config #771

merged 19 commits into from
Jul 9, 2024

Conversation

jbelkins
Copy link
Contributor

@jbelkins jbelkins commented Jun 28, 2024

Issue #

Description of changes

  • Update SwiftDependency to be able to represent dependencies hosted on Swift Package Registry
  • Update the PackageManifestGenerator to write dependencies hosted on Swift Package Registry

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

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

val dependenciesByURL = externalDependencies.distinctBy {
it.getProperty("url", String::class.java).getOrNull()
?: "${it.getProperty("scope", String::class.java).get()}.${it.packageName}"
}
Copy link
Contributor Author

@jbelkins jbelkins Jun 29, 2024

Choose a reason for hiding this comment

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

"External dependencies" are all the dependencies of this package that aren't built-in (i.e. Swift or Foundation.)
"Dependencies by URL" are written to the package dependencies section.

}
}
}
}
}
}

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 two private methods below just write a package or target dependency from a SymbolDependency, and are used in the appropriate sections above.

) : Dependency {

enum class DistributionMethod {
SPR, GIT
}
Copy link
Contributor Author

@jbelkins jbelkins Jun 29, 2024

Choose a reason for hiding this comment

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

Each SwiftDependency now has an enum value indicating how it is hosted, although this value is ignored if the "location" property for the SwiftDependency is empty.

val CRT = SwiftDependency(
"AwsCommonRuntimeKit",
null,
"0.30.0",
"https://github.com/awslabs/aws-crt-swift",
"",
"aws-crt-swift",
DistributionMethod.GIT,
)
val CLIENT_RUNTIME = SwiftDependency(
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 & the other SwiftDependencies below are updated for hosting on the package registry.

.putProperty("target", target)
.putProperty("branch", branch)
.putProperty("localPath", localPath)
.packageName(packageName)
.version(version)
.putProperty("url", url)
.build()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If distributionMethod is git then location contains the Git URL.
If it is SPR then location is the package scope.

These properties are written to the SymbolDependency under the appropriate keys.

val isPackageManifest = listOf(PACKAGE_MANIFEST_NAME, (PACKAGE_MANIFEST_NAME + ".swift")).contains(fullPackageName)
val isNonSwiftSourceFile = listOf(".json", ".version").any { fullPackageName.endsWith(it) }
val noHeader = isPackageManifest || isNonSwiftSourceFile
return contents.takeIf { noHeader } ?: (GENERATED_FILE_HEADER + imports + contents)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are now using the SwiftWriter to write non-Swift content of a couple different kinds, we added more logic to leave out the headers for those files.

@@ -6,12 +6,15 @@ package software.amazon.smithy.swift.codegen

import software.amazon.smithy.codegen.core.SymbolDependency
import software.amazon.smithy.swift.codegen.integration.ProtocolGenerator
import kotlin.jvm.optionals.getOrNull

val PACKAGE_MANIFEST_NAME = "Package.swift.txt"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Package.swift is temporarily renamed so that Xcode does not try to interpret each service client as its own Swift package just yet.

dependenciesByTarget.forEach { writeTargetDependency(writer, it) }
}
writer.openBlock("resources: [", "]") {
writer.write(".process(\"Resources\")")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each service client has a Package.version file in the Resources subfolder that needs to be included with the service client.

}
}
}
dependenciesByURL.forEach { writePackageDependency(writer, it) }
}

val dependenciesByTarget = externalDependencies.distinctBy { it.expectProperty("target", String::class.java) + it.packageName }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Dependencies by target" are written to the service client target's dependencies section.

@jbelkins jbelkins marked this pull request as ready for review July 1, 2024 16:24
@jbelkins jbelkins requested review from dayaffe and sichanyoo July 1, 2024 16:24
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.

Just a couple Qs

//
// Also leave out the headers when JSON or the version file is being written,
// as indicated by the file extension.
val isPackageManifest = listOf(PACKAGE_MANIFEST_NAME, (PACKAGE_MANIFEST_NAME + ".swift")).contains(fullPackageName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Why is fullPackageName (which is "Package" for a Swift Package.swift file) compared against "Package.swift.txt" and "Package.swift.txt.swift"? Can't we just delete this val declaration and do the following?

return contents.takeIf { fullPackageName == "Package" && isNonSwiftSourceFile } ?: (copyrightNotice + imports + contents)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look up above, the suffix .swift is stripped off of fullPackageName (which itself isn't a very good name). I don't know the precise rationale for this, but for now I don't want to alter that behavior.

In the interim, until we go all-in on individual service client packages, I want to generate the package manifests but not have Xcode treat the services as their own packages, so I changed PACKAGE_MANIFEST_NAME to Package.swift.txt so the file is still there but it's not recognized as a package manifest.

But since I want Package.swift.txt to omit the headers, same as Package.swift, I had to include two alternate strings on Line 160 so I don't have to change PACKAGE_MANIFEST_NAME in more than one place to make the switch correctly.

This logic will all go away once service client packages are permanent.

@@ -6,12 +6,15 @@ package software.amazon.smithy.swift.codegen

Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Just a general Q on manifest generator. AFAIK we don't generate Package.swift for smithy-swift, and this is just used downstream in aws-sdk-swift when we generate Package.swift files for services - am I correct in my understanding there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. The only packages that use the PackageManifestGenerator are service clients. Runtime modules in both aws-sdk-swift and in smithy-swift will have hand-maintained manifests.

@jbelkins jbelkins merged commit 571c78d into main Jul 9, 2024
27 checks passed
@jbelkins jbelkins deleted the jbe/spr branch July 9, 2024 22:52
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