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.

Sign up for GitHub

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

Deduplicate replaced dependencies #259

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Deduplicate replaced dependencies #259

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97ae186
Removed duplicate dependencies before calling Closure
salmonb Mar 14, 2024
1964519
Moved distinct() to upper level (project dependencies)
salmonb May 23, 2024
eaec93d
Removed distinct() in TaskFactory and added a test in AbstractBuildMo…
salmonb May 31, 2024
b9f084f
Updated patch to solve scope conflicts
salmonb Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion j2cl-tasks/src/main/java/com/vertispan/j2cl/build/provided/ClosureTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,9 @@ public Task resolve(Project project, Config config) {
// Bytecode sources will include original input sources
// as well as generated input when the jar was built
input(p, OutputTypes.BYTECODE)
));
))
// Removing any duplicate dependencies that would otherwise trigger unjustified duplicate paths detection in Closure
.distinct();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".


Stream<Input> jsFromJsZips = scope(project.getDependencies(), Dependency.Scope.RUNTIME)
.stream()
Expand Down