-
Notifications
You must be signed in to change notification settings - Fork 35
Swift setup to test building the code generated SDK #31
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, couple small suggestions
import java.io.IOException | ||
import java.util.concurrent.TimeUnit | ||
|
||
open class BuildGeneratedSDK : DefaultTask() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting to see as an example of how to do this. Any reason to not just use an exec task though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exec task also achieves the same purpose and with less complexity of defining a custom gradle task. Thanks for pointing this out. Changed to using exec task but leaving this file as a reference for custom gradle task as I spent some time setting this up.
|
||
fun render() { | ||
// add imports | ||
writer.write("import ClientRuntime") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine but if you can it's best to structure it as a dependency on a symbol so that imports get generated automatically. As an example if the service symbol depends on ClientRuntime
when it's declared then add it as a dependency of the service symbol.
Not a big deal though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified to add dependency on service shape.
} | ||
|
||
private fun swiftBuildToolsExist(): Boolean { | ||
val swiftVersionCmd = "swift --version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach has a limitation that we need the swift build tools installed in the host machine to test the build, and if it's not present, it skips this step. In case of circleci, we use open jdk image and if we want to make use of this, we might have to install swift build tools in it, which I am not sure many people do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting is there a way for us to install it tho?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes should be possible. Let me try this. We might need it anyway at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installing swift toolchain on linux circleci image was not straightforward because of several missing shared object files
, failed to launch REPL process: process launch failed: 'A' packet returned an error: 8
errors I was getting. Moved the swift workflow to macOS image instead.
setIndentText(" ") | ||
} | ||
|
||
writer.write("// swift-tools-version:5.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe we should store swift version in SwiftSettings cuz this gets used in multiple places now (podspec generator, infoplist generator)
BIG("pod", "BigNumber", "2.0"), | ||
CLIENT_RUNTIME("pod", "ClientRuntime", "0.1.0"); | ||
enum class SwiftDependency(val type: String, val namespace: String, val version: String, val url: String) : SymbolDependencyContainer { | ||
BIG("pod", "BigNumber", "2.0", url = "https://github.com/mkrd/Swift-Big-Integer.git"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need way to distinguish between spm packages and cocoapods here. this package was originally a pod and there may be some packages that arent on both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are depending on a package, then we need to include it in both podspec and package manifest files right? Which means we cannot depend on a package not supported in one but supported on other? This could potentially be a limitation if we want to support both SPM and Cocoapods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessarily. what if its a swift server package that doesnt have coocapod support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swift server packages do not support cocoapods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah so we cannot have a podspec with dependency on that package and will have to only have a package manifest file in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove type of pod
import software.amazon.smithy.codegen.core.SymbolDependency | ||
import software.amazon.smithy.codegen.core.SymbolDependencyContainer | ||
|
||
enum class SwiftDependency(val type: String, val namespace: String, val version: String) : SymbolDependencyContainer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe type here could be an enum -> cocoapod or spm
...ift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/PackageManifestGenerator.kt
Show resolved
Hide resolved
...ift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/PackageManifestGenerator.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work on this one, few questions and comments here.
… minor formatting
After this PR is merged to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue #, if available:
Need for tools setup to make sure the code generated swift SDK builds successfully
Description of changes:
Add package manifest file for ClientRuntime.
Code generate package manifest file with dependency on local ClientRuntime package.
Defines an extension to build task of
swift-codegen-test
to build the generated SDK -- requires swift build tools to be available in the host machine.Remove XcodeProject for clientruntime; it should be generated when required using
swift package generate-xcodeproj
as a best practice for using SPM.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.