-
Notifications
You must be signed in to change notification settings - Fork 31
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
Upgrade Gradle, Java, Tools #1313
Conversation
@fwatermann Hmmm, unter macOS scheint der Schalter im
Das Schräge dabei ist, dass der Schalter ja extra im Task drin ist und bei der Vorgängerversion noch funktioniert hat. |
@AMatutat Hast Du auch das Problem auf Deiner Maschine oder bin ich nur zu blöd? IntelliJ versteckt ja auch nochmal die JDK-Version für das Projekt (die hatte ich manuell hochgezogen). |
@fwatermann Bitte ersetze im Lt. https://docs.gradle.org/current/dsl/org.gradle.api.tasks.compile.ForkOptions.html ist Mit " @AMatutat Kannst Du das bitte auch nochmal kurz testen/bestätigen? Dabei bitte aufpassen, dass wirklich JDK21 genommen wird (" Edit: Spannend, ich hatte nur nach Edit: Na gut, so ganz falsch war die erste Seite dann doch nicht - "JavaExec" implementiert "JavaForkOptions". Gradle gefällt mir immer besser 🤮 Edit: Noch besser gefällt mir der Methodenaufruf: |
@cagix |
wieviel änderung an der formatierung würde sich allein durch die neue spotless-version ergeben? ich denke, die version sollten wir hier machen, und die sich daraus ergebenden formatierungen auch. den switch auf google-format sollten wir nach wie vor in einem separaten pr machen. was hat das grad mit junit auf sich? nur die version hochziehen sollte doch eigentlich die tests nicht kaputt machen? |
Ich hab PowerMock durch die Mockito-Core der aktuellsten Version ersetzt. Aber vor dem Tausch, liefen bei mir einige Tests auch schon nicht. Vermutlich weil Java 21 🤷♂️ |
@fwatermann was ist denn hier der stand? von mir ist noch ein kommentar offen(?), die bemerkung zum schalter für macos ist nicht komplett umgesetzt? und was ist jetzt mit den tests, laufen die wieder? |
Die Tests laufen wieder. Den MacOS-Schalter mach ich gerade fertig, hab dein Edit nicht gesehen. |
@fwatermann danke! was ich eigentlich meinte war die diskussion #1313 (review) ist noch offen ... wenn ich das richtig sehe, hast du das quasi erledigt, zumindest gibt es jetzt einen entsprechenden eintrag ganz oben in zeile 12..17. aber damit ist zeile 63 jetzt obsolet und sollte komplett gelöscht werden, oder? dann kann auch die diskussion "resolved" werden. |
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.
noch ein paar aktualisierungen, wo wir schon mal dabei sind
hmmm, checkstyle fehlt jetzt was? |
Da scheint es compatiblitätsprobleme zu geben. Mit 10.11.0 läufts 🤷♂️ |
@fwatermann probier mal bitte noch https://mvnrepository.com/artifact/com.puppycrawl.tools/checkstyle/10.12.6 in die gradle-config einzubauen |
irgendwie scheint das ding zwei libs nicht auflösen zu können? sollte das nicht automatisch rekursiv erfolgen? ich glaube nicht, dass wir dieses guava-zeugs direkt ziehen? oder kommt das über doe json-lib (von @AMatutat eingebaut) mit rein und beisst sich dann? |
so langsam habe ich echt immer weniger lust auf dieses projekt :( ... @AMatutat wieso werden für checkstyle die regeln extra konfiguriert im build.gradle, aber die suppression-regeln liegen dann wieder im default-config-ordner? warum ist das nicht einheitlich? |
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.
Checkstyle: checkstyle/checkstyle#14123
hmmmpf. checkstyle ist echt toll 💩🤮 tun so, als ob sie das mit v10.12.7 fixen, aber dabei ist das problem immer noch akut: checkstyle/checkstyle#14211 😤 |
Entscheidung 02.01.24: Wir setzen hier erstmal die alte Checkstyle-Version wieder ein, damit der PR zugeht. Parallel ein Ticket, um Checkstyle rauszuwerfen: #1316 |
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 (see #1316)
Historisch gewachsen. Den Basic-Setup haben wir damals aus dem ersten Dungeon Projekt mitgenommen und da wir nie wirklich glücklich mit unserer Checkstyle-Config (und irgendwie auch unsere Projektstruktur) waren, hat sich das schlichtweg keiner angeschaut. Das gesamte Build-Zeugs ist eher on the fly zusammengesteckt als wirklich konzeptioniert. |
@AMatutat Hihi ... so sieht das auch aus. Naja, wenn ich dazu komme, gehe ich da mal mit ner großen Forke drüber ;) |
closes #1010
closes #1050
Hochziehen von Gradle, JDK, spotbugs, spotless, libGDX, Gson.
Gradle: 8.5
JDK: 21
Spotbugs: 6.0.3
Spotless: 6.23.3
libGDX: 1.12.1
Gson: 2.10.1
mockito-core: 5.8.0
#1312 sollte nach diesem PR rebased & gemerged werden, da es Änderungen in Spotless in der neuen Version gibt.