Skip to content

Conversation

xinsong-cui
Copy link
Contributor

Issue #

Description of changes

refactor build.gradle.kts to fix Gradle configuration cache compatibility issues in build script

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

@xinsong-cui xinsong-cui added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Sep 26, 2025

gradle.buildFinished {
startTestServers.stop()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactor to address 'Gradle.buildFinished' is unsupported

file.writeText(version)
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactor to get rid of Gradle script object references

generatedSourcesDir.set(layout.buildDirectory.dir("generated-src/src"))
}

tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactor to get rid of Gradle script object references

projectionsDir.set(layout.buildDirectory.dir("smithyprojections/serde-tests"))
}

tasks.kotlinSourcesJar {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactor to get rid of Gradle script object references

This comment has been minimized.

@xinsong-cui xinsong-cui marked this pull request as ready for review September 26, 2025 20:56
@xinsong-cui xinsong-cui requested a review from a team as a code owner September 26, 2025 20:56
Comment on lines +98 to +99
@get:Inject
abstract val fs: FileSystemOperations
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use the built-in file operations like we do in other Gradle tasks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using project.copy accesses the Project instance at task execution time which incompatible with gradle cache

}

afterEvaluate {
testServerService.get().parameters.classpath.from(kotlin.targets.getByName("jvm").compilations["test"].runtimeDependencyFiles!!)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this line do?

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 works the same as
https://github.com/smithy-lang/smithy-kotlin/blob/main/runtime/protocol/http-client-engines/test-suite/build.gradle.kts#L106-L107

I was getting Cannot mutate the state of configuration If I didnt put in afterEvaluation

Comment on lines -131 to -134
gradle.buildFinished {
startTestServers.stop()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How do the test servers get shut down now, when does TestServerService::close get invoked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TestServerService is AutoCloseable now, TestServerService::close() will get invoke when build finished. (Same as original version)

Comment on lines +126 to +130
afterEvaluate {
tasks.named("jvmSourcesJar") {
dependsOn(stageGeneratedSources)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to configure this for JVM tasks? Does this work for Native builds too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was gettting Reason: Task ':tests:benchmarks:serde-benchmarks:jvmSourcesJar' uses this output of task ':tests:benchmarks:serde-benchmarks:stageGeneratedSources' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. without it, I am guessing also need this for Native? but not sure

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I am a couple CI checks away from merging kn-main into main, you should be able to test it out when you rebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just merge it, seems like work fines for Native

This comment has been minimized.

This comment has been minimized.

Copy link

Affected Artifacts

Changed in size
Artifact Pull Request (bytes) Latest Release (bytes) Delta (bytes) Delta (percentage)
http-test-jvm.jar 62,181 59,427 2,754 4.63%
crt-util-jvm.jar 21,733 21,055 678 3.22%
aws-signing-default-jvm.jar 68,505 66,941 1,564 2.34%
runtime-core-jvm.jar 852,677 835,526 17,151 2.05%
http-client-engine-crt-jvm.jar 54,233 53,980 253 0.47%
aws-signing-crt-jvm.jar 16,114 16,081 33 0.21%
test-suite-jvm.jar 99,844 100,085 -241 -0.24%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants