From ec2c5617b339844312d4adef4400dcc2ccb73c4f Mon Sep 17 00:00:00 2001 From: Simon Stewart Date: Mon, 1 Mar 2021 11:08:55 +0000 Subject: [PATCH] Generate pseudo-compile jars for bazel java compilation (#518) Normally, bazel uses `ijar` to prepare a jar containing just ABI-entries for classfiles. These are stable when non-API breaking changes are made, and so allow bazel to compile code faster, and are known as "compile jars" Because of https://github.com/bazelbuild/bazel/issues/4549 `rules_jvm_external` doesn't use `ijar`, and instead uses the downloaded jar as the "compile jar". Normally, this is fine, but Bazel 4.0.0 now sets `-Xlint:path` for javac invocations, and this means that if there's a `Class-Path` manifest entry in a compile jar, the jars within _that_ will be checked. This can lead to noisy builds: https://github.com/bazelbuild/bazel/issues/12968 This change generates a "pseudo-compile jar" for `jvm_import` targets. All it does is repack the input jar, excluding the `META-INF/MANIFEST.MF` file. This means that we should avoid compilation problems whilst still working well with Kotlin projects. --- private/rules/jvm_import.bzl | 50 ++--- .../jvm/external/jar/AddJarManifestEntry.java | 172 +++++++++--------- .../tools/java/rules/jvm/external/jar/BUILD | 3 + .../external/jar/AddJarManifestEntryTest.java | 151 +++++++++++++++ tests/com/jvm/external/jar/BUILD | 1 + 5 files changed, 267 insertions(+), 110 deletions(-) diff --git a/private/rules/jvm_import.bzl b/private/rules/jvm_import.bzl index a7fec390e..9840981af 100644 --- a/private/rules/jvm_import.bzl +++ b/private/rules/jvm_import.bzl @@ -15,27 +15,35 @@ def _jvm_import_impl(ctx): injar = ctx.files.jars[0] outjar = ctx.actions.declare_file("processed_" + injar.basename, sibling = injar) - ctx.actions.run_shell( - inputs = [injar] + ctx.files._add_jar_manifest_entry, + args = ctx.actions.args() + args.add_all(["--source", injar, "--output", outjar]) + args.add_all(["--manifest-entry", "Target-Label:{target_label}".format(target_label = ctx.label)]) + ctx.actions.run( + executable = ctx.executable._add_jar_manifest_entry, + arguments = [args], + inputs = [injar], outputs = [outjar], - command = "\n".join([ - # If the jar is signed do not modify the manifest because it will - # make the signature invalid. - "if unzip -l {injar} | grep -qE 'META-INF/.*\\.SF'; then".format(injar = injar.path), - " cp {injar} {outjar}".format(injar = injar.path, outjar = outjar.path), - "else", - " set -e", - " {add_jar_manifest_entry} --source {injar} --manifest-entry 'Target-Label:{target_label}' --output {outjar}".format( - add_jar_manifest_entry = ctx.attr._add_jar_manifest_entry.files_to_run.executable.path, - injar = injar.path, - target_label = ctx.label, - outjar = outjar.path, - ), - "fi", - ]), mnemonic = "StampJarManifest", progress_message = "Stamping the manifest of %s" % ctx.label, - tools = [ctx.attr._add_jar_manifest_entry.files_to_run], + ) + + compilejar = ctx.actions.declare_file("header_" + injar.basename, sibling = injar) + args = ctx.actions.args() + args.add_all(["--source", outjar, "--output", compilejar]) + # We need to remove the `Class-Path` entry since bazel 4 forces `javac` + # to run `-Xlint:path` no matter what other flags are passed. Bazel + # manages the classpath for us, so the `Class-Path` manifest entry isn't + # needed. Worse, if it's there and the jars listed in it aren't found, + # the lint check will emit a `bad path element` warning. We get quiet and + # correct builds if we remove the `Class-Path` manifest entry entirely. + args.add_all(["--remove-entry", "Class-Path"]) + ctx.actions.run( + executable = ctx.executable._add_jar_manifest_entry, + arguments = [args], + inputs = [outjar], + outputs = [compilejar], + mnemonic = "CreateCompileJar", + progress_message = "Creating compile jar for %s" % ctx.label, ) if not ctx.attr._stamp_manifest[StampManifestProvider].stamp_enabled: @@ -46,7 +54,7 @@ def _jvm_import_impl(ctx): files = depset([outjar]), ), JavaInfo( - compile_jar = outjar, + compile_jar = compilejar, output_jar = outjar, source_jar = ctx.file.srcjar, deps = [ @@ -79,11 +87,11 @@ jvm_import = rule( ), "_add_jar_manifest_entry": attr.label( executable = True, - cfg = "host", + cfg = "exec", default = "//private/tools/java/rules/jvm/external/jar:AddJarManifestEntry", ), "_stamp_manifest": attr.label( - default = Label("@rules_jvm_external//settings:stamp_manifest"), + default = "@rules_jvm_external//settings:stamp_manifest", ), }, implementation = _jvm_import_impl, diff --git a/private/tools/java/rules/jvm/external/jar/AddJarManifestEntry.java b/private/tools/java/rules/jvm/external/jar/AddJarManifestEntry.java index 0961cbc63..3e2f40cd7 100644 --- a/private/tools/java/rules/jvm/external/jar/AddJarManifestEntry.java +++ b/private/tools/java/rules/jvm/external/jar/AddJarManifestEntry.java @@ -1,5 +1,7 @@ package rules.jvm.external.jar; +import rules.jvm.external.ByteStreams; + import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -8,18 +10,24 @@ import java.nio.file.Paths; import java.time.LocalDateTime; import java.time.ZoneId; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Enumeration; +import java.util.List; import java.util.Objects; import java.util.jar.Attributes; +import java.util.jar.JarEntry; import java.util.jar.JarFile; +import java.util.jar.JarInputStream; import java.util.jar.JarOutputStream; import java.util.jar.Manifest; -import java.util.zip.CRC32; import java.util.zip.ZipEntry; import java.util.zip.ZipException; -import java.util.zip.ZipInputStream; import java.util.zip.ZipOutputStream; +import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; +import static java.util.jar.Attributes.Name.MANIFEST_VERSION; + /* * A class that will add an entry to the manifest and keep the same modification # times of the jar entries. @@ -43,7 +51,8 @@ public static void verboseLog(String logline) { public static void main(String[] args) throws IOException { Path out = null; Path source = null; - String manifestEntry = null; + List toAdd = new ArrayList<>(); + List toRemove = new ArrayList<>(); for (int i = 0; i < args.length; i++) { switch (args[i]) { @@ -52,7 +61,11 @@ public static void main(String[] args) throws IOException { break; case "--manifest-entry": - manifestEntry = args[++i]; + toAdd.add(args[++i]); + break; + + case "--remove-entry": + toRemove.add(args[++i]); break; case "--source": @@ -67,37 +80,56 @@ public static void main(String[] args) throws IOException { Objects.requireNonNull(source, "Source jar must be set."); Objects.requireNonNull(out, "Output path must be set."); - addEntryToManifest(out, source, manifestEntry, false); - } + if (isJarSigned(source)) { + verboseLog("Signed jar. Will not modify: " + source); + Files.createDirectories(out.getParent()); + Files.copy(source, out, REPLACE_EXISTING); + return; + } - public static void addEntryToManifest(Path out, Path source, String manifestEntry, boolean addManifest) throws IOException { - byte[] buffer = new byte[2048]; - int bytesRead; - boolean manifestUpdated = false; + addEntryToManifest(out, source, toAdd, toRemove); + } - try (InputStream fis = Files.newInputStream(source); - ZipInputStream zis = new ZipInputStream(fis)) { + private static boolean isJarSigned(Path source) throws IOException { + try (InputStream is = Files.newInputStream(source); + JarInputStream jis = new JarInputStream(is)) { + for (ZipEntry entry = jis.getNextEntry(); entry != null; entry = jis.getNextJarEntry()) { + if (entry.isDirectory()) { + continue; + } + if (entry.getName().startsWith("META-INF/") && entry.getName().endsWith(".SF")) { + return true; + } + } + } + return false; + } + public static void addEntryToManifest( + Path out, + Path source, + List toAdd, + List toRemove) throws IOException { + try (JarFile jarFile = new JarFile(source.toFile(), false)) { try (OutputStream fos = Files.newOutputStream(out); ZipOutputStream zos = new JarOutputStream(fos)) { - if (addManifest) { - verboseLog("INFO: No jar manifest found in " + source + " Adding new jar manifest."); - Manifest manifest = new Manifest(); - manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0"); - manifest.getMainAttributes().put(new Attributes.Name("Created-By"), "AddJarManifestEntry"); - String[] manifestEntryParts = manifestEntry.split(":", 2); - manifest.getMainAttributes().put(new Attributes.Name(manifestEntryParts[0]), manifestEntryParts[1]); - - ZipEntry newManifestEntry = new ZipEntry(JarFile.MANIFEST_NAME); - newManifestEntry.setTime(DEFAULT_TIMESTAMP); - zos.putNextEntry(newManifestEntry); - manifest.write(zos); - manifestUpdated = true; + // Rewrite the manifest first + Manifest manifest = jarFile.getManifest(); + if (manifest == null) { + manifest = new Manifest(); + manifest.getMainAttributes().put(MANIFEST_VERSION, "1.0"); } + amendManifest(manifest, toAdd, toRemove); + + ZipEntry newManifestEntry = new ZipEntry(JarFile.MANIFEST_NAME); + newManifestEntry.setTime(DEFAULT_TIMESTAMP); + zos.putNextEntry(newManifestEntry); + manifest.write(zos); - ZipEntry sourceEntry; - while ((sourceEntry = zis.getNextEntry()) != null) { + Enumeration entries = jarFile.entries(); + while (entries.hasMoreElements()) { + JarEntry sourceEntry = entries.nextElement(); String name = sourceEntry.getName(); ZipEntry outEntry = new ZipEntry(name); @@ -106,76 +138,38 @@ public static void addEntryToManifest(Path out, Path source, String manifestEntr outEntry.setComment(sourceEntry.getComment()); outEntry.setExtra(sourceEntry.getExtra()); - if (manifestEntry != null && JarFile.MANIFEST_NAME.equals(name)) { - Manifest manifest = new Manifest(zis); - String[] manifestEntryParts = manifestEntry.split(":", 2); - manifest.getMainAttributes().put(new Attributes.Name(manifestEntryParts[0]), manifestEntryParts[1]); - - if (sourceEntry.getMethod() == ZipEntry.STORED) { - CRC32OutputStream crc32OutputStream = new CRC32OutputStream(); - manifest.write(crc32OutputStream); - outEntry.setSize(crc32OutputStream.getSize()); - outEntry.setCrc(crc32OutputStream.getCRC()); - } - zos.putNextEntry(outEntry); - manifest.write(zos); - manifestUpdated = true; - - } else { + if (JarFile.MANIFEST_NAME.equals(name)) { + continue; + } - if (sourceEntry.getMethod() == ZipEntry.STORED) { - outEntry.setSize(sourceEntry.getSize()); - outEntry.setCrc(sourceEntry.getCrc()); - } + if (sourceEntry.getMethod() == ZipEntry.STORED) { + outEntry.setSize(sourceEntry.getSize()); + outEntry.setCrc(sourceEntry.getCrc()); + } - try { - zos.putNextEntry(outEntry); - } catch (ZipException e) { - if (e.getMessage().contains("duplicate entry:")) { - // If there is a duplicate entry we keep the first one we saw. - verboseLog("WARN: Skipping duplicate jar entry " + outEntry.getName() + " in " + source); - continue; - } else { - throw e; - } - } - while ((bytesRead = zis.read(buffer)) != -1) { - zos.write(buffer, 0, bytesRead); + try (InputStream in = jarFile.getInputStream(sourceEntry)) { + zos.putNextEntry(outEntry); + ByteStreams.copy(in, zos); + } catch (ZipException e) { + if (e.getMessage().contains("duplicate entry:")) { + // If there is a duplicate entry we keep the first one we saw. + verboseLog("WARN: Skipping duplicate jar entry " + outEntry.getName() + " in " + source); + continue; + } else { + throw e; } } } } } - - if (manifestEntry != null && !manifestUpdated) { - // If no manifest was found then re-run and add the MANIFEST.MF as the first entry in the output jar - addEntryToManifest(out, source, manifestEntry, true); - } } - // OutputStream to find the CRC32 of an updated STORED zip entry - private static class CRC32OutputStream extends java.io.OutputStream { - private final CRC32 crc = new CRC32(); - private long size = 0; - - CRC32OutputStream() {} - - public void write(int b) throws IOException { - crc.update(b); - size++; - } - - public void write(byte[] b, int off, int len) throws IOException { - crc.update(b, off, len); - size += len; - } - - public long getSize() { - return size; - } - - public long getCRC() { - return crc.getValue(); - } + private static void amendManifest(Manifest manifest, List toAdd, List toRemove) { + manifest.getMainAttributes().put(new Attributes.Name("Created-By"), "AddJarManifestEntry"); + toAdd.forEach(manifestEntry -> { + String[] manifestEntryParts = manifestEntry.split(":", 2); + manifest.getMainAttributes().put(new Attributes.Name(manifestEntryParts[0]), manifestEntryParts[1]); + }); + toRemove.forEach(name -> manifest.getMainAttributes().remove(new Attributes.Name(name))); } } diff --git a/private/tools/java/rules/jvm/external/jar/BUILD b/private/tools/java/rules/jvm/external/jar/BUILD index 742a828b5..45d3a786f 100644 --- a/private/tools/java/rules/jvm/external/jar/BUILD +++ b/private/tools/java/rules/jvm/external/jar/BUILD @@ -11,6 +11,9 @@ java_binary( visibility = [ "//visibility:public", ], + deps = [ + "//private/tools/java/rules/jvm/external:byte-streams", + ], ) java_binary( diff --git a/tests/com/jvm/external/jar/AddJarManifestEntryTest.java b/tests/com/jvm/external/jar/AddJarManifestEntryTest.java index 47b4ec0c4..241bf716b 100644 --- a/tests/com/jvm/external/jar/AddJarManifestEntryTest.java +++ b/tests/com/jvm/external/jar/AddJarManifestEntryTest.java @@ -13,10 +13,13 @@ import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.jar.Attributes; import java.util.jar.JarFile; +import java.util.jar.JarInputStream; import java.util.jar.JarOutputStream; import java.util.jar.Manifest; import java.util.zip.ZipEntry; @@ -24,7 +27,12 @@ import java.util.zip.ZipOutputStream; import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.jar.Attributes.Name.CLASS_PATH; +import static java.util.jar.Attributes.Name.MANIFEST_VERSION; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; public class AddJarManifestEntryTest { @@ -119,6 +127,149 @@ public void shouldOverwriteManifestEntryIfItAlreadyExistsInJar() throws IOExcept assertTrue(contents.get("META-INF/MANIFEST.MF").contains("Target-Label: @maven//:com_google_guava_guava")); } + @Test + public void doesNotAlterSignedJars() throws IOException { + Path input = temp.newFile("in.jar").toPath(); + Path output = temp.newFile("out.jar").toPath(); + + Manifest manifest = new Manifest(); + manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0"); + createJar(input, manifest, ImmutableMap.of("META-INF/SOME_FILE.SF", "")); + + AddJarManifestEntry.main(new String[]{ + "--output", output.toAbsolutePath().toString(), + "--source", input.toAbsolutePath().toString(), + "--manifest-entry", "Target-Label:@boo//:scary"}); + + byte[] in = Files.readAllBytes(input); + byte[] out = Files.readAllBytes(output); + + assertArrayEquals(in, out); + } + + @Test + public void shouldLeaveOriginalManifestInPlaceIfNoClassPathIsThere() throws IOException { + Path inJar = temp.newFile("input.jar").toPath(); + + Manifest manifest = new Manifest(); + manifest.getMainAttributes().put(MANIFEST_VERSION, "1.0.0"); + Attributes.Name name = new Attributes.Name("Cheese"); + + manifest.getMainAttributes().put(name, "Roquefort"); + createJar(inJar, manifest, new HashMap<>()); + + Path outJar = temp.newFile("output.jar").toPath(); + + AddJarManifestEntry.main(new String[]{ + "--source", inJar.toAbsolutePath().toString(), + "--output", outJar.toAbsolutePath().toString(), + "--remove-entry", "Class-Path" + }); + + try (InputStream is = Files.newInputStream(outJar); + JarInputStream jis = new JarInputStream(is)) { + Manifest read = jis.getManifest(); + + assertEquals("Roquefort", read.getMainAttributes().get(name)); + } + } + + @Test + public void shouldRemoveClassPathFromManifestIfPresent() throws IOException { + Path inJar = temp.newFile("input.jar").toPath(); + + Manifest manifest = new Manifest(); + manifest.getMainAttributes().put(MANIFEST_VERSION, "1.0.0"); + + manifest.getMainAttributes().put(CLASS_PATH, "Brie"); + createJar(inJar, manifest, new HashMap<>()); + + Path outJar = temp.newFile("output.jar").toPath(); + AddJarManifestEntry.main(new String[]{ + "--source", inJar.toAbsolutePath().toString(), + "--output", outJar.toAbsolutePath().toString(), + "--remove-entry", "Class-Path" + }); + + try (InputStream is = Files.newInputStream(outJar); + JarInputStream jis = new JarInputStream(is)) { + Manifest read = jis.getManifest(); + + assertNull(read.getMainAttributes().get(CLASS_PATH)); + } + } + + /** + * There are jars in the wild which cannot be unpacked by zip or bazel's + * own zipper. These fail to unpack because the jar contains a directory + * and a file that would unpack to the same path. Make sure our header + * jar creator handles this. + */ + @Test + public void shouldBeAbleToCopeWithUnpackableJars() throws IOException { + Path inJar = temp.newFile("input.jar").toPath(); + + try (OutputStream is = Files.newOutputStream(inJar); + JarOutputStream jos = new JarOutputStream(is)) { + ZipEntry entry = new ZipEntry("foo"); + jos.putNextEntry(entry); + jos.write("Hello, World!".getBytes(UTF_8)); + + entry = new ZipEntry("foo/"); + jos.putNextEntry(entry); + } + + Path outJar = temp.newFile("output.jar").toPath(); + + // No exception is a Good Thing + AddJarManifestEntry.main(new String[]{ + "--source", inJar.toAbsolutePath().toString(), + "--output", outJar.toAbsolutePath().toString(), + "--remove-entry", "Class-Path" + }); + } + + /** + * One pattern for making "executable jars" is to use a preamble to + * the zip archive. When this is done `ZipInputStream` wrongly + * indicates that there are no entries in the jar, which means that + * the header jar generated is empty, which leads to obvious issues. + */ + @Test + public void shouldStillIncludeClassesIfThereIsAShellPreambleToTheJar() throws IOException { + Path tempJar = temp.newFile("regular.jar").toPath(); + + createJar( + tempJar, + null, + ImmutableMap.of( + "Foo.class", "0xDECAFBAD", + "Bar.class", "0xDEADBEEF")); + + // Write the preamble. We do things this way because a plain text + // file is not a valid zip file. + Path inJar = temp.newFile("input.jar").toPath(); + Files.write(inJar, "#!/bin/bash\necho Hello, World\n\n".getBytes(UTF_8)); + + try (InputStream is = Files.newInputStream(tempJar); + OutputStream os = Files.newOutputStream(inJar, StandardOpenOption.APPEND)) { + ByteStreams.copy(is, os); + } + + Path outJar = temp.newFile("output.jar").toPath(); + + AddJarManifestEntry.main(new String[] { + "--source", inJar.toAbsolutePath().toString(), + "--output", outJar.toAbsolutePath().toString(), + "--manifest-entry", "Target-Label:@maven//:com_google_guava_guava" + }); + + try (JarFile jarFile = new JarFile(outJar.toFile())) { + assertNotNull(jarFile.getEntry("Foo.class")); + assertNotNull(jarFile.getEntry("Bar.class")); + } + } + private void createJar(Path outputTo, Manifest manifest, Map pathToContents) throws IOException { try (OutputStream os = Files.newOutputStream(outputTo); ZipOutputStream zos = new JarOutputStream(os)) { diff --git a/tests/com/jvm/external/jar/BUILD b/tests/com/jvm/external/jar/BUILD index 814da6774..88fd1acbd 100644 --- a/tests/com/jvm/external/jar/BUILD +++ b/tests/com/jvm/external/jar/BUILD @@ -5,6 +5,7 @@ java_test( srcs = ["AddJarManifestEntryTest.java"], test_class = "com.jvm.external.jar.AddJarManifestEntryTest", deps = [ + "//private/tools/java/rules/jvm/external:byte-streams", "//private/tools/java/rules/jvm/external/jar:AddJarManifestEntry", artifact("com.google.guava:guava"), ],