[DRAFT] Adding transformer support to be able to use remapped dependencies in any configuration. #156

marchermans · 2019-11-30T21:53:19Z

Current situation

In the current situation a custom configuration is used to adapt and remap the mod dependencies to the mapping as defined by the user.
Even though this works as intended it is not entirely compatible with Gradle spec.

New situation

This implements transformers that interact with all dependencies added to the project and will transform them accordingly.
The transformers call the old methods, which where previously looping over a configuration.
Each transformer will initially detect if the artifact passed in a valid Fabric Mod before performing either a remap of a library jar, or a sources jar.

… laptop.

…ld code but adapted.

…ied by the transformer, or use none if a dependency is being extracted.

liach · 2019-11-30T22:07:09Z

must you get the project in such nasty ways?

marchermans · 2019-12-01T07:46:53Z

@liach Sadly yes, gradle uses a custom injector system to inject data into the transformers itself, as such what can be injected is extremely limited. (Basically only its inputs). Additionally Transformers need to perform a pure action and their parameters need to be isolateble. In this context that means that the entire parameter set needs to be serializable.

In the current context this means that to get the external data (the data needed to remap, for example the MC dependency or the mapping data) from the project config there is literally only one way.

That said I am currently looking into a technology stack which would pass all of that information through to us without having to use the project at all. But yeah as of right now this PR also needs an update of TinyRemapper since that can not handle being uses in a multithreaded environment. And just crashes. Hence it being a draft as well.

marchermans · 2019-12-01T09:06:54Z

For everybody reading this: This PR needs a bug fix in TinyRemapper to be usefull: FabricMC/tiny-remapper#7 is the hanging issue.

natanfudge

remember to rebase

natanfudge · 2019-12-01T12:48:50Z

src/main/java/net/fabricmc/loom/transformers/CompiledJarRemappingTransformer.java

+
+		//Rudimentary check for sources. Which need to be remapped separately.
+        //Potentially we need to check zip file contents.
+		if (inputFile.getName().contains("-sources"))


Considering this one and the sources jar one are extremely similiar, I think some common code can be extracted.

Yep totally agree.
Some lambda logic and a builder would probably go a long way.
But I would love to write this up a bit better so that it works in nearly all cases perfectly before I go mayham on cleaning loom up and making it more stable.

natanfudge · 2019-12-01T12:49:17Z

src/main/java/net/fabricmc/loom/transformers/TransformerProjectManager.java

+
+public class TransformerProjectManager
+{
+    private static final TransformerProjectManager INSTANCE = new TransformerProjectManager();


add a private constructor if you plan on keeping this

natanfudge · 2019-12-01T12:49:34Z

src/main/java/net/fabricmc/loom/transformers/TransformerProjectManager.java

+    private Map<String, Project> projects = Maps.newConcurrentMap();
+
+    public void register(Project project)
+    {


wrong formatting

natanfudge · 2019-12-01T12:49:51Z

src/main/java/net/fabricmc/loom/util/FabricModUtils.java

+{
+
+    private FabricModUtils()
+    {


natanfudge · 2019-12-01T12:50:54Z

src/main/java/net/fabricmc/loom/util/FabricModUtils.java

+     */
+    public static boolean isFabricMod(Project project, Logger logger, File artifact, String notation) {
+        AtomicBoolean fabricMod = new AtomicBoolean(false);
+        project.zipTree(artifact).visit(f -> {


Naming this lambda param file would be beneficial in my opinion

Stop using project here. Use java 8 zip file sysstem api or zip entry. also for project logger use sys out println instead

natanfudge · 2019-12-01T12:53:03Z

src/main/java/net/fabricmc/loom/util/InstallerUtils.java

+            JsonObject jsonObject = GSON.fromJson(jsonStr, JsonObject.class);
+            return jsonObject;
+        } catch (IOException e) {
+            e.printStackTrace();


Might want something better than this

Again original code does this. Will fix it.

natanfudge · 2019-12-01T12:53:24Z

src/main/java/net/fabricmc/loom/util/InstallerUtils.java

+        throw new IllegalStateException("Tried to initialize: InstallerUtils but this is a Utility class.");
+    }
+
+    static JsonObject readInstallerJson(File file, Project project) {


Is this intentionally package private?

It's package private in ModProcessor so is probably just a holdover from copy-pasting

Yep.
InstallerUtils comes from the ModProcessor class, so yeah I will need to give These Kinds of Things a brush over.

natanfudge · 2019-12-01T12:55:56Z

src/main/java/net/fabricmc/loom/util/InstallerUtils.java

+            int priority = 0;
+
+            try (JarFile jarFile = new JarFile(file)) {
+                ZipEntry entry = null;


I don't like how much mutable state is in this code, but I don't see an easy way around it so don't worry about it too much.
Also formatting.

The way we do this is really nasty. TBH The installer dependencies should not be retroactively injected in the classpath based on some weird spec nobody knows.

I will see what I can do to propose a Change here in the future, then this will probably Change as well.

natanfudge · 2019-12-01T13:06:48Z

src/main/java/net/fabricmc/loom/util/ModProcessor.java


-		File nestedFile = new File(extension.getNestedModCache(), fileName.substring(fileName.lastIndexOf("/")));
+            for (int i = 0; i < jsonArray.size(); i++) {


can't you use a for : (foreach) loop here?

It would iterate over JsonElements rather than JsonObjects so it's hardly much better

It would have the same benefits as in any other for loop,
JsonObject jsonObject = jsonElement.getAsJsonObject();

TBH This is original code, i did not change this. But I plan to go over all of this and clean this kind of stuff up in an upcoming PR later on. This will probably stay in this PR, getting this to behave is already kind of difficult.

natanfudge · 2019-12-01T13:08:19Z

src/main/java/net/fabricmc/loom/util/SourceUtils.java

+
+    public static File findSources(DependencyHandler dependencies, ResolvedArtifact artifact) {
+        @SuppressWarnings("unchecked") ArtifactResolutionQuery query = dependencies.createArtifactResolutionQuery()//
+                                                                                       .forComponents(artifact.getId().getComponentIdentifier())//


IDK why but the formatting here looks awful on github

Chocohead · 2019-12-01T23:03:15Z

src/main/java/net/fabricmc/loom/util/GradleSupport.java

-
-	private static RegularFileProperty getfilePropertyLegacy(Project project) {
-		return project.getLayout().fileProperty();
+		return project.getObjects().fileProperty();


This class could be removed completely if there's no backwards compatibility being gained from it anymore

Yep, should remove this completely.

Okey will do.

Next up implement the tooling lookup.

natanfudge · 2019-12-03T20:01:19Z

src/test/groovy/net/fabricmc/loom/BuildUtils.groovy

@@ -24,10 +24,10 @@ dependencies {
 	//to change the versions see the gradle.properties file
 	minecraft "com.mojang:minecraft:\${project.minecraft_version}"
 	mappings "net.fabricmc:yarn:\${project.yarn_mappings}"
-	modCompile "net.fabricmc:fabric-loader:\${project.loader_version}"
+	compile "net.fabricmc:fabric-loader:\${project.loader_version}"


What?
That is the goal of this whole ordeal :D

Well, this should have been compile in the example mod, given how the loader touches no minecraft code at all

TODO: Add reflective override to the tooling provider.

One more to go for idea and ofcourse the idea gradle task.... Fun.

Now on to patching dep resolution.

…ct dependencies, now time for patching.

modmuss50 · 2020-01-01T16:17:08Z

is this ready for reviews and testing?

marchermans · 2020-01-01T16:43:19Z

I am still working on it.
That damn IDEA task is kind of difficult to deal with. Also christmas, and 3 birthdays in one week kind of took its toll.
Getting back on my horse though.

modmuss50 · 2020-01-01T16:44:44Z

No, dont rush or worry at all. I was just checking you werent waiting on me or something stupid 👍

natanfudge · 2020-01-01T16:52:15Z

I wouldn't worry too much about the IDEA task. You can still just import Gradle instead.

…name (FabricMC#156)

marchermans and others added 15 commits November 30, 2019 11:21

Attempting to get the transformer to work.

dde5f9e

Started work on the CompiledJarRemappingTransformer

36b4ff2

Started work on the CompiledJarRemappingTransformer

759d6df

Removing the renameInvalidLocals.

4f0536f

Attempting to get array stream to work. Also transferring progress to…

6ee9f15

… laptop.

Finished initial implementation.

934d607

Fixed formatting.

9b849be

Adding licence header.

fe72ebf

Fix the return statement in case of none remappable file.

8ae6871

Finishing the initial implementation.

d738726

Reworked the transformers and enabled the implementation to run the o…

0961481

…ld code but adapted.

Removing the ContainedZipStrippingTransformer.java

5c07989

Bumping down the version again.

c130193

Cleanup pass 1.

bafa302

Change the dependency handling back to use the dependencies as specif…

be303fe

…ied by the transformer, or use none if a dependency is being extracted.

marchermans mentioned this pull request Dec 1, 2019

Concurrency issue with ZipFileSystemProvider.newFileSystem FabricMC/tiny-remapper#7

Open

natanfudge suggested changes Dec 1, 2019

View reviewed changes

Chocohead reviewed Dec 1, 2019

View reviewed changes

marchermans and others added 2 commits December 3, 2019 19:45

Started working on the IDEA logic.

c2e74aa

Adding the classes which are relevant to fix the tooling.

ff960a5

Next up implement the tooling lookup.

natanfudge reviewed Dec 3, 2019

View reviewed changes

marchermans and others added 6 commits December 3, 2019 21:22

Finishing up the IDEA implementation.

904d6cb

TODO: Add reflective override to the tooling provider.

Adding initial documentation.

7f15db9

Somewhat working version with the first model builder patched.

d953af4

One more to go for idea and ofcourse the idea gradle task.... Fun.

Small refactoring.

7532811

Attempting to get the ExternalModelBuilder to behave.

ed3bb5f

Patched all groovy stuff

b029e63

Now on to patching dep resolution.

marchermans and others added 3 commits December 8, 2019 09:18

Finished up the initial port of the tooling required for idea to dete…

8b02f36

…ct dependencies, now time for patching.

Splitting into two plugins, since I can not fix it one way.

76dd1b7

Initial working version for idea, patched the ClassLoadingHandler issue.

242782a

jpenilla pushed a commit to jpenilla/fabric-loom that referenced this pull request Nov 19, 2023

fix: missing a patch for FabricMC#153 that c290900 change the method …

2b64c2f

…name (FabricMC#156)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] Adding transformer support to be able to use remapped dependencies in any configuration. #156

[DRAFT] Adding transformer support to be able to use remapped dependencies in any configuration. #156

marchermans commented Nov 30, 2019

liach commented Nov 30, 2019

marchermans commented Dec 1, 2019

marchermans commented Dec 1, 2019

natanfudge left a comment

natanfudge Dec 1, 2019

marchermans Dec 2, 2019

natanfudge Dec 1, 2019

natanfudge Dec 1, 2019

natanfudge Dec 1, 2019

natanfudge Dec 1, 2019

liach Dec 1, 2019

natanfudge Dec 1, 2019

marchermans Dec 2, 2019

natanfudge Dec 1, 2019

Chocohead Dec 1, 2019

marchermans Dec 2, 2019

natanfudge Dec 1, 2019

marchermans Dec 2, 2019

natanfudge Dec 1, 2019

Chocohead Dec 1, 2019

natanfudge Dec 1, 2019

marchermans Dec 2, 2019

natanfudge Dec 1, 2019

Chocohead Dec 1, 2019

liach Dec 2, 2019

marchermans Dec 2, 2019

natanfudge Dec 3, 2019

marchermans Dec 3, 2019

liach Dec 3, 2019

modmuss50 commented Jan 1, 2020

marchermans commented Jan 1, 2020

modmuss50 commented Jan 1, 2020

natanfudge commented Jan 1, 2020


		File nestedFile = new File(extension.getNestedModCache(), fileName.substring(fileName.lastIndexOf("/")));
		for (int i = 0; i < jsonArray.size(); i++) {

[DRAFT] Adding transformer support to be able to use remapped dependencies in any configuration. #156

Are you sure you want to change the base?

[DRAFT] Adding transformer support to be able to use remapped dependencies in any configuration. #156

Conversation

marchermans commented Nov 30, 2019

Current situation

New situation

liach commented Nov 30, 2019

marchermans commented Dec 1, 2019

marchermans commented Dec 1, 2019

natanfudge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modmuss50 commented Jan 1, 2020

marchermans commented Jan 1, 2020

modmuss50 commented Jan 1, 2020

natanfudge commented Jan 1, 2020