From 09cf76e753e9f95070ee862dd19aa7bf579dff8c Mon Sep 17 00:00:00 2001 From: twerth Date: Mon, 24 Sep 2018 00:52:03 -0700 Subject: [PATCH] Make the dependency from genrule to host JDK conditional. We already did only add the toolchain to the input of the action if neccessary but the dependency was always there. Reduces the number of transitive dependencies of a simple genrule from 350 to 19. RELNOTES: None PiperOrigin-RevId: 214224406 --- .../bazel/rules/genrule/BazelGenRuleRule.java | 3 +- .../devtools/build/lib/rules/genrule/BUILD | 1 + .../build/lib/rules/genrule/GenRuleBase.java | 2 +- .../lib/rules/genrule/GenRuleBaseRule.java | 25 ++++++++++ .../genrule/GenRuleConfiguredTargetTest.java | 35 +------------- src/test/shell/integration/bazel_java_test.sh | 47 +++++++++++++++++++ .../integration/discard_graph_edges_test.sh | 2 +- 7 files changed, 77 insertions(+), 38 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java index d23071bf9baa5d..afca3e9e0ba058 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java @@ -27,7 +27,6 @@ import com.google.devtools.build.lib.rules.genrule.GenRuleBaseRule; import com.google.devtools.build.lib.rules.java.JavaConfiguration; import com.google.devtools.build.lib.rules.java.JavaRuntimeInfo; -import com.google.devtools.build.lib.rules.java.JavaSemantics; /** * Rule definition for genrule for Bazel. @@ -62,7 +61,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .add( attr(":host_jdk", LABEL) .cfg(HostTransition.INSTANCE) - .value(JavaSemantics.hostJdkAttribute(env)) + .value(GenRuleBaseRule.maybeHostJdk(env)) .mandatoryProviders(JavaRuntimeInfo.PROVIDER.id())) .build(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/BUILD b/src/main/java/com/google/devtools/build/lib/rules/genrule/BUILD index bfce8e108e2e6c..524f3a2fc671b8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/BUILD @@ -15,6 +15,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib:build-base", "//src/main/java/com/google/devtools/build/lib:events", "//src/main/java/com/google/devtools/build/lib:java-compilation", + "//src/main/java/com/google/devtools/build/lib:java-implicit-attributes", "//src/main/java/com/google/devtools/build/lib:packages", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/cmdline", diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java index aa360ffeb01de9..44b1ebb97440ed 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java @@ -77,7 +77,7 @@ protected static boolean requiresCrosstool(String command) { return CROSSTOOL_MAKE_VARIABLE_PATTERN.matcher(command).find(); } - protected boolean requiresJdk(String command) { + protected static boolean requiresJdk(String command) { return JDK_MAKE_VARIABLE.matcher(command).find(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java index 0b4dcb925d7aa8..861742250ec4bb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; import com.google.devtools.build.lib.analysis.config.HostTransition; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Attribute.ComputedDefault; import com.google.devtools.build.lib.packages.Attribute.LabelLateBoundDefault; @@ -34,8 +35,11 @@ import com.google.devtools.build.lib.packages.RuleClass.ExecutionPlatformConstraintsAllowed; import com.google.devtools.build.lib.rules.cpp.CppConfiguration; import com.google.devtools.build.lib.rules.cpp.CppRuleClasses; +import com.google.devtools.build.lib.rules.java.JavaConfiguration; +import com.google.devtools.build.lib.rules.java.JavaImplicitAttributes; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.FileTypeSet; +import java.io.Serializable; /** * Rule definition for the genrule rule, intended to be inherited by specific GenRule @@ -72,6 +76,27 @@ public Object getDefault(AttributeMap rule) { }; } + /** + * Late-bound dependency on the host JDK iff the genrule has make variables that need the + * host JDK. + */ + public static LabelLateBoundDefault maybeHostJdk(RuleDefinitionEnvironment env) { + return LabelLateBoundDefault.fromHostConfiguration( + JavaConfiguration.class, + env.getToolsLabel(JavaImplicitAttributes.HOST_JDK_LABEL), + (Attribute.LateBoundDefault.Resolver & Serializable) + // null guards are needed for LateBoundAttributeTest + (rule, attributes, configuration) -> { + if (attributes != null) { + String cmd = attributes.get("cmd", Type.STRING); + if (cmd != null && GenRuleBase.requiresJdk(cmd)) { + return configuration.getRuntimeLabel(); + } + } + return null; + }); + } + @Override public RuleClass build( RuleClass.Builder builder, RuleDefinitionEnvironment env) { diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java index e0c7bc2c78f5cd..fa81d5f1c64315 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java @@ -20,7 +20,6 @@ import static com.google.devtools.build.lib.testutil.TestConstants.GENRULE_SETUP_PATH; import static org.junit.Assert.fail; -import com.google.common.base.Predicate; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.Artifact; @@ -30,9 +29,7 @@ import com.google.devtools.build.lib.analysis.ShellConfiguration; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.configuredtargets.FileConfiguredTarget; -import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; -import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider; import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool; import com.google.devtools.build.lib.rules.cpp.CppHelper; @@ -49,26 +46,6 @@ @RunWith(JUnit4.class) public class GenRuleConfiguredTargetTest extends BuildViewTestCase { - /** Filter to remove implicit dependencies of C/C++ rules. */ - private static final Predicate CC_CONFIGURED_TARGET_FILTER = - new Predicate() { - @Override - public boolean apply(ConfiguredTarget target) { - return AnalysisMock.get().ccSupport().labelFilter().apply(target.getLabel()); - } - }; - - /** Filter to remove implicit dependencies of Java rules. */ - private static final Predicate JAVA_CONFIGURED_TARGET_FILTER = - new Predicate() { - @Override - public boolean apply(ConfiguredTarget target) { - Label label = target.getLabel(); - String labelName = "//" + label.getPackageName(); - return !labelName.startsWith("//third_party/java/jdk"); - } - }; - private static final Pattern SETUP_COMMAND_PATTERN = Pattern.compile(".*/genrule-setup.sh;\\s+(?.*)"); @@ -466,23 +443,13 @@ public void testToolsAreHostConfiguration() throws Exception { ConfiguredTarget parentTarget = getConfiguredTarget("//config"); - Iterable prereqs = - Iterables.filter( - Iterables.filter( - getDirectPrerequisites(parentTarget), - CC_CONFIGURED_TARGET_FILTER), - JAVA_CONFIGURED_TARGET_FILTER); + Iterable prereqs = getDirectPrerequisites(parentTarget); boolean foundSrc = false; boolean foundTool = false; boolean foundSetup = false; for (ConfiguredTarget prereq : prereqs) { String name = prereq.getLabel().getName(); - if (name.contains("cc-") || name.contains("jdk")) { - // Ignore these, they are present due to the implied genrule dependency on crosstool and - // JDK. - continue; - } switch (name) { case "src": assertConfigurationsEqual(getConfiguration(parentTarget), getConfiguration(prereq)); diff --git a/src/test/shell/integration/bazel_java_test.sh b/src/test/shell/integration/bazel_java_test.sh index 5666836f0a3bc9..3fbc448bbd25b9 100755 --- a/src/test/shell/integration/bazel_java_test.sh +++ b/src/test/shell/integration/bazel_java_test.sh @@ -174,4 +174,51 @@ function test_no_javabase_default_embedded() { expect_log "Zulu" } +function test_genrule() { + mkdir -p foo/bin bar/bin + cat << EOF > BUILD +java_runtime( + name = "foo_javabase", + java_home = "$PWD/foo", + visibility = ["//visibility:public"], +) + +java_runtime( + name = "bar_runtime", + visibility = ["//visibility:public"], + srcs = ["bar/bin/java"], +) + +genrule( + name = "without_java", + srcs = ["in"], + outs = ["out_without"], + cmd = "cat \$(SRCS) > \$(OUTS)", +) + +genrule( + name = "with_java", + srcs = ["in"], + outs = ["out_with"], + cmd = "echo \$(JAVA) > \$(OUTS)", + toolchains = [":bar_runtime"], +) +EOF + + bazel cquery --implicit_deps 'deps(//:without_java)' >& $TEST_log + expect_not_log "foo" + expect_not_log "bar" + expect_not_log "embedded_jdk" + + bazel cquery --implicit_deps 'deps(//:with_java)' >& $TEST_log + expect_not_log "foo" + expect_log "bar" + expect_log "embedded_jdk" + + bazel cquery --implicit_deps 'deps(//:with_java)' --host_javabase=:foo_javabase >& $TEST_log + expect_log "foo" + expect_log "bar" + expect_not_log "embedded_jdk" +} + run_suite "Tests of specifying custom server_javabase/host_javabase and javabase." diff --git a/src/test/shell/integration/discard_graph_edges_test.sh b/src/test/shell/integration/discard_graph_edges_test.sh index 6ce83ab5500450..a8168124dd813a 100755 --- a/src/test/shell/integration/discard_graph_edges_test.sh +++ b/src/test/shell/integration/discard_graph_edges_test.sh @@ -209,7 +209,7 @@ function test_packages_cleared() { [[ "$package_count" -ge 9 ]] \ || fail "package count $package_count too low: did you move/rename the class?" local glob_count="$(extract_histogram_count "$histo_file" "GlobValue$")" - [[ "$glob_count" -ge 8 ]] \ + [[ "$glob_count" -ge 2 ]] \ || fail "glob count $glob_count too low: did you move/rename the class?" local env_count="$(extract_histogram_count "$histo_file" \ 'Environment\$Extension$')"