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

Artifact transforms: Allow getting ID of artifact #11831

Open
LunNova opened this issue Jan 8, 2020 · 8 comments
Open

Artifact transforms: Allow getting ID of artifact #11831

LunNova opened this issue Jan 8, 2020 · 8 comments
Labels

Comments

@LunNova
Copy link

LunNova commented Jan 8, 2020

Expected Behavior

An artifact transform should be able to get the ID of the artifact being transformed.

Current Behavior

Only the file name is available, so if you have multiple artifacts with the same filename it is hard to ensure the transform only applies to the correct one. It also would be possible for your artifact's filename to be in no way related to its ID's name.

Context

This is very similar to the documented use case in the 'minify' example in your documentation. That example checks the file name to determine whether to run, which seems quite risky.

A way to specify an attribute of a specific dependency instead of on everything in a configuration might also be useful here?

It does seem that there's something off with the model here. If I have a transform which behaves differently for different artifacts, build caching is based on the entire input configuration even though that will have varied if the configuration for any artifact is changed.

A change to improve that would probably be quite complicated though. :c

@LunNova
Copy link
Author

LunNova commented Jan 9, 2020

More discussion on trying to use artifact transforms, with code snippets / kinda working plugin code there: MinimallyCorrect/Mixin#6

Would appreciate it if someone with knowledge here could look at what we're trying, and suggest how to do it properly if it's currently possible?

@wolfs
Copy link
Member

wolfs commented Jan 9, 2020

@nallar Could you please describe what you want to do? It is not clear to me, yet, what your use-case is. Let's start with the use-case without mentioning artifact transforms at all.

@LunNova
Copy link
Author

LunNova commented Jan 9, 2020

@wolfs A standalone example of patching a jar with Mixin looks like this:

val applicator = new MixinApplicator();

// load patches
applicator.addSource(new File("path/to/mixin/src/root").toPath());
// OR
applicator.addSource(new File("path/to/compiled-mixins.jar").toPath());

// apply patches and make new jar
applicator.getMixinTransformer().transform(new File("dependency.jar").toPath(), new File("dependency-patched.jar").toPath());

To develop against the patched code, we need to use a patched version of the dependency.

The gradle plugin is trying to apply these patches using transforms.

A real world example of a patch looks something like this: https://github.com/MinimallyCorrect/TickThreading/blob/1.12/src/main/java/org/minimallycorrect/tickthreading/mixin/extended/server/MixinMinecraftServer.java#L29

@wolfs
Copy link
Member

wolfs commented Jan 10, 2020

@nallar Thank you for the example. So you define some mix-ins and then you transform all the dependencies to use the transformed JARs in the current project, right? And each mix-in has some external dependencies/project dependencies/sources where it is defined.

The dependencies of the mix-in the go into a parameters object (in your case MixinTransform.Parameters and ApplyMixins).

What I don't understand why you have different mix-ins per dependency. Shouldn't all dependencies be transformed by the same mix-in?

@LunNova
Copy link
Author

LunNova commented Jan 23, 2020

@wolfs

What I don't understand why you have different mix-ins per dependency. Shouldn't all dependencies be transformed by the same mix-in?

A mixin patches a particular dependency. A mixin which patches classes in guava has no reason to be applied to other dependencies as they don't include the classes which will be patched.

@jjohannes
Copy link
Contributor

We had a discussion about that yesterday as this also causes issues in Gradle's own build which also uses a version of the minify example.

Another way to improve this is to make the declaration of "artifact types" richer, which is very basic currently. I tend to think it would be the better solution, so I spiked the idea here: #12089

For the minify example, this would mean that you can avoid filtering inside the artifact transform. Instead you assign attributes, triggering the transform, only to a selected set of artifacts:

artifactTypes.create("jar-completete") {
     forFileNameExtensions("jar")
     forModule("com.google.guava", "guava")
     attributes.attribute(minified, java.lang.Boolean.FALSE)
}

So the "jar-complete" type is assigned to all files with extension "jar" only if they orginate from one of the modules identified by forModule().

Happy to get some feedback on this

@jjohannes
Copy link
Contributor

Just stumbled over this again. If anyone is needing a solution for this. What I ended up with is reconstructing the Artifact ID from the location of the Jar file in Gradle's cache (or in .m2):

https://github.com/gradlex-org/extra-java-module-info/blob/08c9df62a2d11eb7a329b32c9377ac4dac5657d3/src/main/java/org/gradlex/javamodule/moduleinfo/FilePathToModuleCoordinates.java

@LunNova
Copy link
Author

LunNova commented Jan 6, 2024

@jjohannes that's what I went with after filing this issue but it seemed too fragile to ship so I didn't keep working on it. Relying on the path of the file to get the coordinates is maybe ok inside gradle's own code but doing it myself is relying on an implementation detail and I can expect it to break mysteriously later.

https://github.com/MinimallyCorrect/Mixin/blob/b6a32c5a1978abe287aaee52f0e6e17cce1286a8/gradle-plugin/src/main/java/dev/minco/gradle/mixin/transform/MixinTransform.java#L64-L113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants