Modernize Gradle buildscript #71

2020-11-02T00:06:53Z

zml2008 commented

2020-11-02 00:06:53 +00:00

(Migrated from github.com)

This PR does a few things to modernize the buildscript and protect mod developers from accidental mistakes:

Applies the release compiler flag where possible to more accurately compile for Java 8. When compiling on newer Java versions, this makes using API not available in Java 8 a compile error.
Use gradle's built-in feature to create the sources jar
Attach the project name to the license file name, to avoid cases where the license could be ambiguous due to multiple licenses in the jar (when shading, for example)
Remove the line for publishing to maven local (it's unnecessary -- the pTML task exists already), and replace it with an example for an actual repository
Update the example fabric.mod.json's suggests object to use a mod name that isn't a mod that actually exists for Fabric

I've also bumped dependency versions while I was stopping by.

This PR does a few things to modernize the buildscript and protect mod developers from accidental mistakes: - Applies the `release` compiler flag where possible to more accurately compile for Java 8. When compiling on newer Java versions, this makes using API not available in Java 8 a compile error. - Use gradle's built-in feature to create the sources jar - Attach the project name to the license file name, to avoid cases where the license could be ambiguous due to multiple licenses in the jar (when shading, for example) - Remove the line for publishing to maven local (it's unnecessary -- the `pTML` task exists already), and replace it with an example for an actual repository - Update the example `fabric.mod.json`'s `suggests` object to use a mod name that isn't a [mod that actually exists for Fabric](https://github.com/FaeCraft/Flamingo/blob/1.16-fabric/src/main/resources/fabric.mod.json#L3) I've also bumped dependency versions while I was stopping by.

dexman545 commented

2020-11-02 01:49:49 +00:00

(Migrated from github.com)

I don't think changing the suggests block is worthwhile, all you've done is create a mod with the id another-mod. Flamingo is at least a fairly small and harmless, and obviously joke-like, mod.

I don't think changing the `suggests` block is worthwhile, all you've done is create a mod with the id `another-mod`. Flamingo is at least a fairly small and harmless, and obviously joke-like, mod.

👍 1 👎 1

zml2008 commented

2020-11-02 03:27:02 +00:00

(Migrated from github.com)

Let's not talk about other people's mods like that :p

dexman545 commented

2020-11-02 03:30:44 +00:00

(Migrated from github.com)

The point still stands: you're just playing cat and mouse with mod ids, and I'd rather it stay with a harmless mod than be opened to something potentially malicious. Have it suggest minecraft or a fapi module if you don't like flamigo.

modmuss50 (Migrated from github.com) reviewed 2020-11-02 09:59:24 +00:00

src/main/resources/fabric.mod.json

						
				@ -35,3 +35,3 @@

				    "flamingo": "*"

				    "another-mod": "*"

				  }

				}

modmuss50 (Migrated from github.com) commented

2020-11-02 09:59:24 +00:00

I would either keep this as flamingo or remote it all togeather.

I dont think many people use the suggests block, and there are 3 good examples on how to use the depends block so removing it seems fine. As good as flamingo is, its a bit of an in joke for the people that know about it, others it may just confuse.

I would either keep this as flamingo or remote it all togeather. I dont think many people use the suggests block, and there are 3 good examples on how to use the depends block so removing it seems fine. As good as flamingo is, its a bit of an in joke for the people that know about it, others it may just confuse.

YTG1234 (Migrated from github.com) approved these changes 2020-11-02 10:04:34 +00:00

modmuss50 (Migrated from github.com) requested changes 2020-11-02 10:59:39 +00:00

.github/workflows/build.yml

						
				@ -0,0 +18,4 @@

				        ]

				        # and run on both Linux and Windows

				        os: [ubuntu-20.04, windows-latest]

				    runs-on: ${{ matrix.os }}

modmuss50 (Migrated from github.com) commented

2020-11-02 10:56:52 +00:00

I would just do ubuntu only, I dont think building on windows provides any benefit. Building on windows would double the amount of stuff to download, and increase the amount github has to build (I know its free, but dont want to wase it for the sake of it)

.github/workflows/build.yml

						
				@ -0,0 +30,4 @@

				          java-version: ${{ matrix.java }}

				      - name: make gradle wrapper executable

				        if: ${{ runner.os != 'Windows' }}

				        run: chmod +x ./gradlew

modmuss50 (Migrated from github.com) commented

2020-11-02 10:57:15 +00:00

