-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
build(gradle): upgrade gradle 6.4.2 >>> 8.2.1 #5109
Conversation
5a08176
to
ef9695f
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.
With Java 11 compilation and starting the game works fine. 👍
With Java 17 it compiles and I see the expected compilation errors.
With Java 20 (Liberica) compilation fails with
❯ gradlew jar game (base)
Processing facade facades:PC, including it as a sub-project
Processing facade facades:TeraEd, including it as a sub-project
> Task :build-logic:generateExternalPluginSpecBuilders UP-TO-DATE
> Task :build-logic:extractPrecompiledScriptPluginPlugins UP-TO-DATE
> Task :build-logic:compilePluginsBlocks UP-TO-DATE
> Task :build-logic:generatePrecompiledScriptPluginAccessors UP-TO-DATE
> Task :build-logic:generateScriptPluginAdapters UP-TO-DATE
> Task :build-logic:compileKotlin FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':build-logic:compileKotlin'.
> Error while evaluating property 'compilerOptions.jvmTarget' of task ':build-logic:compileKotlin'.
> Failed to calculate the value of property 'jvmTarget'.
> Unknown Kotlin JVM target: 20
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
> Get more help at https://help.gradle.org.
BUILD FAILED in 531ms
6 actionable tasks: 1 executed, 5 up-to-date
engine/build.gradle
Outdated
tasks.named("classes") { | ||
dependsOn(tasks.named("copyResourcesToClasses")) | ||
} | ||
|
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 happened to this? All the other changes look like simple drop-in replacements, but this is just removed. Do we just not need it anymore?
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.
gradlew unitTest
fails with a message seemingly related to this:
FAILURE: Build failed with an exception.
* What went wrong:
Some problems were found with the configuration of task ':engine-tests:copyResourcesToClasses' (type 'Copy').
- Gradle detected a problem with the following location: '/home/jenkins/agent/workspace/Terasology_engine_PR-5109/engine-tests/build/classes'.
Reason: Task ':engine-tests:compileTestJava' uses this output of task ':engine-tests:copyResourcesToClasses' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
Possible solutions:
1. Declare task ':engine-tests:copyResourcesToClasses' as an input of ':engine-tests:compileTestJava'.
2. Declare an explicit dependency on ':engine-tests:copyResourcesToClasses' from ':engine-tests:compileTestJava' using Task#dependsOn.
3. Declare an explicit dependency on ':engine-tests:copyResourcesToClasses' from ':engine-tests:compileTestJava' using Task#mustRunAfter.
For more information, please refer to https://docs.gradle.org/8.2/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.
- Gradle detected a problem with the following location: '/home/jenkins/agent/workspace/Terasology_engine_PR-5109/engine-tests/build/resources/main'.
Reason: Task ':engine-tests:copyResourcesToClasses' uses this output of task ':engine-tests:processResources' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
Possible solutions:
1. Declare task ':engine-tests:processResources' as an input of ':engine-tests:copyResourcesToClasses'.
2. Declare an explicit dependency on ':engine-tests:processResources' from ':engine-tests:copyResourcesToClasses' using Task#dependsOn.
3. Declare an explicit dependency on ':engine-tests:processResources' from ':engine-tests:copyResourcesToClasses' using Task#mustRunAfter.
For more information, please refer to https://docs.gradle.org/8.2/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.
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.
Hm. I just delete it , and game running.
And i see how fix this now.
Seems there we should declare second edge for task graph.
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.
First I tried to add those missing dependencies, but it gets weird quickly, and you also have to add DupliactesStrategy
s (I suspect because we wildly copy files around and don't care about duplicates or anything). I've added more and more dependsOn
and duplicatesStrategy = 'exclude'
, but in the end still failed to run the tests 😔
My second attempt was to remove the copyResourcesToClasses
task and pointers to it completely, from both :engine
and :engine-tests
. Eventually, the engine (tests) seemingly built, but then errors popped up when dealing with modules:
* What went wrong:
Some problems were found with the configuration of task ':modules:CoreWorlds:compileTestJava' (type 'JavaCompile').
- Gradle detected a problem with the following location: '/home/skaldarnar/Code/movingblocks/ts-iota/modules/BiomesAPI/build/classes'.
Reason: Task ':modules:CoreWorlds:compileTestJava' uses this output of task ':modules:BiomesAPI:syncModuleInfo' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
Possible solutions:
1. Declare task ':modules:BiomesAPI:syncModuleInfo' as an input of ':modules:CoreWorlds:compileTestJava'.
2. Declare an explicit dependency on ':modules:BiomesAPI:syncModuleInfo' from ':modules:CoreWorlds:compileTestJava' using Task#dependsOn.
3. Declare an explicit dependency on ':modules:BiomesAPI:syncModuleInfo' from ':modules:CoreWorlds:compileTestJava' using Task#mustRunAfter.
For more information, please refer to https://docs.gradle.org/8.2/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.
- Gradle detected a problem with the following location: '/home/skaldarnar/Code/movingblocks/ts-iota/modules/BiomesAPI/build/classes'.
Reason: Task ':modules:CoreWorlds:compileTestJava' uses this output of task ':modules:BiomesAPI:syncAssets' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
Possible solutions:
1. Declare task ':modules:BiomesAPI:syncAssets' as an input of ':modules:CoreWorlds:compileTestJava'.
2. Declare an explicit dependency on ':modules:BiomesAPI:syncAssets' from ':modules:CoreWorlds:compileTestJava' using Task#dependsOn.
3. Declare an explicit dependency on ':modules:BiomesAPI:syncAssets' from ':modules:CoreWorlds:compileTestJava' using Task#mustRunAfter.
For more information, please refer to https://docs.gradle.org/8.2/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.
- Gradle detected a problem with the following location: '/home/skaldarnar/Code/movingblocks/ts-iota/modules/BiomesAPI/build/classes'.
Reason: Task ':modules:CoreWorlds:compileTestJava' uses this output of task ':modules:BiomesAPI:syncDeltas' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
Possible solutions:
1. Declare task ':modules:BiomesAPI:syncDeltas' as an input of ':modules:CoreWorlds:compileTestJava'.
2. Declare an explicit dependency on ':modules:BiomesAPI:syncDeltas' from ':modules:CoreWorlds:compileTestJava' using Task#dependsOn.
3. Declare an explicit dependency on ':modules:BiomesAPI:syncDeltas' from ':modules:CoreWorlds:compileTestJava' using Task#mustRunAfter.
For more information, please refer to https://docs.gradle.org/8.2/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.
- Gradle detected a problem with the following location: '/home/skaldarnar/Code/movingblocks/ts-iota/modules/BiomesAPI/build/classes'.
Reason: Task ':modules:CoreWorlds:compileTestJava' uses this output of task ':modules:BiomesAPI:syncOverrides' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
Possible solutions:
1. Declare task ':modules:BiomesAPI:syncOverrides' as an input of ':modules:CoreWorlds:compileTestJava'.
2. Declare an explicit dependency on ':modules:BiomesAPI:syncOverrides' from ':modules:CoreWorlds:compileTestJava' using Task#dependsOn.
3. Declare an explicit dependency on ':modules:BiomesAPI:syncOverrides' from ':modules:CoreWorlds:compileTestJava' using Task#mustRunAfter.
For more information, please refer to https://docs.gradle.org/8.2/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.
- Gradle detected a problem with the following location: '/home/skaldarnar/Code/movingblocks/ts-iota/modules/CoreAssets/build/classes'.
Reason: Task ':modules:CoreWorlds:compileTestJava' uses this output of task ':modules:CoreAssets:syncModuleInfo' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
Possible solutions:
1. Declare task ':modules:CoreAssets:syncModuleInfo' as an input of ':modules:CoreWorlds:compileTestJava'.
2. Declare an explicit dependency on ':modules:CoreAssets:syncModuleInfo' from ':modules:CoreWorlds:compileTestJava' using Task#dependsOn.
3. Declare an explicit dependency on ':modules:CoreAssets:syncModuleInfo' from ':modules:CoreWorlds:compileTestJava' using Task#mustRunAfter.
For more information, please refer to https://docs.gradle.org/8.2/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.
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.
Do we have to re-model all these dependencies to make Gradle happy? Is the new Gradle version just pointing us to places where the setup was "shaky" in the first place?
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.
@BenjaminAmos @DarkWeird do you know what exactly is meant by
//TODO: Remove it when gestalt will can to handle ProtectionDomain without classes (Resources)
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.
@BenjaminAmos @DarkWeird do you know what exactly is meant by
//TODO: Remove it when gestalt will can to handle ProtectionDomain without classes (Resources)
Gestalt works with one source of module.(one jar, one dir etc)
Gradle split resouces and classes(compiled classes) dirs when build sources.
More gradle creates separate classes dir on every sources (java/kotlin/groovy - etc)
…ses`. unitTests and integrationTests works now
green checkmarks, yey! 🥳 |
configure<SourceSetContainer> { | ||
// Adjust output path (changed with the Gradle 6 upgrade, this puts it back) | ||
main { java.destinationDirectory.set(File("$buildDir/classes")) } | ||
test { java.destinationDirectory.set(File("$buildDir/testClasses")) } | ||
} |
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 this now? how was it done before?
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.
Gradle was less strict before, I think. The issue was that both the tests compilation and the main compilation were outputting to the build/classes
directory and Gradle did not like that. This fixed the implicit dependency errors. I'm open to a better solution though, this was more of a quick-fix.
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 snippet was originally from here:
Terasology/engine-tests/build.gradle
Lines 46 to 50 in 324376a
sourceSets { | |
// Adjust output path (changed with the Gradle 6 upgrade, this puts it back) | |
main.java.outputDir = new File("$buildDir/classes") | |
test.java.outputDir = new File("$buildDir/testClasses") | |
} |
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 would argue it's reasonable to separate main compilation classes and test compilation classes, so I don't see a need to look for a better solution right now 🤷♀️
configure<SourceSetContainer> { | ||
// Adjust output path (changed with the Gradle 6 upgrade, this puts it back) | ||
main { java.destinationDirectory.set(File("$buildDir/classes")) } | ||
test { java.destinationDirectory.set(File("$buildDir/testClasses")) } | ||
} | ||
|
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.
same question here
added a couple of lines in #5113 , which segvaults now with java-20, openjdk, on arch linux:
|
In MovingBlocks/gestalt#136 there's additional changes to the gradle wrapper... we might want to consider adopting those changes here, too. Likely they are automatically generated/updated by gradle - I just don't know how to trigger that thinking Using gradlew after updating the version in gradlew-wrapper.properties didn't... |
noticed the gradle wrapper changes in keturn's PRs - we might want those, too
The gradle upgrade broke the |
…e-to-8.2 # Conflicts: # facades/TeraEd/build.gradle
@DarkWeird you mind merging or cherry-picking 0777f4f , #5113? or you prefer this one to be targetted to develop later? |
kotlin-1.9 knows java-20. setting java-11 as target a little more harsh than currently.
found another one, sputbug needs to be pudated: #5126. if you could merge @DarkWeird ? |
Gradle |
@DarkWeird gradlew output mentions the following:
Guess we might want to fix that as part of this PR? |
Also, there's a bunch of warnings which I think we don't need to fix in this PR, though. |
Contains
How to test
gradlew jar game
-> should build and run gamegradlew unitTest
-> should run the unit test suitesRemarks
Java 20 is not yet supported, see gradle/gradle#23488.
Closes #5111