Skip to content
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

Remove the insertAutoGeneratedEnumMappings gradle task #142

Open
zeichenreihe opened this issue Oct 21, 2023 · 1 comment
Open

Remove the insertAutoGeneratedEnumMappings gradle task #142

zeichenreihe opened this issue Oct 21, 2023 · 1 comment
Labels
buildscript Issue related to the buildscript enhancement New feature or request

Comments

@zeichenreihe
Copy link
Contributor

zeichenreihe commented Oct 21, 2023

The idea is to remove the insertAutoGeneratedEnumMappings task. This task currently runs stitchs CommandProposeV2FieldNames to generate names from the non-stripped enum variants (at least that's what I think it does). Removing this would require us to map these enum variants ourself. This is not hard, we already do this (I think).
In for example 1.12.2 the task only replaced 16, but detected 2184 "interesting names".
In 12w30e-client it replaced 0 names, and detected 130 "interesting names".

Therefore I think we should just remove that task and simpify the building process.

In the image you can see what would be replaced: the two red arrows to and the one from the insertAutoGeneratedEnumMappings task would be replaced with the green one that directly connects buildFeatherTiny with v2UnmergedFeatherJar.
image
Note: I plan to open a PR in the future containing that graph.

In terms of code changes:
The task to be removed is:

feather-mappings/build.gradle

Lines 1079 to 1098 in e28cce1

task insertAutoGeneratedEnumMappings(dependsOn: [buildFeatherTiny, mapCalamusJar], type: FileOutput) {
group = buildMappingGroup
def noEnumV2 = buildFeatherTiny.v2Output
output = new File(tempDir, "unmerged-named-v2-with-enum.tiny")
outputs.upToDateWhen { false }
doLast {
logger.lifecycle(":seeking auto-mappable fields for unmerged mappings")
String[] argsProposeV2 = [
calamusJar.getAbsolutePath(), // must use calamus jar
noEnumV2.getAbsolutePath(),
output.getAbsolutePath(),
"false" // don't replace existing names right now
]
new CommandProposeV2FieldNames().run(argsProposeV2)
}
}

This would require changes in the v2UnmergedFeatherJar task:

feather-mappings/build.gradle

Lines 1130 to 1140 in e28cce1

task v2UnmergedFeatherJar(dependsOn: insertAutoGeneratedEnumMappings, type: Jar) {
def mappings = insertAutoGeneratedEnumMappings.output
group = "mapping build"
outputs.upToDateWhen { false }
archiveFileName = "feather-${featherVersion}-v2.jar"
from(file(mappings)) {
rename mappings.name, "mappings/mappings.tiny"
}
destinationDirectory.set(file("build/libs"))
}

Mainly the dependsOn would need to change to buildFeatherTiny, and def mappings also needs to use buildFeatherTiny.

This issue is created to discuss how much this would break, since I do not currently know how dependant the mappings are on this feature.

for the future PR: (just using this as notes for later on)

  • Remove the insertAutoGeneratedEnumMappings gradle task and instead map these directly; see Remove the insertAutoGeneratedEnumMappings gradle task #142
    I do not yet know the impacts of this, see the issue for this.
    ... maybe there's more that could be cleaned up:
  • Why do we use tiny v1 in so many places? We should just switch all to tiny v2 and generate tiny v1 from them.
@zeichenreihe zeichenreihe added the enhancement New feature or request label Oct 21, 2023
@zeichenreihe
Copy link
Contributor Author

(showing off more of my graphs)

This shows the current tasks state:
tasks
This issue would do the following change:

  • In red: the task dependencies removal
  • In green: new task dependencies
  • And tasks with a white background get removed

tasks

@Copetan Copetan added the buildscript Issue related to the buildscript label Nov 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildscript Issue related to the buildscript enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants