Simplify buildscript and add Fabric API modules comment. #84

2021-02-12T10:08:33Z

LambdAurora commented

2021-02-12 10:08:33 +00:00

(Migrated from github.com)

To ensure Java standard API compatibility with newer JDK on older JREs there is a dedicated Gradle feature which take advantage of the --release flag on the JDK9+.

It's even recommended to use it as seen on the Gradle userguide.

It also deprecates the sourceCompatibility and targetCompatibility fields.

Also added a comment on Fabric API dependency informing developers of the module system to hopefully encourage people to not fully depend on the whole Fabric API if only a few modules are used.

To ensure Java standard API compatibility with newer JDK on older JREs there is a dedicated Gradle feature which take advantage of the `--release` flag on the JDK9+. It's even recommended to use it as seen on [the Gradle userguide](https://docs.gradle.org/current/userguide/building_java_projects.html#introduction). It also deprecates the sourceCompatibility and targetCompatibility fields. Also added a comment on Fabric API dependency informing developers of the module system to hopefully encourage people to not fully depend on the whole Fabric API if only a few modules are used.

LemmaEOF (Migrated from github.com) approved these changes 2021-02-12 10:11:52 +00:00

ramidzkh (Migrated from github.com) approved these changes 2021-02-12 10:14:56 +00:00

modmuss50 (Migrated from github.com) requested changes 2021-02-12 10:56:58 +00:00

modmuss50 (Migrated from github.com) left a comment

No, not a fan of this.

build.gradle

modmuss50 (Migrated from github.com) commented

2021-02-12 10:56:23 +00:00

Doesnt this download and use java 8? The idea before was to build on the newer JDKs while ensuring that it will run on 8.

Setting the release compiler arg does the same thing without actaully needing to run on java 8. I want to move further away from j8 this seems to be moving closer to it by not even ensuing stuff builds on newer versions.

Doesnt this download and use java 8? The idea before was to build on the newer JDKs while ensuring that it will run on 8. Setting the release compiler arg does the same thing without actaully needing to run on java 8. I want to move further away from j8 this seems to be moving closer to it by not even ensuing stuff builds on newer versions.

LambdAurora (Migrated from github.com) reviewed 2021-02-12 11:39:18 +00:00

build.gradle

LambdAurora (Migrated from github.com) commented

2021-02-12 11:39:18 +00:00

Urf, wasn't aware of this, would make more sense to add something in loom to specify the target JVM version then instead of having an obscure option manipulation.

modmuss50 (Migrated from github.com) reviewed 2021-02-12 11:42:05 +00:00

build.gradle

modmuss50 (Migrated from github.com) commented

2021-02-12 11:42:05 +00:00

Its not so obscure, propper support for it was added in gradle 6.6 before it was a matter of setting a compiler argument.

https://docs.gradle.org/6.6/release-notes.html#javacompile-release

It does look like we could possibly make it a little neater.

Its not so obscure, propper support for it was added in gradle 6.6 before it was a matter of setting a compiler argument. https://docs.gradle.org/6.6/release-notes.html#javacompile-release It does look like we could possibly make it a little neater.

modmuss50 (Migrated from github.com) reviewed 2021-02-12 11:43:32 +00:00

build.gradle

modmuss50 (Migrated from github.com) commented

2021-02-12 11:43:32 +00:00

When we first added this, sourceCompatibility/target was removed but I found its best to keep those in as well to ensure IDEs use the correct target version.

LambdAurora (Migrated from github.com) reviewed 2021-02-12 11:43:53 +00:00

build.gradle

LambdAurora (Migrated from github.com) commented

2021-02-12 11:43:52 +00:00

imo, Gradle should have added a proper field for that in the java block like it does for sourceCompatibility and targetCompatibility.

imo, Gradle should have added a proper field for that in the `java` block like it does for `sourceCompatibility` and `targetCompatibility`.

LambdAurora commented

2021-02-12 11:51:22 +00:00

(Migrated from github.com)

@modmuss50 I reverted the toolchain change and added back the release option flag, though I added a new field for which targetCompatibility, sourceCompatibility and the release option flag use for the java version so when changed it changes everywhere and there's not 3 different fields to set. Hopefully that's better, if really I can scrap entirely that and just let the Fabric API modules comment.

modmuss50 (Migrated from github.com) reviewed 2021-02-12 11:55:55 +00:00

modmuss50 (Migrated from github.com) left a comment

This seems better, might be worth adding a comment on the targetJavaVersion to say what it does, and that java 8 is recomended for most if not all mods due to the vanilla launcher shipping 8 by default.

Not quite sure why everyline in the gradlew.bat file was changed?

This seems better, might be worth adding a comment on the targetJavaVersion to say what it does, and that java 8 is recomended for most if not all mods due to the vanilla launcher shipping 8 by default. Not quite sure why everyline in the gradlew.bat file was changed?

LambdAurora commented

2021-02-12 11:57:49 +00:00

(Migrated from github.com)

Not quite sure why everyline in the gradlew.bat file was changed?

I'm not sure either, it was changed when I ran gradle wrapper to update the gradle distributions.

> Not quite sure why everyline in the gradlew.bat file was changed? I'm not sure either, it was changed when I ran gradle wrapper to update the gradle distributions.

kb-1000 commented

2021-02-12 12:24:57 +00:00

(Migrated from github.com)

Not quite sure why everyline in the gradlew.bat file was changed?

I can give you a hint here: You've likely created the current version on windows, with the autocrlf feature on (converting it to lf) while lambdaurora likely isn't (or the other way round).
I've noticed this on all Fabric projects I've touched until now.
The fix is simple, just add this to .gitattributes:

*.bat text eol=crlf

That way you can avoid Git converting it at all.

> Not quite sure why everyline in the gradlew.bat file was changed? I can give you a hint here: You've likely created the current version on windows, with the autocrlf feature on (converting it to lf) while lambdaurora likely isn't (or the other way round). I've noticed this on all Fabric projects I've touched until now. The fix is simple, just add this to .gitattributes: ```gitattributes *.bat text eol=crlf ``` That way you can avoid Git converting it at all.

modmuss50 commented

2021-02-12 12:42:22 +00:00

(Migrated from github.com)

We need to be sure this actually works, knowing windows it might not.

kb-1000 commented

2021-02-12 12:51:29 +00:00

(Migrated from github.com)

A crlf .bat file is just fine. This line of .gitattributes served me well in other projects (although I also used some more, to enforce lf on the rest of the project).
What's quite interesting is that a lf .bat file (like you have right now, I think) is also fine with cmd.exe. ./gradlew wrapper generates crlf, though, I think it would be a good idea to put that into Git like that as well.

Also, you should probably set up .gitattributes to enforce lf in gradlew, *nix shells aren't as permissive as cmd.

A crlf .bat file is just fine. This line of `.gitattributes` served me well in other projects (although I also used some more, to enforce lf on the rest of the project). What's quite interesting is that a lf .bat file (like you have right now, I think) is _also_ fine with cmd.exe. `./gradlew wrapper` generates crlf, though, I think it would be a good idea to put that into Git like that as well. Also, you should probably set up .gitattributes to enforce lf in `gradlew`, *nix shells aren't as permissive as cmd.

liach commented

2021-06-02 23:57:45 +00:00

(Migrated from github.com)

so it appears that the toolchain from gradle is best used by ci. for ide users, this isn't too meaningful as they need to install proper java locally, and almost all mod devs code mods in ide and few need a fixed version of java in a ci. hence i don't recommend this change.

👍 1

This pull request has changes conflicting with the target branch.