This shouldnt be needed, it should be executable on the repo.

.github/workflows/build.yml

						
				@ -0,0 +32,4 @@

				        if: ${{ runner.os != 'Windows' }}

				        run: chmod +x ./gradlew

				      - name: build

				        run: ./gradlew build

modmuss50 (Migrated from github.com) commented

2020-11-02 10:59:18 +00:00

Saving the build artifacts after this might not be a bad idea. Make sure to only do it on one java version

.github/workflows/validate-gradle-wrapper.yml

modmuss50 (Migrated from github.com) commented

2020-11-02 10:55:34 +00:00

Id just move this into the other workflow

build.gradle

modmuss50 (Migrated from github.com) commented

2020-11-02 10:04:42 +00:00

This + 1 is a little odd looking, just hard coding it to 8 might be clearer

build.gradle

						
				@ -51,3 +60,4 @@

					}

				}

				// configure the maven publication

modmuss50 (Migrated from github.com) commented

2020-11-02 10:03:12 +00:00

This is great, will need to add this to our other projects.

build.gradle

						
				@ -64,9 +74,9 @@ publishing {

						}

modmuss50 (Migrated from github.com) commented

2020-11-02 10:02:02 +00:00

I dont think this is really needed, its another thing to confuse new develeops. Very few people publish to maven, and if so there are good docs online? Possbly just link to https://docs.gradle.org/current/userguide/publishing_maven.html

JamiesWhiteShirt (Migrated from github.com) approved these changes 2020-11-02 16:15:08 +00:00

.github/workflows/build.yml

						
				@ -0,0 +30,4 @@

				          java-version: ${{ matrix.java }}

				      - name: make gradle wrapper executable

				        if: ${{ runner.os != 'Windows' }}

				        run: chmod +x ./gradlew

JamiesWhiteShirt (Migrated from github.com) commented

2020-11-02 16:12:49 +00:00

Depending on the method used to replicate this repo, the file mode may be lost. I don't think it should be removed.

Juuxel (Migrated from github.com) reviewed 2020-11-02 17:02:55 +00:00

build.gradle

						
				@ -70,3 +80,3 @@

						// mavenLocal()

						// See https://docs.gradle.org/current/userguide/publishing_maven.html for information on how to set up publishing.

					}

				}

Juuxel (Migrated from github.com) commented

2020-11-02 17:02:55 +00:00

Does this work for sources in other source directories than java, like Kotlin sources?

Does this work for sources in other source directories than `java`, like Kotlin sources?

Juuxel (Migrated from github.com) reviewed 2020-11-02 17:05:14 +00:00

build.gradle

Juuxel (Migrated from github.com) commented

2020-11-02 17:05:14 +00:00

This is not that important, but this can cause problems if a project (most likely a library in this case) doesn't set the project name in gradle.properties. If it's built using Jitpack (for example for jij'ing), the file will be LICENSE_build as Gradle guesses the project name based on the directory, which Jitpack calls build.

This is not that important, but this can cause problems if a project (most likely a library in this case) doesn't set the project name in gradle.properties. If it's built using Jitpack (for example for jij'ing), the file will be `LICENSE_build` as Gradle guesses the project name based on the directory, which Jitpack calls `build`.

zml2008 (Migrated from github.com) reviewed 2020-11-03 05:21:39 +00:00

build.gradle

						
				@ -70,3 +80,3 @@

						// mavenLocal()

						// See https://docs.gradle.org/current/userguide/publishing_maven.html for information on how to set up publishing.

					}

				}

zml2008 (Migrated from github.com) commented

2020-11-03 05:21:39 +00:00

It does. withSourcesJar() collects its source from SourceSet.getAllSource()

