Single-jar pack_sources should conform to Bazel behavior (#1378)

Single-jar pack_sources should conform to Bazel behaviour

Author: wiwa

scala/private/phases/phase_compile.bzl

@@ -308,17 +308,33 @@ def _create_scala_compilation_provider(ctx, ijar, source_jar, deps_providers):
         neverlink = ctx.attr.neverlink,
     )
 
-def _pack_source_jar(ctx, scala_srcs, in_srcjars):
-    output_jar = ctx.outputs.jar
-    source_jar_name = output_jar.basename[:-len(output_jar.extension)] + "-src.jar"
-    output_source_jar = ctx.actions.declare_file(source_jar_name, sibling = output_jar)
-    return java_common.pack_sources(
-        ctx.actions,
-        output_source_jar = output_source_jar,
-        sources = scala_srcs,
-        source_jars = in_srcjars,
-        java_toolchain = find_java_toolchain(ctx, ctx.attr._java_toolchain),
-    )
+def _pack_source_jar(ctx, scala_srcs, input_srcjars):
+    # https://github.com/bazelbuild/bazel/blob/ff6c0333e4f957bb9f7ab5401b01dbf3e9b515b1/src/main/java/com/google/devtools/build/lib/rules/java/JavaInfoBuildHelper.java#L180-L183
+    # java_common.pack_sources checks for no srcs and only a single input jar
+    # if so, it checks that output_source_jar is null
+    # passing that, it will return the input source jar directly
+    # However, pack_sources will FAIL if both output_source_jar and
+    # the deprecated output_jar field are BOTH null
+    # Therefore, we can return the single input jar ourselves
+    if not scala_srcs and len(input_srcjars) == 1:
+        return input_srcjars[0]
+    else:
+        output_jar = ctx.outputs.jar
+        without_ext = output_jar.basename
+        if output_jar.extension:
+            ext_len = len("." + output_jar.extension)
+            without_ext = output_jar.basename[:-ext_len]
+        source_jar_name = without_ext + "-src.jar"
+
+        output_source_jar = ctx.actions.declare_file(source_jar_name, sibling = output_jar)
+
+        return java_common.pack_sources(
+            ctx.actions,
+            output_source_jar = output_source_jar,
+            sources = scala_srcs,
+            source_jars = input_srcjars,
+            java_toolchain = find_java_toolchain(ctx, ctx.attr._java_toolchain),
+        )
 
 def _try_to_compile_java_jar(
         ctx,

test/src/main/scala/scalarules/test/srcjars/BUILD

@@ -1,19 +1,47 @@
 load("//scala:scala.bzl", "scala_library")
+load(":pack_sources_test.bzl", "pack_sources_test_suite")
 
 #TODO the way it SHOULD work (but isn't currently) is that
 # the source_jar target should make available a compiled jar,
 # and use_source_jar should depend on that internally
 
 scala_library(
     name = "source_jar",
-    # SourceJar1.jar was created by:
+    # SourceJar1.srcjar was created by:
     # jar -cfM test/src/main/scala/scalarules/test/srcjars/SourceJar1.srcjar \
     # test/src/main/scala/scalarules/test/srcjars/SourceJar1.scala
     srcs = ["SourceJar1.srcjar"],
 )
 
+scala_library(
+    name = "source_jar_no_expect_java_output",
+    srcs = ["SourceJar1.srcjar"],
+    expect_java_output = False,
+)
+
+scala_library(
+    name = "multi_source_jar",
+    # SourceJar2.srcjar was created the same way as SourceJar1.srcjar, i.e. by:
+    # jar -cfM test/src/main/scala/scalarules/test/srcjars/SourceJar2.srcjar \
+    # test/src/main/scala/scalarules/test/srcjars/SourceJar2.scala
+    srcs = [
+        "SourceJar1.srcjar",
+        "SourceJar2.srcjar",
+    ],
+)
+
+scala_library(
+    name = "mixed_source_jar",
+    srcs = [
+        "SourceJar1.srcjar",
+        "SourceJar2.scala",
+    ],
+)
+
 scala_library(
     name = "use_source_jar",
     srcs = ["SourceJar2.scala"],
     deps = [":source_jar"],
 )
+
+pack_sources_test_suite(name = "pack_sources_test")

test/src/main/scala/scalarules/test/srcjars/SourceJar2.srcjar

@@ -0,0 +1,88 @@
+load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts")
+load("@bazel_skylib//lib:new_sets.bzl", "sets")
+
+# Tests and documents the functionality of phase_compile.bzl's _pack_source_jar
+
+def _source_jar_test_impl(ctx):
+    env = analysistest.begin(ctx)
+
+    target_under_test = analysistest.target_under_test(env)
+
+    srcjar_names = sets.make(
+        [j.basename for j in target_under_test[JavaInfo].source_jars],
+    )
+
+    expected_names = sets.make(ctx.attr.expected_basenames)
+
+    asserts.set_equals(env, expected = expected_names, actual = srcjar_names)
+
+    return analysistest.end(env)
+
+def _make_source_jar_test():
+    return analysistest.make(
+        impl = _source_jar_test_impl,
+        attrs = {
+            "expected_basenames": attr.string_list(
+                mandatory = True,
+                allow_empty = True,
+            ),
+        },
+    )
+
+source_jar_test = _make_source_jar_test()
+
+def pack_sources_test_suite(name):
+    source_jar_test(
+        name = "single_source_jar_test",
+        target_under_test = ":source_jar",
+
+        # In line with Bazel's java_common.pack_sources,
+        # We return the initial .srcjar file since there are no source files
+        # Not sure where the second -src.jar comes from, maybe due to java rules
+        # It can be removed by adding the target attr expect_java_output = False
+        expected_basenames = [
+            "SourceJar1.srcjar",
+            "source_jar_java-src.jar",
+        ],
+    )
+    source_jar_test(
+        name = "single_source_jar_no_java_output_test",
+        target_under_test = ":source_jar_no_expect_java_output",
+        expected_basenames = [
+            "SourceJar1.srcjar",
+        ],
+    )
+    source_jar_test(
+        name = "multi_source_jar_test",
+        target_under_test = ":multi_source_jar",
+        expected_basenames = [
+            "multi_source_jar-src.jar",
+            "multi_source_jar_java-src.jar",
+        ],
+    )
+    source_jar_test(
+        name = "mixed_source_jar_test",
+        target_under_test = ":mixed_source_jar",
+        expected_basenames = [
+            "mixed_source_jar-src.jar",
+            "mixed_source_jar_java-src.jar",
+        ],
+    )
+    source_jar_test(
+        name = "source_jar_with_srcs_test",
+        target_under_test = ":use_source_jar",
+        expected_basenames = [
+            "use_source_jar-src.jar",
+        ],
+    )
+
+    native.test_suite(
+        name = name,
+        tests = [
+            ":single_source_jar_test",
+            ":single_source_jar_no_java_output_test",
+            ":multi_source_jar_test",
+            ":mixed_source_jar_test",
+            ":source_jar_with_srcs_test",
+        ],
+    )