From f7d5d5375eec93bca6732b62256e7cdd3c27c19e Mon Sep 17 00:00:00 2001 From: ilist Date: Thu, 18 Feb 2021 23:43:19 -0800 Subject: [PATCH] BEGIN_PUBLIC Add _direct_source_jars output group to rules that have _source_jars output group. Output group _source_jars provides all transitive source jars, whereas there is a need to provide only the direct source jar. We're trying to eliminate implicit outputs from Bazel. Implicit output `libNAME-src.jar` provides direct source jar. There are targets that collect direct sources and pack them together into an archive. Currently those targets use an implicit output. A nice substitution for that, that doesn't use implicit output is a `filegroup` target with `output_group` attribute set to `_direct_source_jars`. We're concerned cleaning up Java rules, but for the sake of consistency and because one cleanup is easier, output group is added also to other Java related rules. RELNOTES: Added _direct_source_jars output group to Java related targets. END_PUBLIC PiperOrigin-RevId: 358346735 --- .../lib/rules/android/AndroidCommon.java | 5 +++- .../rules/android/AndroidLocalTestBase.java | 3 ++ .../build/lib/rules/java/JavaBinary.java | 3 ++ .../build/lib/rules/java/JavaImport.java | 4 +++ .../build/lib/rules/java/JavaLibrary.java | 4 +++ .../build/lib/rules/java/JavaSemantics.java | 6 +++- .../lib/rules/android/AndroidBinaryTest.java | 8 +++++ .../lib/rules/android/AndroidLibraryTest.java | 8 +++++ .../FilegroupConfiguredTargetTest.java | 29 +++++++++++++++---- 9 files changed, 63 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java index 594885545555a8..b6fbd3444de813 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java @@ -723,7 +723,10 @@ public RuleConfiguredTargetBuilder addTransitiveInfoProviders( .addOutputGroup( OutputGroupInfo.HIDDEN_TOP_LEVEL, collectHiddenTopLevelArtifacts(ruleContext)) .addOutputGroup( - JavaSemantics.SOURCE_JARS_OUTPUT_GROUP, sourceJarsProvider.getTransitiveSourceJars()); + JavaSemantics.SOURCE_JARS_OUTPUT_GROUP, sourceJarsProvider.getTransitiveSourceJars()) + .addOutputGroup( + JavaSemantics.DIRECT_SOURCE_JARS_OUTPUT_GROUP, + NestedSetBuilder.wrap(Order.STABLE_ORDER, sourceJarsProvider.getSourceJars())); } private Runfiles getRunfiles() { diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java index 527586fead5b0a..a78f51eee034cf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java @@ -405,6 +405,9 @@ public ConfiguredTarget create(RuleContext ruleContext) new JavaRuntimeClasspathProvider(javaCommon.getRuntimeClasspath())) .addProvider(JavaPrimaryClassProvider.class, new JavaPrimaryClassProvider(testClass)) .addOutputGroup(JavaSemantics.SOURCE_JARS_OUTPUT_GROUP, transitiveSourceJars) + .addOutputGroup( + JavaSemantics.DIRECT_SOURCE_JARS_OUTPUT_GROUP, + NestedSetBuilder.wrap(Order.STABLE_ORDER, sourceJarsProvider.getSourceJars())) .build(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java index 5b58606f6c2b9d..50388a76b41edd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java @@ -512,6 +512,9 @@ public ConfiguredTarget create(RuleContext ruleContext) JavaRuntimeClasspathProvider.class, new JavaRuntimeClasspathProvider(common.getRuntimeClasspath())) .addOutputGroup(JavaSemantics.SOURCE_JARS_OUTPUT_GROUP, transitiveSourceJars) + .addOutputGroup( + JavaSemantics.DIRECT_SOURCE_JARS_OUTPUT_GROUP, + NestedSetBuilder.wrap(Order.STABLE_ORDER, sourceJarsProvider.getSourceJars())) .build(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java index 07dc050c87fd18..c243b7ff7e2deb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.rules.cpp.LibraryToLink; import java.util.LinkedHashSet; import java.util.Set; @@ -151,6 +152,9 @@ public ConfiguredTarget create(RuleContext ruleContext) .addNativeDeclaredProvider(new JavaNativeLibraryInfo(transitiveJavaNativeLibraries)) .addNativeDeclaredProvider(new ProguardSpecProvider(proguardSpecs)) .addOutputGroup(JavaSemantics.SOURCE_JARS_OUTPUT_GROUP, transitiveJavaSourceJars) + .addOutputGroup( + JavaSemantics.DIRECT_SOURCE_JARS_OUTPUT_GROUP, + NestedSetBuilder.wrap(Order.STABLE_ORDER, sourceJarsProvider.getSourceJars())) .addOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL, proguardSpecs) .build(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibrary.java index 04b0a191deef2d..24a9f2bf5d2225 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibrary.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.analysis.RunfilesProvider; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.rules.cpp.LibraryToLink; import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider.ClasspathType; import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider; @@ -205,6 +206,9 @@ final ConfiguredTarget init( .addNativeDeclaredProvider(new ProguardSpecProvider(proguardSpecs)) .addNativeDeclaredProvider(javaInfo) .addOutputGroup(JavaSemantics.SOURCE_JARS_OUTPUT_GROUP, transitiveSourceJars) + .addOutputGroup( + JavaSemantics.DIRECT_SOURCE_JARS_OUTPUT_GROUP, + NestedSetBuilder.wrap(Order.STABLE_ORDER, sourceJarsProvider.getSourceJars())) .addOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL, proguardSpecs); Artifact validation = diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java index fe90877c40ec2f..11d6736381ec63 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java @@ -106,9 +106,13 @@ static Label javaToolchainAttribute(RuleDefinitionEnvironment environment) { return environment.getToolsLabel("//tools/jdk:current_java_toolchain"); } - /** Name of the output group used for source jars. */ + /** Name of the output group used for transitive source jars. */ String SOURCE_JARS_OUTPUT_GROUP = OutputGroupInfo.HIDDEN_OUTPUT_GROUP_PREFIX + "source_jars"; + /** Name of the output group used for direct source jars. */ + String DIRECT_SOURCE_JARS_OUTPUT_GROUP = + OutputGroupInfo.HIDDEN_OUTPUT_GROUP_PREFIX + "direct_source_jars"; + /** Implementation for the :jvm attribute. */ static Label jvmAttribute(RuleDefinitionEnvironment env) { return env.getToolsLabel("//tools/jdk:current_java_runtime"); diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java index 4d387149b5355d..38832f8aa8ef02 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java @@ -4482,12 +4482,20 @@ public void starlarkJavaInfoToAndroidBinaryAttributes() throws Exception { // TODO(b/75051107): Remove the following line when fixed. " incremental_dexing = 0,", ")"); + // Test that all bottom jars are on the runtime classpath of the app. ConfiguredTarget target = getConfiguredTarget("//java/r/android:foo_app"); + ImmutableList transitiveSrcJars = OutputGroupInfo.get(target).getOutputGroup(JavaSemantics.SOURCE_JARS_OUTPUT_GROUP).toList(); assertThat(ActionsTestUtil.baseArtifactNames(transitiveSrcJars)) .containsExactly("libal_bottom_for_deps-src.jar", "libfoo_app-src.jar"); + ImmutableList directSrcJar = + OutputGroupInfo.get(target) + .getOutputGroup(JavaSemantics.DIRECT_SOURCE_JARS_OUTPUT_GROUP) + .toList(); + assertThat(ActionsTestUtil.baseArtifactNames(directSrcJar)) + .containsExactly("libfoo_app-src.jar"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java index 48c9060012e802..aa098cc030967d 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java @@ -2530,8 +2530,10 @@ public void starlarkJavaInfoToAndroidLibraryAttributes() throws Exception { " deps = [':mya'],", " exports = [':myb'],", ")"); + // Test that all bottom jars are on the runtime classpath of lib_android. ConfiguredTarget target = getConfiguredTarget("//foo:lib_foo"); + ImmutableList transitiveSrcJars = OutputGroupInfo.get(target).getOutputGroup(JavaSemantics.SOURCE_JARS_OUTPUT_GROUP).toList(); assertThat(ActionsTestUtil.baseArtifactNames(transitiveSrcJars)) @@ -2539,6 +2541,12 @@ public void starlarkJavaInfoToAndroidLibraryAttributes() throws Exception { "libjl_bottom_for_exports-src.jar", "libal_bottom_for_deps-src.jar", "liblib_foo-src.jar"); + ImmutableList directSrcJars = + OutputGroupInfo.get(target) + .getOutputGroup(JavaSemantics.DIRECT_SOURCE_JARS_OUTPUT_GROUP) + .toList(); + assertThat(ActionsTestUtil.baseArtifactNames(directSrcJars)) + .containsExactly("liblib_foo-src.jar"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/filegroup/FilegroupConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/filegroup/FilegroupConfiguredTargetTest.java index 2595c517ce5dfc..caa48f84248333 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/filegroup/FilegroupConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/filegroup/FilegroupConfiguredTargetTest.java @@ -151,20 +151,39 @@ public void testGlobMatchesRuleOutputsInsteadOfFileWithTheSameName() throws Exce } @Test - public void testOutputGroupExtractsCorrectArtifacts() throws Exception { + public void outputGroupSourceJars_extractsTransitiveSources() throws Exception { scratch.file("pkg/a.java"); scratch.file("pkg/b.java"); - scratch.file("pkg/in_ouput_group_a"); - scratch.file("pkg/in_ouput_group_b"); - + scratch.file("pkg/c.java"); scratch.file( "pkg/BUILD", "java_library(name='lib_a', srcs=['a.java'])", - "java_library(name='lib_b', srcs=['b.java'])", + "java_library(name='lib_b', srcs=['b.java'], deps = [':lib_c'])", + "java_library(name='lib_c', srcs=['c.java'])", "filegroup(name='group', srcs=[':lib_a', ':lib_b']," + String.format("output_group='%s')", JavaSemantics.SOURCE_JARS_OUTPUT_GROUP)); ConfiguredTarget group = getConfiguredTarget("//pkg:group"); + + assertThat(ActionsTestUtil.prettyArtifactNames(getFilesToBuild(group))) + .containsExactly("pkg/liblib_a-src.jar", "pkg/liblib_b-src.jar", "pkg/liblib_c-src.jar"); + } + + @Test + public void outputGroupDirectSourceJars_extractsDirectSources() throws Exception { + scratch.file("pkg/a.java"); + scratch.file("pkg/b.java"); + scratch.file("pkg/c.java"); + scratch.file( + "pkg/BUILD", + "java_library(name='lib_a', srcs=['a.java'])", + "java_library(name='lib_b', srcs=['b.java'], deps = [':lib_c'])", + "java_library(name='lib_c', srcs=['c.java'])", + "filegroup(name='group', srcs=[':lib_a', ':lib_b']," + + String.format("output_group='%s')", JavaSemantics.DIRECT_SOURCE_JARS_OUTPUT_GROUP)); + + ConfiguredTarget group = getConfiguredTarget("//pkg:group"); + assertThat(ActionsTestUtil.prettyArtifactNames(getFilesToBuild(group))) .containsExactly("pkg/liblib_a-src.jar", "pkg/liblib_b-src.jar"); }