-
Notifications
You must be signed in to change notification settings - Fork 31
misc: enable gradle cache #1427
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
base: main
Are you sure you want to change the base?
Conversation
|
||
gradle.buildFinished { | ||
startTestServers.stop() | ||
} |
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.
refactor to address 'Gradle.buildFinished' is unsupported
file.writeText(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.
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> { |
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.
refactor to get rid of Gradle script object references
projectionsDir.set(layout.buildDirectory.dir("smithyprojections/serde-tests")) | ||
} | ||
|
||
tasks.kotlinSourcesJar { |
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.
refactor to get rid of Gradle script object references
This comment has been minimized.
This comment has been minimized.
runtime/protocol/http-client-engines/test-suite/build.gradle.kts
Outdated
Show resolved
Hide resolved
@get:Inject | ||
abstract val fs: FileSystemOperations |
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.
Why can't we use the built-in file operations like we do in other Gradle tasks?
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.
using project.copy
accesses the Project instance at task execution time which incompatible with gradle cache
runtime/protocol/http-client-engines/test-suite/build.gradle.kts
Outdated
Show resolved
Hide resolved
} | ||
|
||
afterEvaluate { | ||
testServerService.get().parameters.classpath.from(kotlin.targets.getByName("jvm").compilations["test"].runtimeDependencyFiles!!) |
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.
What does this line 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.
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
gradle.buildFinished { | ||
startTestServers.stop() | ||
} |
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.
How do the test servers get shut down now, when does TestServerService::close
get invoked?
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.
TestServerService
is AutoCloseable now, TestServerService::close()
will get invoke when build finished. (Same as original version)
afterEvaluate { | ||
tasks.named("jvmSourcesJar") { | ||
dependsOn(stageGeneratedSources) | ||
} | ||
} |
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.
Why do we need to configure this for JVM tasks? Does this work for Native builds too?
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 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
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.
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
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.
Just merge it, seems like work fines for Native
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Affected ArtifactsChanged in size
|
Issue #
Description of changes
refactor
build.gradle.kts
to fix Gradle configuration cache compatibility issues in build scriptBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.