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

Transform sometimes fails to map GA to file name if version and Jar file name do not align #129

Closed
jjohannes opened this issue Jul 4, 2024 · 5 comments · Fixed by #144
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@jjohannes
Copy link
Member

This can happen e.g. with Guava, where the version can be 32.1.3-android but the Jar name is 32.1.3-jre.

Scan: https://scans.gradle.com/s/xo55e6547gqcq

This does not always happen. So it most likely depends on whether the Jar is taken from .gradle cache or .m2 cache.

@jjohannes jjohannes added the bug Something isn't working label Jul 4, 2024
@jjohannes jjohannes self-assigned this Jul 4, 2024
@Vampire
Copy link

Vampire commented Jul 5, 2024

Another instance of this I just wanted to report is, if other transforms run before the extra-java-module-info transform and change the name of the resulting file which can easily happen if multiple transforms are involved.

See also gradle/gradle#29833

@Vampire
Copy link

Vampire commented Jul 5, 2024

So yeah, if it is not a technical necessity that the filename matches the expectation, it would be great if this restriction is lifted.

@jjohannes
Copy link
Member Author

Thanks for sharing the case of multiple transforms running after each other. I never gave that much thought in the context of this plugin.

Unfortunately, it is necessary to look at the file names/paths – at least partially. For the transform input, we only know the file name. But we would like to know the G:A coordinates from which the Jar was downloaded. So that the user can use these instead of the Jar file name when defining a patch rule.

To determine the G:A, we look at the file path which (usually) contains the G:A information.
This is super hacky, but the best option I know of.

It is a missing Gradle feature: gradle/gradle#11831

I thought about passing additional inputs to the transform which somehow contain this information. For the requireAllDefinedDependencies feature, we do something like that already. We add the metadata of compile/runtimes classpaths as an input which contain G:A information. But to find the right information in there, you still need to map the file name back to the information. I think if another transform ran before, there is no way to establish this relationship again.
Also, passing the metadata of the whole classpath has a cost: The input of the transform is different for every subproject and the transform therefore runs multiple times for the same input Jar. Which is a problem of the requireAllDefinedDependencies feature right now that I do not want to spread further.

@Vampire
Copy link

Vampire commented Jul 8, 2024

To determine the G:A, we look at the file path which (usually) contains the G:A information.
This is super hacky, but the best option I know of.

Urgh, not only super hacky, also super flaky.
For local dependencies, this does not hold.
And for already transformed dependencies this also does not hold, even if the name would be kept identical.
If you for example change the IdTransform we had in the other issue to inputArtifact.get().asFile.copyTo(outputs.file("foo.jar")) and add an external dependency, the outcome of the transform will be

~/.gradle/caches/8.9-rc-2/transforms/dd9982b5c7b5587fa33b5f772878d212/transformed/foo.jar
~/.gradle/caches/8.9-rc-2/transforms/8cc6514df15609f941e6856db5ea2f65/transformed/foo.jar

@jjohannes
Copy link
Member Author

Urgh, not only super hacky, also super flaky.

I don't see how it is flaky. Even if transformations are chained, they run in deterministic order. It may break then, but it will break consistency. You can always use the file name as identifier if it is not working. Of course that is not very nice usability, but it probably affects almost no users in practice

The issue from the description is fixed. Here is a follow up to potentially change the whole approach in the future, but as of now, I don't see a way to do that: #145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants