-
Notifications
You must be signed in to change notification settings - Fork 173
update build to work with gradle 9.1 and Java 25 #962
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
Conversation
016b252
to
c5719d4
Compare
plugin/settings.gradle.kts
Outdated
Runtime::class.java | ||
.let { it.getPackage().specificationVersion ?: it.getMethod("version").invoke(null).toString() } | ||
.split(".")[0].toInt() <= 11 |
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.
Good lord that's messy 😂
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.
yeah, declaring a separate function doesn't work because the plugin block is evaluated before the rest of the script.
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.
Yea... I know... Funnily enough, I've actually mucked around in that part of the Gradle code many-a-year-ago. It's an absolutely wild bit of code. 😆
a022e36
to
015f92c
Compare
015f92c
to
eb7407a
Compare
eb7407a
to
35c787c
Compare
@JLLeitschuh would you mind updating the "required checks" to reflect the new action task names? |
5524597
to
6d59364
Compare
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.
Some questions
- name: Restore TestKit cache | ||
# Inspired by https://github.com/actions/cache/issues/432#issuecomment-740376179 | ||
uses: actions/cache@0400d5f644dc74513175e3cd8d07132dd4860809 # v4.2.4 | ||
with: | ||
path: | | ||
plugin/.gradle-test-kit/caches/modules-2 | ||
plugin/.gradle-test-kit/caches/files-2.1 | ||
plugin/.gradle-test-kit/caches/metadata-2.96 | ||
key: gradle-wrapper-${{ hashFiles('**/gradlew') }}-${{ github.sha }} | ||
restore-keys: | | ||
gradle-wrapper-${{ hashFiles('**/gradlew') }}- | ||
gradle-wrapper- |
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.
Did this get dropped during some of my refactors accidentally? Whoops 😬
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.
no this is copy-pasted from another spot, b/c i had to make a separate task
plugin/build.gradle.kts
Outdated
named<JvmTestSuite>("test") { | ||
useJUnitJupiter() | ||
} | ||
listOf(8, 11, 17, 21, 25) // TODO get from data feed |
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.
Get from the data feed, with a default set that automatically exist for easy checkout-and-run
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.
the default test
task runs with whatever JDK the build is running on, so that takes care of the easy checkout and run.
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.
im not so sure we actually should get this from a data feed, given:
- it doesnt change very often
- I don't think there is a way to do this that wouldnt break the configuration cache
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 mean, we either want to do it, or remove the TODO 😂
Is it fine to break configuration cache on CI? That doesn't seem like it's too bad IMHO
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.
on CI, no, but it would break if for local builds as well.
yeah ill remove the TODO if we agree, I left it here for now until we make the decision
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 removed the TODO
0b52dec
to
08695a9
Compare
08695a9
to
ebfd1b6
Compare
ebfd1b6
to
f7936d2
Compare
That's probably going to need to happen post-merge, I can always bypass the rules to let a merge happen |
plugin/build.gradle.kts
Outdated
enabled = false | ||
} | ||
|
||
val ensureDependenciesAreInlined by tasks.registering { |
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.
Is this task even doing what we were thinking it was doing? Is it doing it adequately?
You said that our pom and gradle metadata dependencies were being exposed as direct dependencies, right?
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 played around with it a bit, I there was nothing I could do to actually make this thing fail. I think the shadow plugin has improved to where it is harder to mess this stuff up now. So I have removed this task
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.
Left some review comments
create separate test tasks for each java version attempt to recreate project isolation failure create separate Gradle test task for each JVM version. these are still individually invoked by github actions matrix fix GMM and POM to properly reflect that the published Jar is a shadow jar
f7936d2
to
c03b760
Compare
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.
LGTM! Just one question
java-version: ${{ matrix.java_version }} | ||
java-version: | | ||
${{ matrix.java_version }} | ||
21 |
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.
Isn't this already in the matrix? Do we need to add it again manually? Just trying to understand
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.
the daemon toolchain is locked to 21 now, so we always need 21 for gradle itself. the test process is forked off of that using a project toolchain launcher, determined by the parameter we pass in (which is sourced from the matrix)
Is this good to merge? Once I do so, I'll update the GitHub actions required cheks |
yep! Thanks |
update build to work with gradle 9.1 and Java 25
create separate test tasks for each java version
attempt to recreate project isolation failure
Kotlin 1.7 support dropped