Skip to content

Conversation

@WaitingIdly
Copy link
Contributor

changes in this PR:

  • create a new file, buildscript.properties, and move all gradle.properties only used in the build.gradle file into the new file.
    • load the options from this file in build.gradle.
    • the entries not moved are org.gradle.logging.stacktrace and org.gradle.jvmargs.
  • add comments describing the files and how to use them.
    • also comment two entries missing a comment, modName and modGroup.
    • adjust the comment for the modId, as it incorrectly stated that the it was "convention" to be all lowercase instead of a requirement, see FMLModContainer#sanityCheckModId throwing IllegalArgumentException, preventing it a mod from being loaded in ModContainerFactory#build.
  • adjust README.md and info/error messages that referenced the gradle.properties file to instead indicate the buildscript.properties file.

reasoning:

  • separates out the custom properties that arent used anywhere other than build.gradle into their own dedicated file, ensuring they dont pollute the global configs.
  • reduces possible confusion involving custom gradle properties (what settings are buildscript vs custom?) and associated clutter.
    • this is particularly relevant with GroovyScript, Universal Tweaks, and other mods like them.

potential concerns:

  • migrations are not needed for this change:
    • if the buildscript.properties file isnt found, and single error will be noted, but loading will continue.
    • as the entries from gradle.properties are loaded in every file, them not being moved doesnt cause any issues.
    • furthermore, actually making a migration to do this is problematic - converting the existing gradle.properties file is non-viable due to breaking custom gradle properties.
  • the "jump to source/uses" feature of intellij might not work properly. for me, it appears to only identify the source uses when they are inside a string.

PR review considerations:

  • adding instructions for how to migrate in order to get the benefit of this might be worthwhile.
  • it might be better to have a different name for buildscript.properties- ie build.properties.
  • the readme likely needs improvement.

@WaitingIdly WaitingIdly requested a review from a team as a code owner April 6, 2025 02:47
README.md Outdated
- Copy all the options from the [`gradle.properties`](https://github.com/GregTechCEu/Buildscripts/blob/master/gradle.properties) file to your `gradle.properties` (or copy entirely if you did not have one). You can leave your existing options if you know you need them, otherwise they can likely be removed
- Configure `gradle.properties` for your mod
- Copy the [`buildscript.properties`](https://github.com/GregTechCEu/Buildscripts/blob/master/buildscript.properties) file from this zip to your project
- Configure `buildscript.properties` for your mod
Copy link
Contributor

Choose a reason for hiding this comment

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

This now also could use a line about configuring gradle.properties.

@ALongStringOfNumbers ALongStringOfNumbers merged commit f4fb837 into GregTechCEu:master Jun 2, 2025
@WaitingIdly WaitingIdly deleted the separate-properties-file branch June 3, 2025 13:08
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.

2 participants