It does. `withSourcesJar()` [collects its source from `SourceSet.getAllSource()`](https://github.com/gradle/gradle/blob/a24197ee1f665acd9ed93f345eb5b7f788385151/subprojects/plugins/src/main/java/org/gradle/api/plugins/internal/DefaultJavaPluginExtension.java#L125)

zml2008 (Migrated from github.com) reviewed 2020-11-03 05:34:43 +00:00

build.gradle

zml2008 (Migrated from github.com) commented

2020-11-03 05:34:42 +00:00

ah -- I usually set the project name in settings.gradle. I might update this PR to skip setting archivesBaseName and instead just set the project name explicitly.

ah -- I usually set the project name in `settings.gradle`. I might update this PR to skip setting `archivesBaseName` and instead just set the project name explicitly.

zml2008 (Migrated from github.com) reviewed 2020-11-03 05:41:09 +00:00

.github/workflows/build.yml

						
				@ -0,0 +30,4 @@

				          java-version: ${{ matrix.java }}

				      - name: make gradle wrapper executable

				        if: ${{ runner.os != 'Windows' }}

				        run: chmod +x ./gradlew

zml2008 (Migrated from github.com) commented

2020-11-03 05:41:09 +00:00

Yeah, windows generally doesn't preserve file permissions -- I've had it happen a few times before when setting up gradle wrapper.

There's no harm to keeping it there -- it'll basically be a no-op if the repo is set up correctly, but removes one more thing that could go wrong.

Yeah, windows generally doesn't preserve file permissions -- I've had it happen a few times before when setting up gradle wrapper. There's no harm to keeping it there -- it'll basically be a no-op if the repo is set up correctly, but removes one more thing that could go wrong.

zml2008 (Migrated from github.com) reviewed 2020-11-03 05:44:15 +00:00

build.gradle

zml2008 (Migrated from github.com) commented

2020-11-03 05:44:15 +00:00

Yeah, it's a bit of an unfortunate workaround to the fact that JavaVersion.getMajorVersion() returns a String of the same value, and not an int. If only Gradle didn't have 3 separate ways to represent a Java version in different parts of its API....

Can probably avoid it though by declaring targetVersion as an int, and converting that to JavaVersion for the pre-Java9 variant.

Yeah, it's a bit of an unfortunate workaround to the fact that [`JavaVersion.getMajorVersion()`](https://github.com/gradle/gradle/blob/46a0779dabbd2d7518cc6ae1bcae1030d4ea1100/subprojects/base-services/src/main/java/org/gradle/api/JavaVersion.java#L269) returns a String of the same value, and not an int. If only Gradle didn't have 3 separate ways to represent a Java version in different parts of its API.... Can probably avoid it though by declaring `targetVersion` as an int, and converting that to `JavaVersion` for the pre-Java9 variant.

zml2008 (Migrated from github.com) reviewed 2020-11-03 05:58:05 +00:00

.github/workflows/build.yml

						
				@ -0,0 +18,4 @@

				        ]

				        # and run on both Linux and Windows

				        os: [ubuntu-20.04, windows-latest]

				    runs-on: ${{ matrix.os }}

zml2008 (Migrated from github.com) commented

2020-11-03 05:58:05 +00:00

I've run into platform-specific issues before, in my own projects and what I've seen in some other repos:

checkstyle doesn't normalize path separators in its suppressions file, so that can end up only working on one OS
some parts of path handling differ, between different line separators and handling of symlinks -- though these would only really be caught by unit tests
When developing on a case-insensitive filesystem (like windows has), renaming a class or folder in a way that only changes its case will not be captured by git.

These can be caught by running CI builds on multiple operating systems.

I've run into platform-specific issues before, in my own projects and what I've seen in some other repos: - checkstyle doesn't normalize path separators in its suppressions file, so that can end up only working on one OS - some parts of path handling differ, between different line separators and handling of symlinks -- though these would only really be caught by unit tests - When developing on a case-insensitive filesystem (like windows has), renaming a class or folder in a way that only changes its case will not be captured by `git`. These can be caught by running CI builds on multiple operating systems.

zml2008 (Migrated from github.com) reviewed 2020-11-03 06:08:22 +00:00

src/main/resources/fabric.mod.json

						
				@ -35,3 +35,3 @@

				    "flamingo": "*"

				    "another-mod": "*"

				  }

				}

zml2008 (Migrated from github.com) commented

2020-11-03 06:08:21 +00:00

will remove... if only JSON allowed comments, would be a good way to explain some things inline

modmuss50 (Migrated from github.com) approved these changes 2020-11-10 22:11:45 +00:00

altrisi (Migrated from github.com) reviewed 2020-11-15 11:58:19 +00:00

src/main/resources/fabric.mod.json

						
				@ -35,3 +35,3 @@

				    "flamingo": "*"

				    "another-mod": "*"

				  }

				}

altrisi (Migrated from github.com) commented

2020-11-15 11:58:19 +00:00

This ended up being merged with another-mod instead of flamingo or nothing...

This ended up being merged with `another-mod` instead of `flamingo` or nothing...

Sign in to join this conversation.

No reviewers

YTG1234

JamiesWhiteShirt

modmuss50

No Label

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Steven/fabric-example-mod#71