-
Notifications
You must be signed in to change notification settings - Fork 403
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
Add version of UnusedTracker that supports full shrinking using R8. #796
Conversation
…e clase for different implementations.
Changes that I have performed since the last PR:
I applied all comments apart from the dependency on R8, its still implementation. I wanted to keep it like that to be able to easily write unit tests using the new shrinking mode. However I am perfectly fine to change it to a compileOnly dependency and check if R8 is available at runtime and use it in that case. |
@netomi Yeah: Your CI tests fail for the same reason as in my pull request: #773. Link to comment: #773 (comment). |
I have seen that, it comes from this commit: Seems easy to fix, will create a PR. |
@netomi I'm not so sure. The original PR author had a valid case for his/her PR: "Do not add a dependencies block if it's already there". But the PR seemed to be incomplete. I saw someone else just reverting it, I don't think that's a good idea, because the idea behind it is valid. |
the change assumes that if a node 'dependencies' exists, it is never empty as it accesses the item at index 0. In the tests you can see that the node is actually empty and thus fails with an index out of bounds exception. |
Ah yes, you're correct. I didn't have time to analyze the faulty code. That's great. Big thanks in advance if you fix it. |
The build issues wrt publication are fixed in PR #798 |
Big thanks. |
Updated the documentation and added possibility to specify additional rules via the shadow spec. I would consider this PR to be finished from my side unless there is some suggestion for change. With the additional rules that can be specified via the shadow spec, this method should allow enough flexibility to handle more complex scenarios. |
…y used for minimization. The test sources are not part of the output, and it confuses the shrinker if these classes are present during minimization.
I did some change in getSourceSetsClassesDirs(): previously it was returning all source set class dirs, also including test sources. This might not be a problem for minimization with JDependency, but if they are present in R8 potential warnings are produced and the minimization is not as good as it could be as some additional classes appear to be kept which are actually not present in the output. I added a filter to not include test sources. |
A more realistic use case of the minimization with R8 can be tested here: https://github.com/netomi/uom/tree/shadow-dependencies
In this project 2 classes from external dependencies are used (from guava and hibernate). Without minimization or the previous one, a huge number of classes is copied in the shadow jar. With R8 enabled, the copied dependencies is minimal as expected. |
Updated PR for #566
Adds support for #565