-
Notifications
You must be signed in to change notification settings - Fork 201
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
Refactor cargo test Gradle steps #1378
Labels
Comments
david-perez
added a commit
that referenced
this issue
May 30, 2022
This commit modifies the Gradle buildscripts to avoid unnecessary Cargo rebuilds of the generated crates, decreasing development iteration cycles. Prior to this commit, if you ran a crate-generating command _twice_ on any of the `codegen-test`, `codegen-server-test`, `sdk-codegen-test`, and `codegen-server-test:python` subprojects (without making any changes to the codegen code), both invocations would take the same time: Cargo would recompile the crate and its dependencies, even when the generated crate is identical. This is because Gradle deletes everything under `buildDir` when running the `generateSmithyBuild` task: ``` > Task :codegen-server-test:smithyBuildJar Deleting stale output file: /local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/codegen-server-test/build/smithyprojections/codegen-server-test ``` So the files get recreated each time. While developing, it is likely that only a small number of the generated crate files are modified across rebuilds. [Cargo uses `mtime`](rust-lang/cargo#6529) (among other factors) to determine whether it needs to recompile a unit. Indeed, running with `CARGO_LOG=cargo::core::compiler::fingerprint=trace` yields `err: current filesystem status shows we're outdated`. This commit adds a Gradle task that compares the hashes of the newly generated files with the (previously cached) old ones, and restores their `mtime`s if the hashes coincide. Another issue was causing unnecessary crate rebuilds. Prior to this commit, we were sending `RUSTFLAGS=-D warnings` when invoking Cargo. However, a common thing to do after generating a crate is to open its contents in an editor. The editor's `rust-analyzer` would compile the crate and its dependencies without the `RUSTFLAGS` we had used earlier. The next time you rebuilt the crate, Cargo would claim `err: RUSTFLAGS has changed: previously [], now ["-D", "warnings"]` and recompile everything again. This commit refactors the Gradle tasks so as to not send these flags when invoking Cargo, instead generating a `.cargo/config.toml` containing these flags. This way, `rust-analyzer` also picks them up and does not need to recompile the crates. With both patches, Cargo avoids unnecessary crate rebuilds. All in all, the second invocation of a `./gradlew --info -P modules='simple' -P cargoCommands='test' codegen-server-test:build` command now takes 15 seconds less than the first invocation on my `c5.4xlarge` machine; Cargo does not need to do _any_ work on the second invocation. This commit also refactors the `build.gradle.kts` files of the `sdk`, `sdk-codegen-test`, `codegen-test`, `codegen-server-test`, and `codegen-server-test:python` subprojects to make them DRYer and more consistent. The last 4 subprojects' buildscripts are now much shorter, with all the common logic having been moved to `CodegenTestCommon.kt`. Note that we have made the last 4 subprojects' `cargo check` and `cargo doc` invocations use the same set of flags than in the `sdk` subproject for consistency. Closes #1378. Closes #1412.
david-perez
added a commit
that referenced
this issue
Jun 15, 2022
Avoid unnecessary Cargo crate rebuilds This commit modifies the Gradle buildscripts to avoid unnecessary Cargo rebuilds of the generated crates, decreasing development iteration cycles. Prior to this commit, if you ran a crate-generating command _twice_ on any of the `codegen-test`, `codegen-server-test`, `sdk-codegen-test`, and `codegen-server-test:python` subprojects (without making any changes to the codegen code), both invocations would take the same time: Cargo would recompile the crate and its dependencies, even when the generated crate is identical. This is because Gradle deletes everything under `buildDir` when running the `generateSmithyBuild` task: ``` > Task :codegen-server-test:smithyBuildJar Deleting stale output file: /local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/codegen-server-test/build/smithyprojections/codegen-server-test ``` So the files get recreated each time. While developing, it is likely that only a small number of the generated crate files are modified across rebuilds. [Cargo uses `mtime`](rust-lang/cargo#6529) (among other factors) to determine whether it needs to recompile a unit. Indeed, running with `CARGO_LOG=cargo::core::compiler::fingerprint=trace` yields `err: current filesystem status shows we're outdated`. This commit adds a Gradle task that compares the hashes of the newly generated files with the (previously cached) old ones, and restores their `mtime`s if the hashes coincide. Another issue was causing unnecessary crate rebuilds. Prior to this commit, we were sending `RUSTFLAGS=-D warnings` when invoking Cargo. However, a common thing to do after generating a crate is to open its contents in an editor. The editor's `rust-analyzer` would compile the crate and its dependencies without the `RUSTFLAGS` we had used earlier. The next time you rebuilt the crate, Cargo would claim `err: RUSTFLAGS has changed: previously [], now ["-D", "warnings"]` and recompile everything again. This commit refactors the Gradle tasks so as to not send these flags when invoking Cargo, instead generating a `.cargo/config.toml` containing these flags. This way, `rust-analyzer` also picks them up and does not need to recompile the crates. With both patches, Cargo avoids unnecessary crate rebuilds. All in all, the second invocation of a `./gradlew --info -P modules='simple' -P cargoCommands='test' codegen-server-test:build` command now takes 15 seconds less than the first invocation on my `c5.4xlarge` machine; Cargo does not need to do _any_ work on the second invocation. This commit also refactors the `build.gradle.kts` files of the `sdk`, `sdk-codegen-test`, `codegen-test`, `codegen-server-test`, and `codegen-server-test:python` subprojects to make them DRYer and more consistent. The last 4 subprojects' buildscripts are now much shorter, with all the common logic having been moved to `CodegenTestCommon.kt`. Note that we have made the last 4 subprojects' `cargo check` and `cargo doc` invocations use the same set of flags than in the `sdk` subproject for consistency. Closes #1378. Closes #1412.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In PR #1366 we noticed we are replicating the cargo gradle testing tasks in
codegen-server/build.gradles.kt
,codegen-server/python/build.gradle.kt
andcodegen/build.gradle.kt
.It will be good to refactor and make the steps reusable and define them in a single place.
Tasks to refactor:
The text was updated successfully, but these errors were encountered: