Don't build unnecessary jars. (#5515)

* Don't build unnecessary jars.

* Remove unnecessary assignment

* Put feature behind a flag/expirement

* fix

* Update aspect/testing/tests/src/com/google/idea/blaze/aspect/java/dependencies/DependenciesTest.java

* Aspect parameter attribute 'optimize_building_jars' must have type 'string' and use the 'values' restriction.

* revert test modifcation

---------

Co-authored-by: Uri Baghin <[email protected]>

aspect/intellij_info_impl.bzl

@@ -18,7 +18,6 @@ load(
     "ACTION_NAMES",
 )
 
-
 # Defensive list of features that can appear in the C++ toolchain, but which we
 # definitely don't want to enable (when enabled, they'd contribute command line
 # flags that don't make sense in the context of intellij info).
@@ -195,13 +194,20 @@ def annotation_processing_jars(generated_class_jar, generated_source_jar):
         source_jars = [artifact_location(src_jar)] if src_jar else None,
     )
 
-def jars_from_output(output):
+def jars_from_output(output, should_optimize_building_jars):
     """Collect jars for intellij-resolve-files from Java output."""
     if output == None:
         return []
+    source_jars = get_source_jars(output)
+    if should_optimize_building_jars:
+        return [
+            jar
+            for jar in ([output.ijar if len(source_jars) > 0 and output.ijar else output.class_jar] + source_jars)
+            if jar != None and not jar.is_source
+        ]
     return [
         jar
-        for jar in ([output.class_jar, output.ijar] + get_source_jars(output))
+        for jar in ([output.class_jar, output.ijar] + source_jars)
         if jar != None and not jar.is_source
     ]
 
@@ -599,6 +605,11 @@ def _collect_generated_files(java):
         return [(java.annotation_processing.class_jar, java.annotation_processing.source_jar)]
     return []
 
+def _should_optimize_building_jars(ctx):
+    if ctx.attr.optimize_building_jars == "enabled":
+        return True
+    return False
+
 def collect_java_info(target, ctx, semantics, ide_info, ide_info_file, output_groups):
     """Updates Java-specific output groups, returns false if not a Java target."""
     java = get_java_provider(target)
@@ -619,7 +630,7 @@ def collect_java_info(target, ctx, semantics, ide_info, ide_info_file, output_gr
     sources = sources_from_target(ctx)
     jars = [library_artifact(output, ctx.rule.kind) for output in java_outputs]
     class_jars = [output.class_jar for output in java_outputs if output and output.class_jar]
-    output_jars = [jar for output in java_outputs for jar in jars_from_output(output)]
+    output_jars = [jar for output in java_outputs for jar in jars_from_output(output, _should_optimize_building_jars(ctx))]
     resolve_files = output_jars
     compile_files = class_jars
 
@@ -762,16 +773,21 @@ def _build_filtered_gen_jar(ctx, target, java_outputs, gen_java_sources, srcjars
     """Filters the passed jar to contain only classes from the given manifest."""
     jar_artifacts = []
     source_jar_artifacts = []
+    should_optimize_building_jars = _should_optimize_building_jars(ctx)
     for jar in java_outputs:
         if jar.ijar:
             jar_artifacts.append(jar.ijar)
-        elif jar.class_jar:
+        elif jar.class_jar and not should_optimize_building_jars:
             jar_artifacts.append(jar.class_jar)
         if hasattr(jar, "source_jars") and jar.source_jars:
             source_jar_artifacts.extend(_list_or_depset_to_list(jar.source_jars))
         elif hasattr(jar, "source_jar") and jar.source_jar:
             source_jar_artifacts.append(jar.source_jar)
 
+    if should_optimize_building_jars:
+        if len(source_jar_artifacts) == 0 or len(jar_artifacts) == 0:
+            jar_artifacts.extend([jar.class_jar for jar in java_outputs.jars if jar.class_jar])
+
     filtered_jar = ctx.actions.declare_file(target.label.name + "-filtered-gen.jar")
     filtered_source_jar = ctx.actions.declare_file(target.label.name + "-filtered-gen-src.jar")
     args = []
@@ -877,7 +893,7 @@ def _collect_android_ide_info(target, ctx, semantics, ide_info, ide_info_file, o
 
     resources = []
     res_folders = []
-    resolve_files = jars_from_output(output_jar)
+    resolve_files = jars_from_output(output_jar, _should_optimize_building_jars(ctx))
     if hasattr(ctx.rule.attr, "resource_files"):
         for artifact_path_fragments, res_files in get_res_artifacts(ctx.rule.attr.resource_files).items():
             # Generate unique ArtifactLocation for resource directories.
@@ -949,7 +965,7 @@ def _collect_android_ide_info(target, ctx, semantics, ide_info, ide_info_file, o
     # b/176209293: expose resource jar to make sure empty library
     # knows they are remote output artifact
     if android.resource_jar:
-        resolve_files += [jar for jar in jars_from_output(android.resource_jar)]
+        resolve_files += [jar for jar in jars_from_output(android.resource_jar, _should_optimize_building_jars(ctx))]
 
     ide_info["android_ide_info"] = android_info
     update_sync_output_groups(output_groups, "intellij-resolve-android", depset(resolve_files))
@@ -1261,6 +1277,11 @@ def make_intellij_info_aspect(aspect_impl, semantics):
             executable = True,
             allow_files = True,
         ),
+        "optimize_building_jars": attr.string(
+            doc = "Reduce the number of jars built by the aspect and tracked by the ide",
+            default = "disabled",
+            values = ["enabled", "disabled"],
+        ),
     }
 
     # add attrs required by semantics

aspect/testing/rules/intellij_aspect_test_fixture.bzl

@@ -61,16 +61,22 @@ _intellij_aspect_test_fixture = rule(
             executable = True,
             allow_files = True,
         ),
+        "optimize_building_jars": attr.string(
+            doc = "Reduce the number of jars built by the aspect and tracked by the ide",
+            default = "disabled",
+            values = ["enabled", "disabled"],
+        ),
     },
 )
 
-def intellij_aspect_test_fixture(name, deps, transitive_configs = []):
+def intellij_aspect_test_fixture(name, deps, transitive_configs = [], optimize_building_jars = "disabled"):
     _intellij_aspect_test_fixture(
         name = name,
         output = name + ".intellij-aspect-test-fixture",
         deps = deps,
         testonly = 1,
         transitive_configs = transitive_configs,
+        optimize_building_jars = optimize_building_jars,
     )
 
 def test_sources(outs):

aspect/testing/tests/src/com/google/idea/blaze/aspect/general/noide/BUILD

@@ -25,6 +25,16 @@ java_library(
 
 intellij_aspect_test_fixture(
     name = "noide_fixture",
+    optimize_building_jars = "disabled",
+    deps = [
+        ":baz",
+        ":foo",
+    ],
+)
+
+intellij_aspect_test_fixture(
+    name = "noide_fixture_optimize_building_jars",
+    optimize_building_jars = "enabled",
     deps = [
         ":baz",
         ":foo",
@@ -34,7 +44,10 @@ intellij_aspect_test_fixture(
 java_test(
     name = "NoIdeTest",
     srcs = ["NoIdeTest.java"],
-    data = [":noide_fixture"],
+    data = [
+        ":noide_fixture",
+        ":noide_fixture_optimize_building_jars",
+    ],
     deps = [
         "//aspect/testing:BazelIntellijAspectTest",
         "//aspect/testing:guava",

aspect/testing/tests/src/com/google/idea/blaze/aspect/general/noide/NoIdeTest.java

@@ -45,4 +45,23 @@ public void testNoIde() throws Exception {
     assertThat(getOutputGroupFiles(testFixture, "intellij-compile-java"))
         .containsExactly(testRelative("libfoo.jar"));
   }
+
+  @Test
+  public void testNoIdeOptimizedJars() throws Exception {
+    IntellijAspectTestFixture testFixture = loadTestFixture(":noide_fixture_optimize_building_jars");
+    assertThat(findTarget(testFixture, ":foo")).isNotNull();
+    assertThat(findTarget(testFixture, ":bar")).isNull();
+    assertThat(findTarget(testFixture, ":baz")).isNull();
+
+    assertThat(getOutputGroupFiles(testFixture, "intellij-info-java"))
+        .containsAtLeast(
+            testRelative("foo.java-manifest"), testRelative(intellijInfoFileName("foo")));
+    assertThat(getOutputGroupFiles(testFixture, "intellij-resolve-java"))
+        .containsExactly(
+            testRelative("libfoo-hjar.jar"),
+            testRelative("libfoo-src.jar"),
+            testRelative("libfoo.jdeps"));
+    assertThat(getOutputGroupFiles(testFixture, "intellij-compile-java"))
+        .containsExactly(testRelative("libfoo.jar"));
+  }
 }

aspect/testing/tests/src/com/google/idea/blaze/aspect/java/dependencies/DependenciesTest.java

@@ -54,13 +54,10 @@ public void testJavaLibraryWithTransitiveDependencies() throws Exception {
             testRelative(intellijInfoFileName("transitive_dep")));
     assertThat(getOutputGroupFiles(testFixture, "intellij-resolve-java"))
         .containsAtLeast(
-            testRelative("libfoo.jar"),
             testRelative("libfoo-hjar.jar"),
             testRelative("libfoo-src.jar"),
-            testRelative("libsingle_dep.jar"),
             testRelative("libsingle_dep-hjar.jar"),
             testRelative("libsingle_dep-src.jar"),
-            testRelative("libtransitive_dep.jar"),
             testRelative("libtransitive_dep-hjar.jar"),
             testRelative("libtransitive_dep-src.jar"));
     assertThat(getOutputGroupFiles(testFixture, "intellij-compile-java"))
@@ -103,13 +100,10 @@ public void testJavaLibraryWithExports() throws Exception {
             testRelative(intellijInfoFileName("export_consumer")));
     assertThat(getOutputGroupFiles(testFixture, "intellij-resolve-java"))
         .containsAtLeast(
-            testRelative("libfoo.jar"),
             testRelative("libfoo-hjar.jar"),
             testRelative("libfoo-src.jar"),
-            testRelative("libfoo_exporter.jar"),
             testRelative("libfoo_exporter-hjar.jar"),
             testRelative("libfoo_exporter-src.jar"),
-            testRelative("libexport_consumer.jar"),
             testRelative("libexport_consumer-hjar.jar"),
             testRelative("libexport_consumer-src.jar"));
     assertThat(getOutputGroupFiles(testFixture, "intellij-compile-java"))

aspect/testing/tests/src/com/google/idea/blaze/aspect/java/javalibrary/BUILD

@@ -30,6 +30,13 @@ java_library(
 
 intellij_aspect_test_fixture(
     name = "foo_fixture",
+    optimize_building_jars = "disabled",
+    deps = [":foo"],
+)
+
+intellij_aspect_test_fixture(
+    name = "foo_fixture_optimize_building_jars",
+    optimize_building_jars = "enabled",
     deps = [":foo"],
 )
 
@@ -56,15 +63,17 @@ java_library(
 
 intellij_aspect_test_fixture(
     name = "foo_exports_fixture",
+    optimize_building_jars = "disabled",
     deps = [":foo_exports"],
 )
 
 java_test(
     name = "JavaLibraryTest",
     srcs = ["JavaLibraryTest.java"],
     data = [
-        "foo_exports_fixture",
+        ":foo_exports_fixture",
         ":foo_fixture",
+        ":foo_fixture_optimize_building_jars",
     ],
     deps = [
         "//aspect/testing:BazelIntellijAspectTest",

aspect/testing/tests/src/com/google/idea/blaze/aspect/java/javalibrary/JavaLibraryTest.java

@@ -186,4 +186,109 @@ public void testJavaLibrary() throws Exception {
 
     assertThat(target.getJavaIdeInfo().getMainClass()).isEmpty();
   }
+
+  @Test
+  public void testJavaLibraryOptimizedJars() throws Exception {
+    IntellijAspectTestFixture testFixture = loadTestFixture(":foo_fixture_optimize_building_jars");
+    TargetIdeInfo target = findTarget(testFixture, ":foo");
+
+    assertThat(target.getKindString()).isEqualTo("java_library");
+    assertThat(target.hasJavaIdeInfo()).isTrue();
+    assertThat(target.hasCIdeInfo()).isFalse();
+    assertThat(target.hasAndroidIdeInfo()).isFalse();
+    assertThat(target.hasPyIdeInfo()).isFalse();
+
+    assertThat(relativePathsForArtifacts(target.getJavaIdeInfo().getSourcesList()))
+        .containsExactly(testRelative("Foo.java"));
+    assertThat(
+        target.getJavaIdeInfo().getJarsList().stream()
+            .map(IntellijAspectTest::libraryArtifactToString)
+            .collect(toList()))
+        .containsExactly(
+            jarString(
+                testRelative("libfoo.jar"),
+                testRelative("libfoo-hjar.jar"),
+                testRelative("libfoo-src.jar")));
+
+    // intellij-info groups
+    assertThat(getOutputGroupFiles(testFixture, "intellij-info-java"))
+        .containsAtLeast(
+            testRelative("foo.java-manifest"), testRelative(intellijInfoFileName("foo")),
+            testRelative("direct.java-manifest"), testRelative(intellijInfoFileName("direct")),
+            testRelative("indirect.java-manifest"), testRelative(intellijInfoFileName("indirect")),
+            testRelative("distant.java-manifest"), testRelative(intellijInfoFileName("distant")));
+
+    assertThat(getOutputGroupFiles(testFixture, "intellij-info-java-outputs"))
+        .containsExactly(
+            testRelative("foo.java-manifest"), testRelative(intellijInfoFileName("foo")));
+
+    assertThat(getOutputGroupFiles(testFixture, "intellij-info-java-direct-deps"))
+        .containsAtLeast(
+            testRelative("foo.java-manifest"), testRelative(intellijInfoFileName("foo")),
+            testRelative("direct.java-manifest"), testRelative(intellijInfoFileName("direct")));
+    assertThat(getOutputGroupFiles(testFixture, "intellij-info-java-direct-deps"))
+        .containsNoneOf(
+            testRelative("indirect.java-manifest"), testRelative(intellijInfoFileName("indirect")),
+            testRelative("distant.java-manifest"), testRelative(intellijInfoFileName("distant")));
+
+    // intellij-resolve groups
+    assertThat(getOutputGroupFiles(testFixture, "intellij-resolve-java"))
+        .containsExactly(
+            // foo
+            testRelative("libfoo-hjar.jar"),
+            testRelative("libfoo-src.jar"),
+            testRelative("libfoo.jdeps"),
+            // direct
+            testRelative("libdirect-hjar.jar"),
+            testRelative("libdirect-src.jar"),
+            testRelative("libdirect.jdeps"),
+            // indirect
+            testRelative("libindirect-hjar.jar"),
+            testRelative("libindirect-src.jar"),
+            testRelative("libindirect.jdeps"),
+            // distant
+            testRelative("libdistant-hjar.jar"),
+            testRelative("libdistant-src.jar"),
+            testRelative("libdistant.jdeps"));
+    assertThat(getOutputGroupFiles(testFixture, "intellij-resolve-java-outputs"))
+        .containsExactly(
+            testRelative("libfoo-hjar.jar"),
+            testRelative("libfoo-src.jar"),
+            testRelative("libfoo.jdeps"));
+    assertThat(getOutputGroupFiles(testFixture, "intellij-resolve-java-direct-deps"))
+        .containsExactly(
+            // foo
+            testRelative("libfoo-hjar.jar"),
+            testRelative("libfoo-src.jar"),
+            testRelative("libfoo.jdeps"),
+            // direct
+            testRelative("libdirect-hjar.jar"),
+            testRelative("libdirect-src.jar"),
+            testRelative("libdirect.jdeps"),
+            // indirect (only hjar and src-jar)
+            testRelative("libindirect-hjar.jar"),
+            testRelative("libindirect-src.jar"),
+            // distant (only hjar and src-jar)
+            testRelative("libdistant-hjar.jar"),
+            testRelative("libdistant-src.jar"));
+
+    // intellij-compile groups
+    assertThat(getOutputGroupFiles(testFixture, "intellij-compile-java"))
+        .containsExactly(
+            testRelative("libfoo.jar"),
+            testRelative("libdirect.jar"),
+            testRelative("libindirect.jar"),
+            testRelative("libdistant.jar"));
+    assertThat(getOutputGroupFiles(testFixture, "intellij-compile-java-outputs"))
+        .containsExactly(testRelative("libfoo.jar"));
+    assertThat(getOutputGroupFiles(testFixture, "intellij-compile-java-direct-deps"))
+        .containsExactly(testRelative("libfoo.jar"), testRelative("libdirect.jar"));
+
+    assertThat(getOutputGroupFiles(testFixture, "intellij-info-generic")).isEmpty();
+
+    assertThat(target.getJavaIdeInfo().getJdeps().getRelativePath())
+        .isEqualTo(testRelative("libfoo.jdeps"));
+
+    assertThat(target.getJavaIdeInfo().getMainClass()).isEmpty();
+  }
 }

aspect/testing/tests/src/com/google/idea/blaze/aspect/proto/jpl/BUILD

@@ -29,6 +29,13 @@ java_library(
 
 intellij_aspect_test_fixture(
     name = "lib_fixture",
+    optimize_building_jars = "disabled",
+    deps = [":lib"],
+)
+
+intellij_aspect_test_fixture(
+    name = "lib_fixture_optimized_jars",
+    optimize_building_jars = "enabled",
     deps = [":lib"],
 )
 
@@ -37,6 +44,7 @@ java_test(
     srcs = ["JavaProtoLibraryTest.java"],
     data = [
         ":lib_fixture",
+        ":lib_fixture_optimized_jars",
     ],
     deps = [
         "//aspect/testing:BazelIntellijAspectTest",