Skip to content

Conversation

@aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Jul 2, 2021

Issue #

Description of changes

  • (ci): Adds a job to build all the AWS services (in parallel) as part of CI. This should help keep the quality bar high and reduce the chance we break a working/compiling service.
    • NOTE: unlike the protocol tests the building of all AWS services will not run from smithy-kotlin CI. This isn't insurmountable but it is quite a bit more work. Getting the build times reasonable required implementing the build as a workflow which makes it hard to re-use.
  • (fix/chore): Fixed the dependency behavior of protocol tests and SDK codegen. The smithy build (generate) tasks now depend on the runtime classpath which ensures the dependencies declared are up to date. Changes to codegen should now automatically be rebuilt when you goto (re)generate code. There is still an issue with smithy itself and caching behavior where it requires a ./gradlew --stop call but at least now you can be sure codegen is up to date when you generate.

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

import sys
import os

not_supported_yet = ["ec2", "location", "elasticbeanstalk", "marketplacecommerceanalytics"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Correctness: Elastic Beanstalk should be supported now.

Copy link
Contributor

Choose a reason for hiding this comment

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

marketplacecommerceanalytics can join the party as well.

find codegen/sdk/aws-models -name '*.json' | sort | awk "NR % $JOBS == $JOB - 1" > services.txt
echo "Building the following services"
cat services.txt
./gradlew --no-daemon :codegen:sdk:bootstrap -Paws.services=$(cat services.txt | .github/scripts/generate-includes.py)
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why is the generate-includes.py script necessary if we can specify which protocols/services not to build? Won't this be equivalent and simpler?

./gradlews -Paws.services=-location,-marketplacecommerceanalytics -Paws.protocols=-ec2Query :codegen:sdk:bootstrap

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 is executed in batches. We only want to build n services per runner (where n = 1/NUM_JOBS). So the include list enforces that only those services even exist in the build. Honestly, this can all evolve I just wanted to get something that runs in a reasonable amount of time in the short term.

Comment on lines 20 to 22
```sh
./gradlew :codegen:sdk:bootstrap
./gradlew --no-daemon :codegen:sdk:bootstrap
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why are we recommending --no-daemon now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should help prevent having to call ./gradlew --stop before executing.

@@ -1,10 +1,13 @@
kotlin.code.style=official
kkotlin.incremental.js=true
Copy link
Contributor

Choose a reason for hiding this comment

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

Correctness: kkotlinkotlin

Copy link
Contributor

@kggilmer kggilmer left a comment

Choose a reason for hiding this comment

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

Nice work on raising the bar for build correctness along w/ this CI addition.

with:
name: m2-${{ env.name }}-${{ github.sha }}
path: ~/.m2
retention-days: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

question

why 2 days here? I assume this repo caching is particular to a given PR. Would this be for day boundries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really need the artifact past building the AWS services in parallel. 2 days seemed reasonable to me to debug and diagnose any build failures.

- name: Generate SDKs
run: |
ls -lsa ~/.m2/repository
find codegen/sdk/aws-models -name '*.json' | sort | awk "NR % $JOBS == $JOB - 1" > services.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

question

I take it this is what breaks the work up for each worker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it splits the list of services into batches modulo the number of "jobs".

dependsOn(tasks["generateSmithyBuild"])
inputs.file(projectDir.resolve("smithy-build.json"))
// ensure smithy-aws-kotlin-codegen is up to date
inputs.files(configurations.compileClasspath)
Copy link
Contributor

Choose a reason for hiding this comment

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

comment

very nice! I was trying to get these gradle configurations to work at one point but couldn't make it work. Well done!

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 seems to solve it but let me know if you see issues still. For me it seemed to rebuild appropriately now. However, I still have issues where I have to call ./gradlew --stop but at least everything is up-to date. Running codegen with --no-daemon seems to help some FWIW


// force rebuild every time while developing
tasks["buildSdk"].outputs.upToDateWhen { false }
tasks["generateSdk"].outputs.upToDateWhen { false }
Copy link
Contributor

Choose a reason for hiding this comment

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

nice rename

@aajtodd
Copy link
Contributor Author

aajtodd commented Aug 23, 2021

closing in favor #289 which adds a codebuild job instead. All the other gradle changes were pulled in separately in #281

@aajtodd aajtodd closed this Aug 23, 2021
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.

3 participants