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

build(gradle): upgrade gradle 6.4.2 >>> 8.2.1 #5109

Merged
merged 17 commits into from
Sep 3, 2023

Conversation

DarkWeird
Copy link
Contributor

@DarkWeird DarkWeird commented Jul 6, 2023

Contains

  • Updating gradle from 6.4.2 -> 8.2.1
  • Updating gradle's files for which became invalid

How to test

  • run gradlew jar game -> should build and run game
  • run gradlew unitTest -> should run the unit test suites

Remarks

Java 20 is not yet supported, see gradle/gradle#23488.

Closes #5111

@github-actions github-actions bot added the Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. label Jul 6, 2023
@jdrueckert jdrueckert added this to the 2023 Revive - Milestone 1 milestone Jul 6, 2023
Copy link
Member

@skaldarnar skaldarnar left a 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

Comment on lines 273 to 276
tasks.named("classes") {
dependsOn(tasks.named("copyResourcesToClasses"))
}

Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 DupliactesStrategys (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.

Copy link
Member

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?

Copy link
Member

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)

Copy link
Contributor Author

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)

@skaldarnar skaldarnar changed the title build(bumpup-gradle): upgrade gradle and code for it. build(gradle): upgrade gradle 6.4.2 >>> 8.2 Jul 6, 2023
…ses`. unitTests and integrationTests works now
@jdrueckert
Copy link
Member

green checkmarks, yey! 🥳

jdrueckert
jdrueckert previously approved these changes Jul 9, 2023
Comment on lines +12 to +16
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")) }
}
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor

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:

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")
}
.

Copy link
Member

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 🤷‍♀️

Comment on lines +15 to +20
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")) }
}

Copy link
Member

Choose a reason for hiding this comment

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

same question here

@soloturn
Copy link
Contributor

soloturn commented Jul 10, 2023

added a couple of lines in #5113 , which segvaults now with java-20, openjdk, on arch linux:

---------------  S U M M A R Y ------------

Command Line: -XX:MaxDirectMemorySize=512M -XX:+PrintCommandLineFlags -Xmx768M -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en -Duser.variant org.terasology.engine.Terasology --homedir=.

Host: Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz, 4 cores, 15G, Arch Linux
Time: Mon Jul 10 06:26:14 2023 CEST elapsed time: 4.768798 seconds (0d 0h 0m 4s)

---------------  T H R E A D  ---------------

Current thread (0x00007fe948988640):  JavaThread "splashscreen-loop" daemon [_thread_in_native, id=114136, stack(0x00007fe92c7f8000,0x00007fe92c8f8000)]

Stack: [0x00007fe92c7f8000,0x00007fe92c8f8000],  sp=0x00007fe92c8f6320,  free space=1016k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
C  [iris_dri.so+0xb560c]
Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
j  org.lwjgl.opengl.GL11C.glClear(I)V+0
j  org.lwjgl.opengl.GL11.glClear(I)V+1
j  org.terasology.splash.glfw.graphics.Renderer.clear()V+3
j  org.terasology.engine.GLFWSplashScreen.run()V+457
j  java.lang.Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V+5 java.base@20.0.1
j  java.lang.Thread.run()V+19 java.base@20.0.1
v  ~StubRoutines::call_stub 0x00007fe937d37cc6

siginfo: si_signo: 11 (SIGSEGV), si_code: 1 (SEGV_MAPERR), si_addr: 0x00007feeb60ba9f0

@jdrueckert
Copy link
Member

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...

@jdrueckert jdrueckert self-requested a review July 12, 2023 18:39
@jdrueckert jdrueckert dismissed their stale review July 12, 2023 18:40

noticed the gradle wrapper changes in keturn's PRs - we might want those, too

@BenjaminAmos
Copy link
Contributor

The gradle upgrade broke the groovyw command, so I've replaced the groovyw wrapper a newer one from 2.1.0 to fix it.

…e-to-8.2

# Conflicts:
#	facades/TeraEd/build.gradle
@soloturn
Copy link
Contributor

#5113 does not crash java-20 jvm any more when as well merging #5107 . java-11 still works. imo these 2 plus #5109 could be merged now.

@soloturn
Copy link
Contributor

soloturn commented Jul 20, 2023

@DarkWeird you mind merging or cherry-picking 0777f4f , #5113? or you prefer this one to be targetted to develop later?

soloturn and others added 2 commits July 20, 2023 12:17
kotlin-1.9 knows java-20. setting java-11 as target a little more harsh
than currently.
@soloturn
Copy link
Contributor

soloturn commented Jul 28, 2023

found another one, sputbug needs to be pudated: #5126. if you could merge @DarkWeird ?

@BenjaminAmos
Copy link
Contributor

Gradle 8.3-rc2 now supports Java 20. I don't believe that we're aiming for Java 20 support yet but it should now be possible to test against it with a slight gradle upgrade.

@jdrueckert jdrueckert changed the title build(gradle): upgrade gradle 6.4.2 >>> 8.2 build(gradle): upgrade gradle 6.4.2 >>> 8.2.1 Jul 30, 2023
@jdrueckert
Copy link
Member

jdrueckert commented Jul 31, 2023

@DarkWeird gradlew output mentions the following:

This version of Gradle expects version '4.0.14' of the `kotlin-dsl` plugin but version '4.1.0' has been applied to project ':build-logic'. Let Gradle control the version of `kotlin-dsl` by removing any explicit `kotlin-dsl` version constraints from your build logic.
WARNING: Unsupported Kotlin plugin version.
The `embedded-kotlin` and `kotlin-dsl` plugins rely on features of Kotlin `1.8.20` that might work differently than in the requested version `1.9.0`.

Guess we might want to fix that as part of this PR?

@jdrueckert
Copy link
Member

Also, there's a bunch of warnings which I think we don't need to fix in this PR, though.
I created an issue to track them: #5127

@jdrueckert jdrueckert merged commit 43a7e05 into develop Sep 3, 2023
10 checks passed
@jdrueckert jdrueckert deleted the build/bumpup-gradle-to-8.2 branch September 3, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants