Deduplicate replaced dependencies #259
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
niloc132
reviewed
Apr 10, 2024
| )); | ||
| )) | ||
| // Removing any duplicate dependencies that would otherwise trigger unjustified duplicate paths detection in Closure | ||
| .distinct(); |
There was a problem hiding this comment.
I don't really like doing this, for two chief reasons:
- Since it seems like there could be more than one that might be the "right" answer expected by different libraries on a classpath. The BUNDLE_JAR tasks have a certain order (both of files in a maven module, and maven modules in the tree) already so that a distinct() will probably do the opposite of what you intended, but module and file order here is not specified at all, is likely to be maven- and/or filesystem-specific.
- It is currently convenient that modules are mostly 1:1 with paths, but that is mostly happenstance, and not something to rely on.
Questions to consider:
- Closure module de-duping has nothing to do with the module name - path similarity doesn't bother closure, only module names, so in the long run this isn't even fixing the right thing...?
- This isn't necessarily ordered though right? How are you certain that this removes the "right" files?
- Also, I'm pretty sure you also need this in the BUNDLE_JAR wiring so that j2cl:watch still works and doesnt break at runtime (which is how it responds to duplicate modules).
I think it probably should be redone in your own fork or your own task (note that taskfactory instances are picked up via service loader, so this is well supported...) as "this is a specific fix needed for my own build", or redone in this patch as "this dedup's goog.provides (by name), goog.modules (by name), and es6 modules (by path), not dedup all files by path".
|
The issue I'm facing is that after the dependency replacements have been applied, the effective project has some final dependencies listed multiple times. The Vertispan plugin doesn't manage this case properly. In general in Maven it's allowed to have the same dependency listed multiple times in a pom.xml, even if it's not recommended, it's not a fault and this shouldn't break the build. And in particular if that pom is an effective pom generated by a tool (even if that effective pom is just virtual in memory) it should be even more relevant to not break the build. I moved the Can you please reconsider my PR? |
You're conflating Maven and java - in Maven with the j2cl-maven-plugin this is allowed too, and while Java handles duplicates on its classpath very gracefully, ClosureCompiler does not. You're also mixing "dependency" and "class" - listing a single dependency, even with different versions, multiple times is managed the way you would expect with Maven for a Java project too - Maven will decide which version of that groupId+artifactId to use (default strategy is to pick the definition "closest" to your project, which is sort of defined as "sort the various dependency decls by distance from the project being built, and use the first"), and will not include the others. You can see this when reviewing With all of that said, if this fix resolves your bug, it sounds like you have two classifiers but the same groupid+artifactid for some dependency, and, most importantly, that these artifacts contain the same .java (or potentially .js) files. If that is the case, it probably is correct for your project to just use And if so, this could break cases where "classifier" is used to mean "this artifact is different from that one for some important reason" - imagine a library that ships with an optional shim, and you can add the classifier to opt-in to using the shim. Not saying someone is necessarily using it this way, but it is possible at least, and it would break that case (without a clear rule about "the first one wins" or how to define which classifier beats other classifiers). But this fix will not solve the "bug" where two distinctly named dependencies contain the same class or js file, which seemed closer to what the original fix was intended to do (and would have solved). Can we take a few steps back and look at the problem rather than jumping to solutions? If an excludes (or possibly even "just don't add that extra classifier=sources dep") resolves this, it would seem simpler... |
|
Ok, let's try to investigate further the cause, but I don't think I have 2 classifiers of the same groupid+artifactid like you said, because there are no classifiers other than I don't think my dependency graph has any issue, I think the issue happens after replacements, so it's very difficult to investigate because I don't see the final effective pom after replacements, it's kind of virtual in your plugin. Here are all the replacements I do so far on any WebFX projects such as the one above: <plugin>
<groupId>com.vertispan.j2cl</groupId>
<artifactId>j2cl-maven-plugin</artifactId>
<version>${vertispan.j2cl.plugin.version}</version>
<executions>
<execution>
<id>build-js</id>
<phase>process-classes</phase>
<goals>
<goal>build</goal>
</goals>
</execution>
</executions>
<configuration>
<!-- Note: Specifying dependencyReplacements removes the defaults provided by Vertispan -->
<dependencyReplacements>
<!-- So, we first re-apply the Vertispan defaults (from https://github.com/Vertispan/j2clmavenplugin/blob/320e5ba05f2916cb14839a829f73151776d1755b/j2cl-maven-plugin/src/main/java/com/vertispan/j2cl/mojo/AbstractBuildMojo.java#L91) -->
<dependencyReplacement>
<original>com.google.jsinterop:base</original>
<replacement>com.vertispan.jsinterop:base:${vertispan.jsinterop.base.version}</replacement>
</dependencyReplacement>
<dependencyReplacement>
<original>org.realityforge.com.google.jsinterop:base</original>
<replacement>com.vertispan.jsinterop:base:${vertispan.jsinterop.base.version}</replacement>
</dependencyReplacement>
<dependencyReplacement>
<original>com.google.gwt:gwt-user</original>
</dependencyReplacement>
<dependencyReplacement>
<original>com.google.gwt:gwt-dev</original>
</dependencyReplacement>
<dependencyReplacement>
<original>com.google.gwt:gwt-servlet</original>
</dependencyReplacement>
<!-- Then, we apply the replacements required for WebFX -->
<!-- Considering new GWT groupId -->
<dependencyReplacement>
<original>org.gwtproject:gwt-user</original>
</dependencyReplacement>
<dependencyReplacement>
<original>org.gwtproject:gwt-dev</original>
</dependencyReplacement>
<dependencyReplacement>
<original>org.gwtproject:gwt-servlet</original>
</dependencyReplacement>
<!-- OpenJFX javafx-base => WebFX emul module for javafx-base -->
<dependencyReplacement>
<original>org.openjfx:javafx-base</original>
<replacement>dev.webfx:webfx-kit-javafxbase-emul:${webfx.version}</replacement>
</dependencyReplacement>
<!-- OpenJFX javafx-graphics => WebFX emul module for javafx-graphics -->
<dependencyReplacement>
<original>org.openjfx:javafx-graphics</original>
<replacement>dev.webfx:webfx-kit-javafxgraphics-fat-j2cl:${webfx.version}</replacement>
</dependencyReplacement>
<!-- There are actually several WebFX modules for javafx-graphics, but they are all merged
into 1 single module for J2CL, the reason being that individual replacements was causing a
dependency cycle in the J2CL plugin). This merged module is a kind of fat module of the
individual modules (that are still in used for GWT and for the development).
So, we need to replace all possible dependencies to the individual modules to that merged
module when compiling for J2CL. -->
<dependencyReplacement>
<original>dev.webfx:webfx-kit-launcher</original>
<replacement>dev.webfx:webfx-kit-javafxgraphics-fat-j2cl:${webfx.version}</replacement>
</dependencyReplacement>
<dependencyReplacement>
<original>dev.webfx:webfx-kit-javafxgraphics-emul</original>
<replacement>dev.webfx:webfx-kit-javafxgraphics-fat-j2cl:${webfx.version}</replacement>
</dependencyReplacement>
<dependencyReplacement>
<original>dev.webfx:webfx-kit-javafxgraphics-peers</original>
<replacement>dev.webfx:webfx-kit-javafxgraphics-fat-j2cl:${webfx.version}</replacement>
</dependencyReplacement>
<dependencyReplacement>
<original>dev.webfx:webfx-kit-javafxgraphics-peers-base</original>
<replacement>dev.webfx:webfx-kit-javafxgraphics-fat-j2cl:${webfx.version}</replacement>
</dependencyReplacement>
<dependencyReplacement>
<original>dev.webfx:webfx-kit-javafxgraphics-peers-gwt-j2cl</original>
<replacement>dev.webfx:webfx-kit-javafxgraphics-fat-j2cl:${webfx.version}</replacement>
</dependencyReplacement>
<dependencyReplacement>
<original>dev.webfx:webfx-kit-javafxgraphics-registry-gwt-j2cl</original>
<replacement>dev.webfx:webfx-kit-javafxgraphics-fat-j2cl:${webfx.version}</replacement>
</dependencyReplacement>
<dependencyReplacement>
<original>dev.webfx:webfx-kit-util</original>
<replacement>dev.webfx:webfx-kit-javafxgraphics-fat-j2cl:${webfx.version}</replacement>
</dependencyReplacement>
<!-- OpenJFX javafx-controls => WebFX emul module for javafx-controls -->
<dependencyReplacement>
<original>org.openjfx:javafx-controls</original>
<replacement>dev.webfx:webfx-kit-javafxcontrols-emul:${webfx.version}</replacement>
</dependencyReplacement>
<!-- OpenJFX javafx-media => WebFX emul module for javafx-media -->
<dependencyReplacement>
<original>org.openjfx:javafx-media</original>
<replacement>dev.webfx:webfx-kit-javafxmedia-emul:${webfx.version}</replacement>
</dependencyReplacement>
<!-- OpenJFX javafx-web => WebFX emul module for javafx-web -->
<dependencyReplacement>
<original>org.openjfx:javafx-web</original>
<replacement>dev.webfx:webfx-kit-javafxweb-emul:${webfx.version}</replacement>
</dependencyReplacement>
</dependencyReplacements>
<compilationLevel>${plugin.j2cl.compilationLevel}</compilationLevel>
<enableSourcemaps>${plugin.j2cl.enableSourcemaps}</enableSourcemaps>
</configuration>
</plugin>It would be very helpful if your plugin could detect the Here is an extract of the Closure error I'm getting without my previous fix (just the first few lines of a long list): But if I try to find such duplicate paths in gwt3BuildCache, I don't find them: I'm getting only 1 entry, so what duplicate paths is it talking about? |
|
Looks like I made a mistake - a Project's key does include its classifier, if any, so if the current patch fixes your project, you do indeed seem to have two dependencies in the same project that point to the same other project. Breaking this down a bit - your old patch was on this code while the new patch was specific to the scope() call, which is line 157 above, or just So, that is the one specific line you need to distinct() to fix this issue... Project.dependencies, for a non-test project, is set in one of two places in AbstractBuildMojo - line 259 for jszips, and 350 for all non-jszip projects. I assume the duplicate project in question is not a jszip? Line 350 gets its items from iterating mavenProject.getArtifacts(), through filtering out unneeded items and otherwise translating each maven artifact to a j2clmavenplugin Project wrapped in a Dependency. MavenProject.getArtifacts() must only return the flattened Are you able to see where in this flow, from MavenProject.getArtifacts() forward that your duplicate is being added? |
|
No, I have no jszip, so must be line 350. I will try to investigate, but my feeling is that the issue happens because I have several replacements that points to the same fat jar (see in my Vertispan plugin configuration), so the fat jar artifact is processed and added several times (which shouldn't be a problem as we both agreed), but then this causes the issue to happen (which is a bug in the plugin in my opinion). I wanted to add a case in the dependency replacement code (which starts at line 311) to skip the replacement if it was already processed, but there is no way to know the replacement history (unless I add a HashSet to the method signature but I didn't want to modify your code too much). Or do you see another way to achieve this? I also tried to add this test at line 348: if (!dependencies.contains(dep))
dependencies.add(dep);but this doesn't solve the issue 🤷 So I'm still not sure precisely where/when the multiple dependency is added... |
|
Maybe some interesting info: I replaced my fix in the So the fat jar Then I added some other code in So it's only the fat jar duplicates that cause the problem, not the other duplicates... |
|
I just realised that After implementing it, then if (!dependencies.contains(dep))
dependencies.add(dep);at line 348 solves the issue at a sooner stage than my previous fix in Does that fix at line 348 + |
|
To confirm, you are saying that MavenProject.getArtifacts() contains exact duplicates? That's the root cause here and the surprise - if we need other filtering, we should understand why there are duplicates rather than just removing the first instance that matches. Can you share the code that printed those duplicates? I don't understand what it means to have duplicates over the whole list, esp the row that doesn't have duplicates at all: Just in case this is a point of confusion: duplicates across the entire Project tree are expected and necessary. Duplicates within a single Project's getDependencies() are not. A Project's getDependencies() are all that is used to build that project, the "walk the tree and find transitive dependencies" was already performed by Maven before MavenProject.getArtifacts() was called. |
|
Here is my code for duplicates detection: protected List<Project> scope(Collection<? extends Dependency> dependencies, Dependency.Scope scope) {
List<Project> projects = dependencies.stream()
.filter(d -> ((com.vertispan.j2cl.build.Dependency) d).belongsToScope(scope))
.map(Dependency::getProject)
.collect(Collectors.toUnmodifiableList());
List<Project> duplicates = listDuplicates(projects);
if (!duplicates.isEmpty())
System.out.println("Found duplicate projects: " + duplicates);
return projects;
}
List<Project> listDuplicates(List<Project> list) {
List<Project> duplicates = new ArrayList<>();
Set<Project> set = new HashSet<>();
for (Project i : list) {
if (set.contains(i)) {
duplicates.add(i);
} else {
set.add(i);
}
}
return duplicates;
}
Anyway, can you try it on another project and let me know if you also have duplicates? |
|
I also tried this at line 348: if (!dependencies.contains(dep))
dependencies.add(dep);
else
System.out.println("Skipping dependency " + dep);and then got this output (truncated but build is successful): As you can see, duplicate dependencies are introduced by the replacement mechanism when different artifacts are replaced by the same replacement artifact. This issue is solved with the condition |
|
Ah thank you, that clarifies things. So the specific bug is: Add multiple replacement rules, where each replacement rule maps from distinct artifacts to a single artifact. In this case, each individual artifact (but not dependencies) is removed and replaced. That really isn't what the tool is intended for, but never mind that - we should be easily able to deduplicate just at the maven level, and not bother the j2cl tooling. It looks like this can be most simply and directly achieved by modifying AbstractBuildMojo.java leading up to line 350, so that the list of dependencies that are passed in contain no duplicates. Ideally, I would make it more specific - as discussed, maven itself will not generated duplicates, only the replacement rules will. So, even more precisely, the block from 310-322 could avoid duplicating items. I'm not sure if that is worth the extra overhead though - a commented "here's what and why we're distinct()ing on" at around line 350 seems best, and least likely to imply that anything outside of maven and dependency replacements is involved. Which is what I think you're saying here (but with comments):
|
|
Ok, great! Reading your comment, not sure between: Fix 1) at line 350: project.setDependencies(dependencies.stream().distinct().collect(Collectors.toList()));Fix 2) at line 348: if (!dependencies.contains(dep))
dependencies.add(dep);what's your preference? (I will add comment once you clarified) |
|
I'd lean towards the second, it would make it easier for an assertion as well (that either the already-added was a dep replacement, or the one that we're skipping is), which in turn means adding a Set local var to track which are dependency replacements. (plus |
…jo instead (root cause)
|
Ok, so I pushed the following changes:
So in the end, 2. & 3. are the only changes made by this PR to the original files. Let me know if you're happy with that |
|
As discussed on the matrix chat, I changed the code to resolve duplicate projects with different scopes and keep only the one with scope = BOTH in that case. Because this new code doesn't rely on Dependency.equals() anymore, I removed my changes from Dependency, and in the end this PR just changes AbstractBuildMojo. Let me know if you're happy with this last proposition. |
niloc132
approved these changes
Jun 28, 2024
|
Looks great, builds pass. Can you update the title/description to match the current expectations of this patch (broadly something like "deduplicate replaced dependencies")? |
salmonb
changed the title
|
Thanks for approving these changes. I renamed the PR title as you suggested. This code is now yours, so feel free to amend it (including comments) to fit your standards. Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.