Allow attributes to indicate whether they are for tools or not.
This allows common attributes like `target_compatible_with` to indicate that they do not create real dependencies on the targets pointed to.
PiperOrigin-RevId: 375559104
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
index 612867c..0b7de92 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
@@ -387,6 +387,9 @@
attr(RuleClass.TARGET_COMPATIBLE_WITH_ATTR, LABEL_LIST)
.mandatoryProviders(ConstraintValueInfo.PROVIDER.id())
// This should be configurable to allow for complex types of restrictions.
+ .tool(
+ "target_compatible_with exists for constraint checking, not to create an"
+ + " actual dependency")
.allowedFileTypes(FileTypeSet.NO_FILE))
.build();
}
@@ -447,6 +450,9 @@
attr(RuleClass.EXEC_COMPATIBLE_WITH_ATTR, BuildType.LABEL_LIST)
.allowedFileTypes()
.nonconfigurable("Used in toolchain resolution")
+ .tool(
+ "exec_compatible_with exists for constraint checking, not to create an"
+ + " actual dependency")
.value(ImmutableList.of()))
.build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index 1c2f59f..72041e1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -348,7 +348,7 @@
// not fail the overall build, since those dependencies should have their own builds
// and tests that should surface any failing validations.
Attribute attribute = ruleContext.attributes().getAttributeDefinition(attributeName);
- if (!attribute.getTransitionFactory().isTool()
+ if (!attribute.isToolDependency()
&& !attribute.isImplicit()
&& attribute.getType().getLabelClass() == LabelClass.DEPENDENCY) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
index ff38aa8..30086f7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
@@ -759,7 +759,7 @@
// Use the same implicit deps check that query uses. This facilitates running queries to
// determine exactly which rules need to be constraint-annotated for depot migrations.
if (!DependencyFilter.NO_IMPLICIT_DEPS.apply(ruleContext.getRule(), attrDef)
- || attrDef.getTransitionFactory().isTool()) {
+ || attrDef.isToolDependency()) {
continue;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
index 8632c63..1e1e2ff 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
@@ -148,11 +148,17 @@
attr(RuleClass.EXEC_COMPATIBLE_WITH_ATTR, BuildType.LABEL_LIST)
.allowedFileTypes()
.nonconfigurable("Used in toolchain resolution")
+ .tool(
+ "exec_compatible_with exists for constraint checking, not to create an"
+ + " actual dependency")
.value(ImmutableList.of()))
.add(
attr(RuleClass.TARGET_COMPATIBLE_WITH_ATTR, LABEL_LIST)
.mandatoryProviders(ConstraintValueInfo.PROVIDER.id())
// This should be configurable to allow for complex types of restrictions.
+ .tool(
+ "target_compatible_with exists for constraint checking, not to create an"
+ + " actual dependency")
.allowedFileTypes(FileTypeSet.NO_FILE))
.build();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java b/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java
index 96afb5b..a8407b2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFilesCollector.java
@@ -459,7 +459,7 @@
List<TransitiveInfoCollection> prerequisites = new ArrayList<>();
for (Attribute attr : ruleContext.getRule().getAttributes()) {
if ((attr.getType() == BuildType.LABEL_LIST || attr.getType() == BuildType.LABEL)
- && !attr.getTransitionFactory().isTool()) {
+ && !attr.isToolDependency()) {
prerequisites.addAll(ruleContext.getPrerequisites(attr.getName()));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index 5c34600..d7f4a0b 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -246,6 +246,12 @@
* analysis tests.
*/
HAS_ANALYSIS_TEST_TRANSITION,
+
+ /**
+ * Signals that a dependency attribute is used as a tool (regardless of the actual configuration
+ * or transition). Cannot be used for non-dependency attributes.
+ */
+ IS_TOOL_DEPENDENCY,
}
// TODO(bazel-team): modify this interface to extend Predicate and have an extra error
@@ -1069,6 +1075,13 @@
return setPropertyFlag(PropertyFlag.NONCONFIGURABLE, "nonconfigurable");
}
+ public Builder<TYPE> tool(String reason) {
+ Preconditions.checkState(
+ type.getLabelClass() == LabelClass.DEPENDENCY, "must be a label-valued type");
+ Preconditions.checkState(!reason.isEmpty());
+ return setPropertyFlag(PropertyFlag.IS_TOOL_DEPENDENCY, "is_tool_dependency");
+ }
+
/** Returns an {@link ImmutableAttributeFactory} that can be invoked to create attributes. */
public ImmutableAttributeFactory buildPartial() {
Preconditions.checkState(
@@ -2083,6 +2096,22 @@
}
/**
+ * Returns true if this attribute is used as a tool dependency, either because the attribute
+ * declares it directly (with {@link Attribute$Builder.tool}), or because the value's {@link
+ * TransitionFactory} declares it.
+ *
+ * <p>Non-dependency attributes will always return {@code false}.
+ */
+ public boolean isToolDependency() {
+ if (getType().getLabelClass() != LabelClass.DEPENDENCY) {
+ return false;
+ }
+ if (getPropertyFlag(PropertyFlag.IS_TOOL_DEPENDENCY)) {
+ return true;
+ }
+ return getTransitionFactory().isTool();
+ }
+ /**
* Returns a predicate that evaluates to true for rule classes that are allowed labels in this
* attribute. If this is not a label or label-list attribute, the returned predicate always
* evaluates to true.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/DependencyFilter.java b/src/main/java/com/google/devtools/build/lib/packages/DependencyFilter.java
index aaa4306..754847f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/DependencyFilter.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/DependencyFilter.java
@@ -38,12 +38,7 @@
new DependencyFilter() {
@Override
public boolean apply(AttributeInfoProvider infoProvider, Attribute attribute) {
- // getConfigurationTransition() is only defined for labels which introduce a dependency.
- if (attribute.getType().getLabelClass() != LabelClass.DEPENDENCY) {
- return true;
- }
-
- return !attribute.getTransitionFactory().isTool();
+ return !attribute.isToolDependency();
}
};
/** Dependency predicate that excludes implicit dependencies */
diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh
index 4058417..1d9feef 100755
--- a/src/test/shell/integration/target_compatible_with_test.sh
+++ b/src/test/shell/integration/target_compatible_with_test.sh
@@ -928,6 +928,20 @@
expect_log '^//target_skipping:genrule_foo1$'
}
+# Regression test for http://b/189071321: --notool_deps should exclude constraints.
+function test_query_no_tools() {
+ write_query_test_targets
+ cd target_skipping || fail "couldn't cd into workspace"
+
+ bazel query \
+ --notool_deps \
+ 'deps(//target_skipping:sh_foo1)' &> "${TEST_log}" \
+ || fail "Bazel query failed unexpectedly."
+ expect_log '^//target_skipping:sh_foo1$'
+ expect_log '^//target_skipping:genrule_foo1$'
+ expect_not_log '^//target_skipping:foo1$'
+}
+
# Run a cquery on a target that is compatible. This should pass.
function test_cquery_compatible_target() {
write_query_test_targets
