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

Introduce Classpath duplicates check test #2309

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vorburger
Copy link
Member

Related to (and initial version of) #2220.

This introduces a (first) test which discovers, and fails in case of, duplicates in JARs on the classpath.

It already found 1 issue which seems a bit suspicious (to me), see #2308, but for now excludes that.

If this is acceptable, my idea (next step) is to extend this to the other (and more interesting) modules than just common.

@jingtang10
Copy link
Collaborator

how can this be extended to other modules - the test cases themselves seem a little bit "involved". is there a ... gradle plugin we can find to do the same job?

@kevinmost @yigit for gradle fun.

@vorburger
Copy link
Member Author

vorburger commented Oct 30, 2023

how can this be extended to other modules

I'm testing the water to see if you would find this acceptable... 😄

If this is accepted and merged, then I would propose to follow-up and extend this to other modules as follows:

Copy/paste that ClasspathHellDuplicatesCheckTest.kt, which is very small (to me), to each other module. Maybe there is a way to make it even shorter? But we're talking about saving 2-3 lines more - not worth it (I think).

Note that the ClasspathHellDuplicatesCheckRule.java would NOT get copy/pasted - that stays single, in common. (I would have to spend a minute, if / after this is merged, to figure out if e.g. tests in engine/ can depend on "test infrastructure" in common/ in Gradle. If not, I would, in the future follow-up PR, propose moving this ClasspathHellDuplicatesCheckRule from common/ into (something like) a tiny new common-testutil/ kind of module; perhaps there are other "test utilities" which could live there in the future.)

is there a ... gradle plugin we can find to do the same job? @kevinmost @yigit for gradle fun.

There are 2-3 old and unmaintained ones (follow links in #2220 for some history). In my view, this approach is simpler and more future proof. In a perfect world with unlimited time one sure could write a Gradle plugin for this, but (unless @kevinmost @yigit you want to help me and do this?) I unfortunately wouldn't really have the bandwidth to do that here. Personally I also feel like Gradle plugins may be a bit overrated some times... great for code generators incl. compilers and other such tools, but in this particular case this is really just a (re-usable) test utility, which (IMHO) is great as simple JUnit test as-is here; I don't really see significant additional value in having this as a Gradle plugin. It could be "nice to have" - but then that plugin needs to be maintained for future major Gradle upgrades etc. TL;DR I'm willing to collaborate with someone wanting to do that, but not do it alone myself.

@yigit
Copy link
Contributor

yigit commented Oct 30, 2023

So one problem with this approach is that you are traversing the classpath of the test, not the classpath of the library, which is different. I'm guessing what we really care is the library, not the test.

I do also think that solving this in gradle would be nicer.
You could write a task that will do this by resolving a classpath. But also, I don't think you even need that.

You can create a fatjar task that will bundle all dependencies and set its duplicate policy (just like in the test).

You can start in this module's build.gradle; once you have it working, you can move that into buildSrc and applly to all modules.

Unfortunately, i don't have much time to write it but shadow plugin is likely all you need.

@vorburger
Copy link
Member Author

vorburger commented Oct 30, 2023

You can create a fatjar task that will bundle all dependencies and set its duplicate policy (just like in the test).

Oh... that's a very nice idea! You mean something like https://www.baeldung.com/gradle-fat-jar#creating-a-separate-task with a https://docs.gradle.org/current/javadoc/org/gradle/api/file/DuplicatesStrategy.html#FAIL - basically? That JAR would be "useless" of course, and just for testing this, right? Slightly inefficient, but it doesn't really matter I guess... I like this idea!

Unfortunately, i don't have much time to write it but shadow plugin is likely all you need.

Can you elaborate on this approach a little more? Did you mean this instead of or in addition to using the Jar Task from the Java Plugin? From an only very quick glance at https://imperceptiblethoughts.com/shadow/introduction/, I don't see shadow having a duplicate policy? Sorry I don't think I really understood what you meant here. (FYI: I have used shadow in past lives and projects, e.g. here; I just don't understand yet how you see it possibly adding any value for #2220...)

@yigit
Copy link
Contributor

yigit commented Feb 6, 2024

Duplicate file strategy comes from the Jar task (docs)

My idea was to create a plugin that will create a Jar task from the runtime configuration. I'm actually not 100% if you need shadow or not, might just need to create a Jar task to jar it with duplication strategy of none.

I would probably try this first:
https://www.baeldung.com/gradle-fat-jar

// in gradle plugin
project.tasks.register("checkForDuplicatesion", Jar::class.java) {
    it.duplicatesStrategy = DuplicatesStrategy.FAIL
   // not sure about this but something like this code to find it
    it.from { configurations.named("runtimeClasspath").flatMap { it.isDirectory() ? it : zipTree(it) } }
}

and run it in CI. The android libraries' configuration might be a different one but you can simply list configurations to figure out which one.

Lmk if that helps.

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

Successfully merging this pull request may close these issues.

None yet

3 participants