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

Refactor cargo test Gradle steps #1378

Closed
crisidev opened this issue May 7, 2022 · 0 comments · Fixed by #1422
Closed

Refactor cargo test Gradle steps #1378

crisidev opened this issue May 7, 2022 · 0 comments · Fixed by #1422
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@crisidev
Copy link
Contributor

crisidev commented May 7, 2022

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 and codegen/build.gradle.kt.

It will be good to refactor and make the steps reusable and define them in a single place.

Tasks to refactor:

task("generateSmithyBuild") {
    description = "generate smithy-build.json"
    doFirst {
        projectDir.resolve("smithy-build.json")
            .writeText(
                generateSmithyBuild(
                    rootProject.projectDir.absolutePath,
                    pluginName,
                    codegenTests(properties, allCodegenTests)
                )
            )
    }
}

task("generateCargoWorkspace") {
    description = "generate Cargo.toml workspace file"
    doFirst {
        buildDir.resolve("$workingDirUnderBuildDir/Cargo.toml")
            .writeText(generateCargoWorkspace(pluginName, codegenTests(properties, allCodegenTests)))
    }
}

tasks["smithyBuildJar"].dependsOn("generateSmithyBuild")

tasks["assemble"].finalizedBy("generateCargoWorkspace")

tasks.register<Exec>(Cargo.CHECK.toString) {
    workingDir("$buildDir/$workingDirUnderBuildDir")
    environment("RUSTFLAGS", defaultRustFlags)
    commandLine("cargo", "check")
    dependsOn("assemble")
}

tasks.register<Exec>(Cargo.TEST.toString) {
    workingDir("$buildDir/$workingDirUnderBuildDir")
    environment("RUSTFLAGS", defaultRustFlags)
    commandLine("cargo", "test")
    dependsOn("assemble")
}

tasks.register<Exec>(Cargo.DOCS.toString) {
    workingDir("$buildDir/$workingDirUnderBuildDir")
    environment("RUSTDOCFLAGS", defaultRustDocFlags)
    commandLine("cargo", "doc", "--no-deps")
    dependsOn("assemble")
}

tasks.register<Exec>(Cargo.CLIPPY.toString) {
    workingDir("$buildDir/$workingDirUnderBuildDir")
    environment("RUSTFLAGS", defaultRustFlags)
    commandLine("cargo", "clippy")
    dependsOn("assemble")
}

tasks["test"].finalizedBy(cargoCommands(properties).map { it.toString })

tasks["clean"].doFirst { delete("smithy-build.json") }
@crisidev crisidev added the enhancement New feature or request label May 7, 2022
@david-perez david-perez added server Rust server SDK good first issue Good for newcomers and removed server Rust server SDK labels May 27, 2022
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
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants