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

gradle cleanup #645

Open
wants to merge 90 commits into
base: 24w45a
Choose a base branch
from

Conversation

supersaiyansubtlety
Copy link
Contributor

@supersaiyansubtlety supersaiyansubtlety commented Sep 28, 2024

Ready for review to start, but still WIP because some questions remain

Remaining questions:

Tasks

  • the thisening
  • the finalening
  • the line wrappening
  • the abstractening
  • the registerening
  • move all custom tasks and (most of) their configuration to QuiltMappingsPlugin
  • move (most) task configuration to QuiltMappingsPlugin (makes it easier to see dependencies and makes tasks more reusable)
  • eliminate buildSrc referencing build.gradle things
  • eliminate project access in task actions
  • refine inputs/outputs and dependencies
  • move more things to QuiltMappingsExtension
  • separate QuiltMappingsPlugin's contents into several plugins
  • wait for add a test to make sure broken unpick definitions error #659 which fixes tests, make sure tests still pass
  • resolve a few remaining questions
  • replace TasksTest and depend on gradle test kit maybe in a followup PR
  • apply a checkstyle to buildSrc? maybe in a followup PR

Summary

renames MappingsPlugin/mappings-logic -> QuiltMappingsPlugin/quilt-mappings
renames MappingssExtension/mappings -> QuiltMappingsExtension/quiltMappings
splits QuiltMappingsPlugin into:

  • QuiltMappingsBasePlugin
  • MinecraftJarsPlugin
  • MapMinecraftJarsPlugin
  • MapV2Plugin
  • MapIntermediaryPlugin
  • ProcessMappingsPlugin
  • EnigmaMappingsPlugin
  • TargetDiffPlugin

all implement MappingsProjectPlugin and are applied by QuiltMappingsPlugin

eliminates FileConstants:

  • some common directories are in MappingsProjectPlugin
  • files that were the output of one task are now defined where that task's output is set and other tasks take that output as input

adds several extensions that expose objects created by their plugins
extensions are accessed via plugin instances which ensures extension have been created

no use of tasks.named(...) to access custom tasks:

  • instead they're accessed via the provider returned by tasks.register(...)
  • tasks used outside of the plugin they're defined in are accessed via Tasks objects obtained via extensions, obtained via plugin instances
  • this makes sure required plugins are applied and the tasks are registered

ensures all custom tasks have a group
eliminates MappingsTask and DefaultMappingsTask
(some MappingsTask methods are replaced with helper methods in more specific task interfaces and plugins)

moves Constants.MINECRAFT_VERSION to libs.versions.toml and passes it to QuiltMappingsExtension
moves Constants.MAPPINGS_VERSION to direct project.version assignment in build.gradle and passes it to QuiltMappingsExtension
moves dictionary file url to libs.versions.toml and downloads it with an ivy repository hack
eliminates downloadDictionaryFile and passes the dictionary file to mappingLint in build.gradle
saves unpickVersion in a QuiltMappingsEntension field and passes it to the constructors of tasks that require it
(it's required at configuration time for some tasks so it can't be a property)

extracts a bunch of magic strings to interfaces in constants package
moves all task names to their classes and renames all TASK_NAME fields <ACTUAL_NAME>_TASK_NAME
adds javadocs to all task names that link the plugin that registers them
adds javadocs to each plugin with any configurEaches listing what they do (these can be annoying to track down)
adds javadocs to each class that's configureEached and to all their subclasses, linking the plugin that does the configuring (these can be really annoying to track down)

moves almost all outputs to build/*
moves combineUnpickDefinitions's output to build/mappings/
eliminates custom clean task logic (everything is in build/ and gets cleaned)

names AbstractArchiveTask outputs using its name composition methods
adds ArtifactFileProducingTask which mimics AbstractArchiveTask name composition (implemented by CompressTinyTask)
eliminates explicit classifiers for published artifacts in build.gralde

mapped providers are the solution to every problem

moves unpick.json to unpick/ and unpick definitions to unpick/definitions/
moves enimga_profile.json and simple_type_field_names.json5 to enigma/

eliminates decompileClasspath configuration -> passes downloadMinecraftLibraries.librariesDir.fileTree
replaces CheckStuffExists/Matches tasks with mapped providers

populates unpickCli configuration using included build :unpick-holders
splits enigmaRuntime configuration into enigmaSwing and enigmaServer, populates them using :unpick-holders

replaces de.undercouch.gradle.tasks.download.DownloadAction usage with DownloadUtil methods (which use org.apache.commons.io.FileUtils::copyURLToFile)
eliminates de.undercouch:gradle-download-task dependency

adds generateDiff task (Windows users may have to put diff on their PATH)

Future

  • use generateDiff in gerenate-diff.yml (pass output path to avoid magic string)
  • move namedSource/ and namedTargetSource/once generateDiff is used in gerenate-diff.yml
  • update/replace licenser plugin: the last release is from 2021 and it's incompatible with gradle 9

@supersaiyansubtlety supersaiyansubtlety added enhancement new feature or request t: toolchain changes to the quilt mappings toolchain outdated this pull request hasn't been updated to the latest version or has merge conflicts wip this is a work in progress labels Sep 28, 2024
@supersaiyansubtlety supersaiyansubtlety self-assigned this Sep 28, 2024
@supersaiyansubtlety
Copy link
Contributor Author

supersaiyansubtlety commented Sep 28, 2024

Early reviewers beware: I will probably be rewriting history so any review progress on github may be lost.
Ready-ish

@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Oct 1, 2024
Copy link
Contributor

github-actions bot commented Oct 1, 2024

🚨 Target branch is already set to 24w39a

@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Oct 1, 2024
@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Oct 3, 2024
@github-actions github-actions bot changed the base branch from 24w39a to 24w40a October 3, 2024 00:36
Copy link
Contributor

github-actions bot commented Oct 3, 2024

🚀 Target branch has been updated to 24w40a

Copy link
Contributor

github-actions bot commented Oct 3, 2024

🚨 Please fix merge conflicts before this can be merged

@github-actions github-actions bot added v: snapshot targets a snapshot version of minecraft and removed update-base used to notify github actions that the base branch should be updated labels Oct 3, 2024
Copy link

@lukebemish lukebemish left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Unsure if the eventual aim is config cache here (and maybe it ought to be, as config cache will be on by default in gradle 9 which is on the horizon) -- regardless, if so there's a bit of work to do, since a lot of stuff is still kinda unidiomatic in that regard; all things told though a drastic improvement. Didn't comment on any remaining TODOs.

@supersaiyansubtlety supersaiyansubtlety removed the outdated this pull request hasn't been updated to the latest version or has merge conflicts label Oct 4, 2024
@supersaiyansubtlety
Copy link
Contributor Author

supersaiyansubtlety commented Oct 4, 2024

Note: Rather than marking this PR as ready for review, when it's ready I'll be closing it and creating a new one.
That will clean up the messy history here on GitHub caused by rewriting history.
Never mind, it cleaned itself up, I underestimated GitHub.

@supersaiyansubtlety supersaiyansubtlety added the s: large PRs with more than 700 lines label Oct 4, 2024
@supersaiyansubtlety supersaiyansubtlety force-pushed the procrastination+gradle-cleanup branch 2 times, most recently from 3e74240 to c6f2f85 Compare October 8, 2024 05:26
@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Oct 9, 2024
Copy link
Contributor

github-actions bot commented Oct 9, 2024

🚀 Target branch has been updated to 1.21.2-pre1

@github-actions github-actions bot changed the base branch from 24w40a to 1.21.2-pre1 October 9, 2024 00:46
@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Oct 9, 2024
@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Oct 10, 2024
@github-actions github-actions bot changed the base branch from 1.21.2-pre1 to 1.21.2-pre2 October 10, 2024 16:53
Copy link
Contributor

🚀 Target branch has been updated to 1.21.2-pre2

@supersaiyansubtlety supersaiyansubtlety added v: release targets a release version of minecraft and removed v: snapshot targets a snapshot version of minecraft labels Oct 31, 2024
@supersaiyansubtlety
Copy link
Contributor Author

Re: Configuration cache

I tried running some tasks with --configuration-cache and got:

Configuration cache state could not be cached: field pendingMap of org.gradle.api.internal.DefaultNamedDomainObjectCollection$UnfilteredIndex bean found in field delegate of org.gradle.api.internal.DefaultNamedDomainObjectCollection$FilteredIndex bean found in field index of org.gradle.api.internal.tasks.DefaultTaskCollection bean found in field delegate of org.gradle.api.internal.tasks.DefaultRealizableTaskCollection bean found in input property $1 of task :buildMappingsTiny of type quilt.internal.task.build.BuildMappingsTinyTask: error writing value of type 'java.util.LinkedHashMap'
java.util.ConcurrentModificationException (no error message)

That's for buildMappingsTiny, I got a similar error mentioning error writing value of type 'java.util.LinkedHashMap' for findDuplicateMappingFiles.

idk how to fix these because idk where the LinkedHashMap comes from.

If it turns out it wouldn't take too many changes to fix config caching I'm ok with including it in this PR, otherwise it'll have to go in a followup PR.

@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Nov 5, 2024
Copy link
Contributor

github-actions bot commented Nov 5, 2024

🚀 Target branch has been updated to 24w44a

@github-actions github-actions bot changed the base branch from 1.21.3 to 24w44a November 5, 2024 22:59
Copy link
Contributor

github-actions bot commented Nov 5, 2024

🚨 Please fix merge conflicts before this can be merged

@github-actions github-actions bot added v: snapshot targets a snapshot version of minecraft outdated this pull request hasn't been updated to the latest version or has merge conflicts and removed update-base used to notify github actions that the base branch should be updated v: release targets a release version of minecraft labels Nov 5, 2024
@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Nov 7, 2024
Copy link
Contributor

github-actions bot commented Nov 7, 2024

🚀 Target branch has been updated to 24w45a

@github-actions github-actions bot changed the base branch from 24w44a to 24w45a November 7, 2024 02:46
Copy link
Contributor

github-actions bot commented Nov 7, 2024

🚨 Please fix merge conflicts before this can be merged

@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Nov 7, 2024
@ix0rai
Copy link
Member

ix0rai commented Nov 9, 2024

is there anything that remains to be done on this now that #659 is merged?

@supersaiyansubtlety
Copy link
Contributor Author

supersaiyansubtlety commented Nov 9, 2024

is there anything that remains to be done on this now that #659 is merged?

It's not waiting on any other PRs, but I need to merge changes and make sure tests work. done

There are also a few unresolved questions above.

@supersaiyansubtlety supersaiyansubtlety removed the outdated this pull request hasn't been updated to the latest version or has merge conflicts label Nov 11, 2024
…ectPlugin with DefaultExtensionedMappingsProjectPlugin + Default/TaskedExtension

access Tasks objects and MapIntermediaryPlugin's intermediaryFile via extension instances obtained via plugin instances instead of directly via plugin instances
Copy link
Member

@OroArmor OroArmor left a comment

Choose a reason for hiding this comment

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

Honestly too big to properly review now. From my previous looks its going great. I trust that you did it correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new feature or request s: large PRs with more than 700 lines t: toolchain changes to the quilt mappings toolchain v: snapshot targets a snapshot version of minecraft wip this is a work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants