WIP 1.12 support #26
WIP 1.12 support #26
Conversation
| repos.maven(mvn -> { | ||
| mvn.setName("BlameJared Maven"); | ||
| mvn.setUrl("https://maven.blamejared.com/"); | ||
| }); |
There was a problem hiding this comment.
You should be able to safely remove this, as lwjgl3ify 1.12 is on gtceu maven, and mixinbooter+deps are on cleanroom maven
There was a problem hiding this comment.
JEI is on this maven, if we care about that at all for local testing
There was a problem hiding this comment.
These mavens should also just be enabled for 1.12, each additional maven adds additional risk for the plugin breaking builds if any of them go offline. Gradle fails the build if it tries a maven and the maven errors out, regardless if the artifact is present on other mavens.
There was a problem hiding this comment.
I moved Cleanroom and GTCEu maven's to only be included when the version is set for 1.12, and I also moved BlameJared repo to the well known repositories module. It's included by default if the version is 1.12, but can be added to the excluded repos in gradle.properties easily.
I think this is the only thing that makes sense for including the BlameJared repo as part of GTNHGradle, otherwise I'd say it would just be up to the individual project to put it in their repositories.gradle if we don't want it there.
| repos.maven(mvn -> { | ||
| mvn.setName("BlameJared Maven"); | ||
| mvn.setUrl("https://maven.blamejared.com/"); | ||
| }); |
There was a problem hiding this comment.
These mavens should also just be enabled for 1.12, each additional maven adds additional risk for the plugin breaking builds if any of them go offline. Gradle fails the build if it tries a maven and the maven errors out, regardless if the artifact is present on other mavens.
| String mixinSpec = ""; | ||
| String lwjgl3ifySpec = ""; | ||
|
|
||
| if (gtnh.configuration.minecraftVersion.equals("1.7.10")) { |
There was a problem hiding this comment.
Please make the 1.7.10 and 1.12.2 strings constants somewhere to prevent typos
There was a problem hiding this comment.
I added an enum for the MC versions, I went with TauMC's Archaic/Vintage naming, but that could be changed if we wanted it to be something else. The GTNHExtension object gets the minecraft version set on creation based on what the minecraftVersion property is
There was a problem hiding this comment.
I'd rather this be V1_7_10 and V1_12_2, archaic/vintage is too generic if we end up supporting 1.10/1.11 in the future for some reason
src/main/java/com/gtnewhorizons/gtnhgradle/UpdateableConstants.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
In PropertiesConfiguration, we need to update the Forge version and the mappings version. Or do we even need these properties entries, as they are handled by RFG directly? Unless having them be properties is to allow for customization of those versions.
In 1.12, the Forge version used is 14.23.5.2847, and the mappings version is Stable-39
We should not go above this Forge version to the newer versions, because those are published versions with Forge's attempts to transition to FG3, which didn't really work at all.
Related to that, we may need to add a project to remove newer Forge version deps on projects, so that they can be used in dev environment, or implment it ourselves. See https://github.com/GregTechCEu/Buildscripts/blob/master/gradle.properties#L96-L100
The remoteMappings entry should be left blank for 1.12, as our mappings are mostly complete, but also we don't want to pull in those 1.7 mappings and attempt to apply them to 1.12
There is a deprecated properties option that could just be removed, at least for 1.12, so that it is not introduced deprecated. See replaceGradleTokenInFile.
Other property options, gradleTokenModId, gradleTokenModName, are marked as deprecated, but are supported at least in the 1.12 CEu buildscript. However, they could be safe to remove, as we don't really use them ourselves, and if someone wants them, they can always be added back manually into the generated tags file.
Going back to the AT thing that was discussed on discord, you are using space separated, while we offered comma separated. It shouldn't be too hard to support either though, or we can change to one separation format. There is still the moving of the AT file difference though.
We offer a property to have separate run directories for the client and the server, using the workingDir option. Right now in IdeIntegrationModule, both the client and the server run tasks are just using the default RFG defined working dir of /run, It could be nice to add an option there to separate the possible working directories for the client and the server tasks. EG, we provided /run/Client and /run/Server when the property was enabled.
In comments for Modrinth and CF publishing, it is stated that GTNH Mixins/UniMix is automatically set as a dependency if useMixins is enabled. This should be updated for 1.12 to be MixinBooter, and comments updated
In MixinModule, we offered a property to customize the name of the default mixin file. We used the same default location as your provided location, but some customization was allowed, see https://github.com/GregTechCEu/Buildscripts/blob/master/gradle.properties#L69-L70
| public static final String NEWEST_UNIMIXINS = "io.github.legacymoddingmc:unimixins:0.1.18"; | ||
|
|
||
| /** Latest version of MixinBooter for 1.12.2 */ | ||
| public static final String NEWEST_MIXINBOOTER = "zone.rong:mixinbooter:9.3"; |
Because of the forge 2848+ issues, I think the best approach would be to not expose a forge/mapping version here, and only support 2847 and stable_39. However this isn't mandatory.
I would rather keep these to not break any scripts relying on them.
If this is referring to the format in the gradle properties, I think again it's best to keep as consistent with the original script as possible to reduce the needed changes when users update. If overall many breaks are needed, we could instead do a "2.0" version of the script using the plugin, though I would like to avoid this since it would likely drastically reduce adoption of this new work. Additionally, the 1.12 script includes a changelogger gradle task that would be great to include since I know many people rely on it. |
does the needful