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.

Sign up for GitHub

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

Jump to bottom

refactor: fix gradle warnings #5130

Closed
wants to merge 2 commits into from
Closed

refactor: fix gradle warnings #5130

wants to merge 2 commits into from

Conversation

PurityLake
Copy link
Contributor

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.

  • protobuf -> 0.9.4
  • spotbug -> 5.1.3

@soloturn soloturn mentioned this pull request Aug 21, 2023
Merged
@skaldarnar skaldarnar changed the base branch from develop to build/bumpup-gradle-to-8.2 August 21, 2023 15:41
skaldarnar
skaldarnar approved these changes Aug 21, 2023
View reviewed changes
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.

Overall these changes look very reasonable to me, but I did not test them yet.

build-logic/src/main/kotlin/org/terasology/gradology/module_deps.kt
Comment on lines -93 to -95
val artifactView = resolvable.artifactView {
lenient(true)
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this no longer needed or no longer possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it made no difference to building the project, artifactView may have been used before but not anymore. I believe all that method does is create a builder, but since it used nowhere in the function it is kinda pointless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docs for artifactView

Copy link
Member

Choose a reason for hiding this comment

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

💡

@jdrueckert jdrueckert changed the title Refactor warnings refactor: fix gradle warnings Aug 21, 2023
@jdrueckert jdrueckert added Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Status: Needs Testing Requires to be tested in-game for reproducibility Size: S Small effort likely only affecting a single area and requiring little to no research Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity labels Aug 21, 2023
@jdrueckert jdrueckert added this to the 2023 Revive - Milestone 1 milestone Aug 21, 2023
@jdrueckert jdrueckert linked an issue Aug 21, 2023 that may be closed by this pull request
Refactor build logic to remove deprecations #5127
Closed
6 tasks
@soloturn
Copy link
Contributor

soloturn commented Aug 22, 2023 •
edited
Loading

@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
Copy link
Contributor Author

PurityLake commented Aug 22, 2023 •
edited
Loading

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
Copy link
Member

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
Copy link
Contributor Author

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

@soloturn
Copy link
Contributor

soloturn commented Aug 22, 2023 •
edited
Loading

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 ci: update spotbugs version for gradle 7+, java-17, java-20 #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 Java 18 - do not set SecurityManager if unsupported #5107 for the run problem before kotlin update.

@PurityLake
Copy link
Contributor Author

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
Copy link
Member

@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 jdrueckert deleted the branch MovingBlocks:build/bumpup-gradle-to-8.2 September 3, 2023 21:02
@jdrueckert jdrueckert closed this Sep 3, 2023
@jdrueckert
Copy link
Member

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

@soloturn soloturn mentioned this pull request Sep 18, 2023
Merged
@soloturn
Copy link
Contributor

@PurityLake can you please open the pr please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skaldarnar skaldarnar skaldarnar approved these changes

Assignees
No one assigned
Labels
Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Size: S Small effort likely only affecting a single area and requiring little to no research Status: Needs Testing Requires to be tested in-game for reproducibility Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity
Projects
Revive Terasology
Status: In Progress
Milestone
2023 Revive - Milestone 2
Development

Successfully merging this pull request may close these issues.

Refactor build logic to remove deprecations
4 participants
@PurityLake @soloturn @skaldarnar @jdrueckert