Skip to content

Commit

Permalink
Make the dependency from genrule to host JDK conditional.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
meisterT authored and Copybara-Service committed Sep 24, 2018
1 parent c361da0 commit 09cf76e
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -72,6 +76,27 @@ public Object getDefault(AttributeMap rule) {
};
}

/**
* Late-bound dependency on the host JDK <i>iff</i> 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<JavaConfiguration, Label> & 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<ConfiguredTarget> CC_CONFIGURED_TARGET_FILTER =
new Predicate<ConfiguredTarget>() {
@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<ConfiguredTarget> JAVA_CONFIGURED_TARGET_FILTER =
new Predicate<ConfiguredTarget>() {
@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+(?<command>.*)");

Expand Down Expand Up @@ -466,23 +443,13 @@ public void testToolsAreHostConfiguration() throws Exception {

ConfiguredTarget parentTarget = getConfiguredTarget("//config");

Iterable<ConfiguredTarget> prereqs =
Iterables.filter(
Iterables.filter(
getDirectPrerequisites(parentTarget),
CC_CONFIGURED_TARGET_FILTER),
JAVA_CONFIGURED_TARGET_FILTER);
Iterable<ConfiguredTarget> 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));
Expand Down
47 changes: 47 additions & 0 deletions src/test/shell/integration/bazel_java_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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."
2 changes: 1 addition & 1 deletion src/test/shell/integration/discard_graph_edges_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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$')"
Expand Down

0 comments on commit 09cf76e

Please sign in to comment.