closure-compiler icon indicating copy to clipboard operation
closure-compiler copied to clipboard

Remove shaded content and restore dependency list to unshaded maven artifact

Open niloc132 opened this issue 2 years ago • 8 comments

Uses rules_jvm_external's java_export rule to mark a given java_library as an artifact for export to maven. This rule will find any dependencies that were originally fetched from maven (by looking at the maven_coordinates tags attached to their java_library), and exclude them from the output jar, plus also populate a pom.xml file template with those dependencies.

For consistency, renamed the "unshaded" deploy jar to "uberjar", so as to be clear that the dependencies have been merged, but not relocated.

Both shaded and unshaded were built and installed locally (via mvn install:install-file) and tested with the sample project at https://gist.github.com/niloc132/1244ad7b72ec951800c5c1c12bfe08bc to confirm that a stage1 build would first write typedast content, and then a stage2+3 would read them and finish the job.

Fixes #3896

niloc132 avatar Apr 14 '22 19:04 niloc132

Build failed with

Run mkdir -p bazel-bin-unsymlink
cp: cannot stat 'bazel-bin/compiler_unshaded_deploy.jar': No such file or directory

I'll push a commit to fix the paths that are being referenced to "unshaded" content.

niloc132 avatar Apr 14 '22 20:04 niloc132

Added a commit that replaces nearly every "compiler_unshaded_deploy.jar" reference with "compiler_shaded.jar", as they almost certainly all expect to find the entire classpath in a single jar, rather than an actual unshaded jar.

I did notice that in package.json, the build script is non-functional, it will try to call maven instead of bazel. That probably should be corrected, I can sneak it into this PR, or another followup?

niloc132 avatar Apr 14 '22 20:04 niloc132

 > google-closure-compiler-linux
yarn run v1.22.18
$ node ./test.js
google-closure-compiler-linux
  ✓ compiler binary exists
Exception in thread "main" java.lang.IllegalArgumentException: class com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.spi.BooleanOptionHandler does not have the proper constructor
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.OptionHandlerRegistry.getConstructor(OptionHandlerRegistry.java:111)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.OptionHandlerRegistry.access$000(OptionHandlerRegistry.java:43)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.OptionHandlerRegistry$DefaultConstructorHandlerFactory.<init>(OptionHandlerRegistry.java:217)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.OptionHandlerRegistry.registerHandler(OptionHandlerRegistry.java:138)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.OptionHandlerRegistry.initHandlers(OptionHandlerRegistry.java:72)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.OptionHandlerRegistry.<init>(OptionHandlerRegistry.java:67)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.OptionHandlerRegistry.getRegistry(OptionHandlerRegistry.java:57)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.CmdLineParser.createOptionHandler(CmdLineParser.java:178)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.CmdLineParser.addOption(CmdLineParser.java:146)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.ClassParser.parse(ClassParser.java:34)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.CmdLineParser.<init>(CmdLineParser.java:96)
	at com.google.javascript.jscomp.jarjar.org.kohsuke.args4j.CmdLineParser.<init>(CmdLineParser.java:71)
	at com.google.javascript.jscomp.CommandLineRunner$Flags.<init>(CommandLineRunner.java:957)
	at com.google.javascript.jscomp.CommandLineRunner.<init>(CommandLineRunner.java:1455)
	at com.google.javascript.jscomp.CommandLineRunner.main(CommandLineRunner.java:2239)

From this latest failure and taking a quick look at the closure-compiler-npm repo, it appears that the graal build assumes an uberjar, but not shaded build. Does it seem likely that we would need three flavors, unshaded and shaded (as the terms meant prior to https://github.com/google/closure-compiler/commit/c9f3d327be6146a7d4de1f3d7c03feb36f49524a, and as they generally mean in the general java ecosystem), plus also either a single jar with the full runtime classpath contained within it, or an ordered list of jars to use, which we can somehow pass to the graal build?

My guess right now is that in google/closure-compiler-npm, the build-scripts/reflection-config.json needs to be updated to reflect the package to the shaded jars. I'm currently trying to set up the rest of the build so I can confirm that - the simplest fix seems to be to duplicate all org.kuhsuke.args4j.* packages for a short time, until this release is out, then delete the unshaded packages.

Alternatively, I'm still looking for how the closure-compiler-npm build handled the unshaded jars pre-c9f3d327, but still had the full classpath. My best guess at the moment is that the pom file change is something of a red herring, and that the npm output was exclusively using bazel output (see https://github.com/google/closure-compiler/blob/c9f3d327be6146a7d4de1f3d7c03feb36f49524a^/.github/workflows/ci.yaml#L133 for example, the commit before the pom change, which explicitly uses the "unshaded" deploy jar). If npm wants non-jarjar'd uberjars jars, I can update these scripts - specifically, this diff would use compiler_uberjar_deploy.jar where previously it used compiler_unshaded_deploy.jar.

niloc132 avatar Apr 18 '22 14:04 niloc132

Haven't heard back in a while, so I tried the simple replacement of compiler_uberjar_deploy.jar in for compiler_unshaded_deploy.jar that was used before this diff, since that seems to be what it actually wants. We'll see what CI says about this.

niloc132 avatar May 05 '22 19:05 niloc132

Sorry. This PR is on my to-do list, but other things keep pushing it down before I can get to it.

brad4d avatar May 06 '22 23:05 brad4d

I've sent an internal change for review that will remove the bad "script" entries from package.json and the corresponding yarn commands from the README.md file. Thanks for pointing those out.

brad4d avatar May 07 '22 00:05 brad4d

@ChadKillingsworth this change will need to be synchronized with a change to https://github.com/google/closure-compiler-npm since that repo builds and uses compiler_unshaded_deploy.jar. I think it should work to have it just use compiler_uberjar_deploy.jar instead.

Do you have any concerns about that?

It looks like the uberjar just includes some additional .proto files and META-INF/maven/protobuf-java/pom.(xml|properties) files.

Here's a diff of the jar contents.

unshadedto_uberjar_diff.txt

brad4d avatar May 12 '22 22:05 brad4d

Just now seeing this. I have no concerns as long as a build target exists to produce the full jar. There will need to be path updates to the build made to the npm repo, but those changes would be pretty trivial.

ChadKillingsworth avatar Aug 14 '22 19:08 ChadKillingsworth

And I just now saw Chad's response. :)

I've imported this PR and I'm sending it through internal review and testing.

brad4d avatar Oct 13 '22 23:10 brad4d