-
-
Notifications
You must be signed in to change notification settings - Fork 445
[7.0] Add support for mavenizer output json, and expose mappings helper methods #1042
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Jonathing
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far. Losing focus since it's getting late at night so I'd like to wait until I can do a second pass tomorrow morning.
Please fix the typos I pointed out. There aren't many.
src/main/java/net/minecraftforge/gradle/internal/MavenizerInstanceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/net/minecraftforge/gradle/internal/MavenizerInstanceImpl.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public Provider<File> getToSrgFile() { | ||
| return getToSrg().map(this.extension.getProject()::file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.extension.getProject()::file makes this illegal to be called during task execution. If it's not used in our tasks, in the future it would be most prudent to turn these into properties, since they can be finalized at the end of configuration time without hassle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated on discord, this is intended to be used as inputs to tasks, not in the task body. So in theory this should be perfectly safe and if it isn't then its on the modder for using it wrong.
src/main/java/net/minecraftforge/gradle/internal/MavenizerInstanceImpl.java
Outdated
Show resolved
Hide resolved
| private Map<String, String> invoke() { | ||
| if (this.map == null) { | ||
| valueSource.get(); // Execute Mavenizer, probably called before, but just be sure. | ||
| this.map = (Map<String, String>) new JsonSlurper().parse(this.jsonFile, "UTF-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are using Gson in Mavenizer, we should prefer using it in here as well. This is fine for now, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didnt want to add extra deps to FG as we are not using GSON for anything else currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FG already bundles Gson as it is a transitive dependency of JSON Data Utils. But not a big deal, we can revisit this as needed.
|
Tempted to make MinecraftExtensionForProject extend MavenizerInstance to remove the need for the Functionally this works, just need to iterate the API as it exposes some that we need to be happy with.
|
|
I've made a couple of notable changes to this PR. Minor, but substantial.
These changes change your proposed API from this: renamer.classes(jar) {
mappings(minecraft.instance().toSrg.get())
}to this: renamer.classes(jar) {
mappings(minecraft.dependency.toSrg.get())
}or this: def minecraftDep = minecraft.dependency(libs.forge)
dependencies {
implementation(minecraftDep)
}
renamer.classes(jar) {
mappings(minecraftDep.toSrg.get())
} |
The initial design is to allow easier access to the mapping files for versions of minecraft that need renaming.
This is reliant on finalizing MinecraftForge/MinecraftMavenizer#15
Currently the json is just a Map<String, String> seems to work fine tho.
Changes the obfusication example from:
to
or
The names of everything is just a quick pass, needs javadocs, etc..
This is a proof of concept so point out anything major you see wrong.