Skip to content

Conversation

@TheDGOfficial
Copy link

setup-gradle adds persistent gradle cache for faster CI builds, build summary and does wrapper validation as part of its lifecycle. https://github.com/gradle/actions?tab=readme-ov-file#the-wrapper-validation-action

setup-gradle adds persistent gradle cache for faster CI builds, build summary and does wrapper validation as part of its lifecycle. https://github.com/gradle/actions?tab=readme-ov-file#the-wrapper-validation-action
Comment on lines +20 to +21
- name: setup gradle
uses: gradle/actions/setup-gradle@v5
Copy link
Member

Choose a reason for hiding this comment

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

I dont see why this is needed, I dont think the caching it does by default is a great idea for us to include in the example mod. There are some serious secuirty considerations that need to be made when using any form of caching with github actions, this isnt a choice I want to make for others.

Copy link
Author

Choose a reason for hiding this comment

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

From the gradle/actions repository's README at the very top: "The setup-gradle action can be used to configure Gradle for optimal execution on any platform supported by GitHub Actions."

In my opinion, the template mod should ship an optimal configuration by default.

Gradle already performs encryption on the relevant parts of the cache per my knowledge.

See https://github.com/gradle/actions/blob/main/docs/setup-gradle.md#why-use-the-setup-gradle-action for more information. This section lists advantages over using setup-java's cache: gradle option (which is also not used here), or custom configurations with actions/cache.

The current behaviour wastes resources by unnecessarily throwing away the already built caches and repeatedly downloading a gradle distribution, fetching dependencies, configuring and running tasks each workflow run, even if nothing has been changed. This not only wastes resources of the runner, but also the repositories serving the dependencies, and potentially delays development while you wait for the workflow to finish.

Copy link
Author

Choose a reason for hiding this comment

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

actions/setup-java apparently also recommends setup-gradle now for caching. See actions/setup-java#588

The official GitHub document about "Building and testing Java with Gradle" also seems to recommend setup-gradle. See https://docs.github.com/en/actions/tutorials/build-and-test-code/java-with-gradle#using-a-gradle-workflow-template - step 8 explains the purpose of setup-gradle: "Sets up the Gradle environment. The gradle/actions/setup-gradle action takes care of caching state between workflow runs, and provides a detailed summary of all Gradle executions."

As someone that got into Fabric modding recently and did not fiddle with the default actions configuration much, discovering it didn't use setup-gradle was suprising. Once switched over to it, there was tremendous reductions in build times in actions (from 13 minutes to 2 minutes for a 3 version multi-version build), so I decided to make a PR to include it in the template. However, if others do not agree I'm willing to close this PR, but maybe it should at least have a note like "# If you get long actions build times, consider switching to setup-gradle" or something, as a reference point for when people's mods get bigger or build for multiple versions. It does not do any harm for single version or small mods per my testing though so I'd prefer if it was the default starting point.

The security concerns might be valid, but in that case I don't think setup-java or official GitHub documents would also recommend the action. It does not reside under GitHub maintained standard actions like actions/checkout or actions/setup-java but rather gradle/setup-gradle, but I don't think that's relevant as the current default workflow also uses gradle/wrapper-validation (wrapper-validation is included as part of setup-gradle implicitly, so it is not needed with it, to clarify again)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants