MovingBlocks / Terasology

Terasology - open source voxel world
http://terasology.org
Apache License 2.0
3.69k stars 1.34k forks source link

refactor: fix gradle warnings #5130

Closed PurityLake closed 1 year ago

PurityLake commented 1 year ago

Contains

Addresses #5127

This should be added into #5109

How to test

./gradlew wrapper --warning-mode all

Outstanding before merging

I made some changes to the versions of some plugins due to the version being older, so they were using deprecated functions.

soloturn commented 1 year ago

@skaldarnar as you approved as is: setting the kotlin version to something outdated should not be done, "4.0.14". the future kotlin version is fine as gradle-8.3 will be merged next and has this new version anyway.

spotbugs needs this one as well to really run: #5126 .

PurityLake commented 1 year ago

Well, at compile time it complains about using a newer version. It even says what version to use.

@soloturn could you point me to where spotbugs gradle plug-in requires a certain kotlin version? Can't find anything anywhere on their repo

skaldarnar commented 1 year ago

Well, at compile time it complains about using a newer version. It even says what version to use.

I think this is what I observed as well. I don't like using old(er) versions, but if the specific lineup of tools explicitly requires some version we have to figure that out somehow.

I'm more than happy if we can upgrade to a newer version with the next Gradle release, but looking at the past I'd expect that we have to use a specifc (by then outdated) version until there's another upgrade for Gradle available.

Let's merge things one by one, and we'll hopefully end up in a state where we use a modern Gradle with a modern Kotlin running moder Spotbugs 🙃

PurityLake commented 1 year ago

@skaldarnar I could add a comment to remind us to upgrade maybe?

soloturn commented 1 year ago

this pull request includes 2 things too much which imo should be taken out:

  1. spotbugs update. only one version updated, second one still old, which makes it fail. addressed in #5126. there is no dependency to kotlin version though.
  2. kotlin downgrade would make it fail again with java-20. true, it issues a warning currently - but fortunately its only a warning, and build works. proper done, gradle should be upgraded and kotlin explicit version removed to have the default again. at the time when we explicitely set the kotlin version, gradle-8.3 was not yet released. see @skaldarnar comments in #5107 for the run problem before kotlin update.
PurityLake commented 1 year ago

Afaik there are no plans to support Java 20, just Java 17 for now. Also specifying versions is better in the long run, version mismatches makes things be a bit more of a pain, but it is clear when an update is required and what may cause a compilation issue which could be hard to debug.

jdrueckert commented 1 year ago

@soloturn I merged #5126 and the java 20 issue is not a problem at the moment as we're targeting java 20 only at a later point. For the next milestone, java 17 is the target.

@PurityLake can you please update the PR with the latest state of develop?

jdrueckert commented 1 year ago

umm... whoops... didn't recall that effect on PRs from forks, sorry :see_no_evil: can you open it up again against develop pls? :upside_down_face:

soloturn commented 1 year ago

@PurityLake can you please open the pr please?