[java_export]: Smarter handling of duplicate file names

In bazel-contrib#1239 we essentially allowed anything that wasn't a class file to be
included in the jar more than once. While this was useful in some cases
it meant that if we pulled in dependencies generated by (eg) an aspect,
such as when using protobufs and packing the original proto files, this
can lead to unwanted to duplicates.

We now allow users more control of what to include, though we always
include files that are often legally required (eg. the LICENSE file)
which should strike a balance between flexibility and Doing The Right
Thing by default.

Author: shs96c

MODULE.bazel

@@ -178,6 +178,7 @@ dev_maven = use_extension(
 dev_maven.install(
     artifacts = [
         "com.google.guava:guava:31.1-jre",
+        "com.google.protobuf:protobuf-java:4.29.4",
         "org.hamcrest:hamcrest-core:2.1",
         "io.netty:netty-tcnative-boringssl-static:2.0.61.Final",
     ],

maven_install.json

@@ -1,7 +1,7 @@
 {
   "__AUTOGENERATED_FILE_DO_NOT_MODIFY_THIS_FILE_MANUALLY": "THERE_IS_NO_DATA_ONLY_ZUUL",
-  "__INPUT_ARTIFACTS_HASH": -1642503178,
-  "__RESOLVED_ARTIFACTS_HASH": -1242467072,
+  "__INPUT_ARTIFACTS_HASH": 1448912684,
+  "__RESOLVED_ARTIFACTS_HASH": 771049556,
   "artifacts": {
     "com.google.code.findbugs:jsr305": {
       "shasums": {
@@ -39,6 +39,12 @@
       },
       "version": "1.3"
     },
+    "com.google.protobuf:protobuf-java": {
+      "shasums": {
+        "jar": "b5ae02c6b76780261852cdc2549c1e36a6b4cc9d339ef80a8ef7c599c9775a96"
+      },
+      "version": "4.29.4"
+    },
     "io.netty:netty-tcnative-boringssl-static": {
       "shasums": {
         "jar": "b6f974972c44cd6f9cecabc255290286faac40b6393c66c3c3c0db7f421cc28e",
@@ -167,6 +173,10 @@
     "com.google.j2objc:j2objc-annotations": [
       "com.google.j2objc.annotations"
     ],
+    "com.google.protobuf:protobuf-java": [
+      "com.google.protobuf",
+      "com.google.protobuf.compiler"
+    ],
     "io.netty:netty-tcnative-classes": [
       "io.netty.internal.tcnative"
     ],
@@ -226,6 +236,7 @@
       "com.google.guava:guava",
       "com.google.guava:listenablefuture",
       "com.google.j2objc:j2objc-annotations",
+      "com.google.protobuf:protobuf-java",
       "io.netty:netty-tcnative-boringssl-static",
       "io.netty:netty-tcnative-boringssl-static:jar:linux-aarch_64",
       "io.netty:netty-tcnative-boringssl-static:jar:linux-x86_64",

private/rules/java_export.bzl

@@ -12,6 +12,7 @@ def java_export(
         deploy_env = [],
         excluded_workspaces = {name: None for name in DEFAULT_EXCLUDED_WORKSPACES},
         pom_template = None,
+        allowed_duplicate_names = None,
         visibility = None,
         tags = [],
         testonly = None,
@@ -64,6 +65,8 @@ def java_export(
       maven_coordinates: The maven coordinates for this target.
       pom_template: The template to be used for the pom.xml file.
       manifest_entries: A dict of `String: String` containing additional manifest entry attributes and values.
+      allowed_duplicate_names: A list of `String` containing patterns for files that can be included more than
+        once in the jar file. Examples include `["log4j.properties"]`
       deploy_env: A list of labels of Java targets to exclude from the generated jar.
         [`java_binary`](https://bazel.build/reference/be/java#java_binary) targets are *not*
         supported.
@@ -113,6 +116,7 @@ def java_export(
         deploy_env = deploy_env,
         excluded_workspaces = excluded_workspaces,
         pom_template = pom_template,
+        allowed_duplicate_names = allowed_duplicate_names,
         visibility = visibility,
         tags = tags,
         testonly = testonly,
@@ -132,6 +136,7 @@ def maven_export(
         deploy_env = [],
         excluded_workspaces = {},
         pom_template = None,
+        allowed_duplicate_names = None,
         visibility = None,
         tags = [],
         testonly = False,
@@ -210,6 +215,7 @@ def maven_export(
     manifest_entries = manifest_entries if manifest_entries else {}
     deploy_env = deploy_env if deploy_env else []
     excluded_workspaces = excluded_workspaces if excluded_workspaces else {}
+    allowed_duplicate_names = allowed_duplicate_names if allowed_duplicate_names else []
     doc_url = doc_url if doc_url else ""
     doc_deps = doc_deps if doc_deps else []
     tags = tags if tags else []
@@ -228,6 +234,7 @@ def maven_export(
         deploy_env = deploy_env,
         excluded_workspaces = excluded_workspaces.keys(),
         additional_dependencies = additional_dependencies,
+        allowed_duplicate_names = allowed_duplicate_names,
         visibility = visibility,
         tags = tags + maven_coordinates_tags,
         testonly = testonly,

private/rules/maven_project_jar.bzl

@@ -29,11 +29,12 @@ def _strip_excluded_workspace_jars(jar_files, excluded_workspaces):
 
     return to_return
 
-def _combine_jars(ctx, merge_jars, inputs, excludes, output):
+def _combine_jars(ctx, merge_jars, inputs, excludes, allowed_duplicates, output):
     args = ctx.actions.args()
     args.add("--output", output)
     args.add_all(inputs, before_each = "--sources")
     args.add_all(excludes, before_each = "--exclude")
+    args.add_all(allowed_duplicates, before_each = "--allow-duplicate")
 
     ctx.actions.run(
         mnemonic = "MergeJars",
@@ -74,6 +75,7 @@ def _maven_project_jar_impl(ctx):
         depset(transitive =
                    [ji.transitive_runtime_jars for ji in info.dep_infos.to_list()] +
                    [jar[JavaInfo].transitive_runtime_jars for jar in ctx.attr.deploy_env]),
+        ctx.attr.allowed_duplicate_names,
         intermediate_jar,
     )
 
@@ -121,6 +123,7 @@ def _maven_project_jar_impl(ctx):
         depset(transitive =
                    [ji.transitive_source_jars for ji in info.dep_infos.to_list()] +
                    [jar[JavaInfo].transitive_source_jars for jar in ctx.attr.deploy_env]),
+        [],
         src_jar,
     )
 
@@ -229,6 +232,10 @@ single artifact that other teams can download and use.
                 [JavaInfo],
             ],
         ),
+        "allowed_duplicate_names": attr.string_list(
+            doc = "A list of patterns (compiled as a java regex) that may be duplicated within the generated jar",
+            allow_empty = True,
+        ),
         "_add_jar_manifest_entry": attr.label(
             executable = True,
             cfg = "exec",

private/tools/java/com/github/bazelbuild/rules_jvm_external/jar/MergeJars.java

@@ -43,20 +43,30 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.function.Predicate;
 import java.util.jar.Attributes;
 import java.util.jar.JarOutputStream;
 import java.util.jar.Manifest;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
 import java.util.zip.ZipInputStream;
 
 public class MergeJars {
 
+  private static final Set<Predicate<String>> ALWAYS_ALLOW_DUPLICATES =
+      Set.of("COPYRIGHT", "LICENSE", "NOTICE").stream()
+          .map(Pattern::compile)
+          .map(Pattern::asPredicate)
+          .collect(Collectors.toSet());
+
   public static void main(String[] args) throws IOException {
     Path out = null;
     // Insertion order may matter
     Set<Path> sources = new LinkedHashSet<>();
     Set<Path> excludes = new HashSet<>();
+    Set<Predicate<String>> duplicateAllowList = new HashSet<>(ALWAYS_ALLOW_DUPLICATES);
     DuplicateEntryStrategy onDuplicate = LAST_IN_WINS;
 
     for (int i = 0; i < args.length; i++) {
@@ -66,6 +76,10 @@ public static void main(String[] args) throws IOException {
           // ignore
           break;
 
+        case "--allow-duplicate":
+          duplicateAllowList.add(Pattern.compile(args[++i]).asPredicate());
+          break;
+
         case "--duplicates":
           onDuplicate = DuplicateEntryStrategy.fromShortName(args[++i]);
           break;
@@ -111,7 +125,7 @@ public static void main(String[] args) throws IOException {
     manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0");
 
     Map<String, List<String>> allServices = new TreeMap<>();
-    Set<String> excludedPaths = readExcludedClassNames(excludes);
+    Set<String> excludedPaths = readExcludedFileNames(excludes, duplicateAllowList);
 
     // Ultimately, we want the entries in the output zip to be sorted
     // so that we have a deterministic output.
@@ -295,7 +309,8 @@ private static void createDirectories(JarOutputStream jos, String name, Set<Stri
     }
   }
 
-  private static Set<String> readExcludedClassNames(Set<Path> excludes) throws IOException {
+  private static Set<String> readExcludedFileNames(
+      Set<Path> excludes, Set<Predicate<String>> duplicateAllowList) throws IOException {
     Set<String> paths = new HashSet<>();
 
     for (Path exclude : excludes) {
@@ -307,7 +322,10 @@ private static Set<String> readExcludedClassNames(Set<Path> excludes) throws IOE
           if (entry.isDirectory()) {
             continue;
           }
-          if (!entry.getName().endsWith(".class")) {
+
+          // We hope that the duplicate allow list is nice and short
+          ZipEntry finalEntry = entry;
+          if (duplicateAllowList.stream().anyMatch(pred -> pred.test(finalEntry.getName()))) {
             continue;
           }
 

tests/com/github/bazelbuild/rules_jvm_external/jar/MergeJarsTest.java

@@ -600,6 +600,7 @@ public void mergedJarKeepsNonClassFiles() throws IOException {
           "--output", outputJar.toAbsolutePath().toString(),
           "--sources", inputOne.toAbsolutePath().toString(),
           "--exclude", excludeOne.toAbsolutePath().toString(),
+          "--allow-duplicate", "log4j.properties",
         });
 
     Map<String, String> contents = readJar(outputJar);
@@ -626,6 +627,7 @@ public void mergedJarKeepsNonClassFilesDefaultDuplicateStrategy() throws IOExcep
           "--sources", inputOne.toAbsolutePath().toString(),
           "--sources", inputTwo.toAbsolutePath().toString(),
           "--exclude", excludeOne.toAbsolutePath().toString(),
+          "--allow-duplicate", "log4j.properties",
         });
 
     Map<String, String> contents = readJar(outputJar);
@@ -653,6 +655,7 @@ public void mergedJarKeepsNonClassFilesFirstWinsStrategy() throws IOException {
           "--sources", inputTwo.toAbsolutePath().toString(),
           "--duplicates", "first-wins",
           "--exclude", excludeOne.toAbsolutePath().toString(),
+          "--allow-duplicate", "log4j.properties",
         });
 
     Map<String, String> contents = readJar(outputJar);

tests/integration/duplicates_in_java_export/AllowedDuplicatesTest.java

@@ -0,0 +1,59 @@
+package com.github.bazelbuild.rules_jvm_external.duplicates_in_java_export;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.BufferedInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.jar.JarInputStream;
+import java.util.zip.ZipEntry;
+import org.junit.Test;
+
+public class AllowedDuplicatesTest {
+
+  @Test
+  public void shouldNormallyDisallowDuplicates() {
+    Path childWithoutParent = Paths.get(System.getProperty("child.without.parent"));
+    assertTrue(Files.exists(childWithoutParent));
+
+    // Find all the `.proto` files in the jar
+    Set<String> fileNames = getProtoFileNames(childWithoutParent);
+    assertEquals(Set.of("child.proto"), fileNames);
+  }
+
+  @Test
+  public void shouldAllowNamedDuplicates() {
+    Path childWithParent = Paths.get(System.getProperty("child.with.parent"));
+    assertTrue(Files.exists(childWithParent));
+
+    // Find all the `.proto` files in the jar
+    Set<String> fileNames = getProtoFileNames(childWithParent);
+    assertEquals(Set.of("child.proto", "parent.proto"), fileNames);
+  }
+
+  Set<String> getProtoFileNames(Path jar) {
+    Set<String> fileNames = new HashSet<>();
+    try (InputStream is = Files.newInputStream(jar);
+        InputStream bis = new BufferedInputStream(is);
+        JarInputStream jis = new JarInputStream(is)) {
+
+      for (ZipEntry entry = jis.getNextEntry(); entry != null; entry = jis.getNextEntry()) {
+        if (entry.getName().endsWith(".proto")) {
+          fileNames.add(entry.getName());
+        }
+      }
+
+      return Set.copyOf(fileNames);
+    } catch (IOException e) {
+      fail(e.getMessage());
+      throw new RuntimeException(e);
+    }
+  }
+}