Add proto-output-format flag for adding $internal_attr_hash
$internal_attr_hash is a magic attribute which can be used to tell if a rule
has changed in a potentially meaningful way across queries. The name itself
is a slight misnomer (non-attribute properties can contribute to the hash),
but it is what it is for historical reasons.
Note that the postProcess call removed from ConfiguredProtoOutputFormatter was
always a noop.
RELNOTES: None
PiperOrigin-RevId: 300146650
diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java b/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java
index 878461c..84fcbd3 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java
@@ -172,6 +172,14 @@
help = "Whether or not to populate the rule_input and rule_output fields.")
public boolean protoIncludeRuleInputsAndOutputs;
+ @Option(
+ name = "proto:include_synthetic_attribute_hash",
+ defaultValue = "false",
+ documentationCategory = OptionDocumentationCategory.QUERY,
+ effectTags = {OptionEffectTag.TERMINAL_OUTPUT},
+ help = "Whether or not to calculate and populate the $internal_attr_hash attribute.")
+ public boolean protoIncludeSyntheticAttributeHash;
+
/** An enum converter for {@code AspectResolver.Mode} . Should be used internally only. */
public static class AspectResolutionModeConverter extends EnumConverter<Mode> {
public AspectResolutionModeConverter() {
diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallback.java
index cc79c8b..fd7e54f 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallback.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallback.java
@@ -159,8 +159,7 @@
private class ConfiguredProtoOutputFormatter extends ProtoOutputFormatter {
@Override
protected void addAttributes(
- Build.Rule.Builder rulePb, Rule rule, Object extraDataForPostProcess)
- throws InterruptedException {
+ Build.Rule.Builder rulePb, Rule rule, Object extraDataForAttrHash) {
// We know <code>currentTarget</code> will be one of these two types of configured targets
// because this method is only triggered in ProtoOutputFormatter.toTargetProtoBuffer when
// the target in currentTarget is an instanceof Rule.
@@ -190,7 +189,6 @@
serializedAttributes.put(attr, serializedAttribute);
}
rulePb.addAllAttribute(serializedAttributes.values());
- postProcess(rule, rulePb, serializedAttributes, extraDataForPostProcess);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/BUILD b/src/main/java/com/google/devtools/build/lib/query2/query/output/BUILD
index 711dd31..bab3465 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/BUILD
@@ -24,9 +24,11 @@
"//src/main/java/com/google/devtools/build/lib/query2/compat:fake-load-target",
"//src/main/java/com/google/devtools/build/lib/query2/engine",
"//src/main/java/com/google/devtools/build/lib/query2/query/aspectresolvers",
+ "//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/common/options",
"//src/main/protobuf:build_java_proto",
+ "//third_party:flogger",
"//third_party:guava",
"//third_party:jsr305",
"//third_party/protobuf:protobuf_java",
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
index 23db2a6..0a78721 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
@@ -51,6 +51,7 @@
import com.google.devtools.build.lib.query2.engine.SynchronizedDelegatingOutputFormatterCallback;
import com.google.devtools.build.lib.query2.engine.ThreadSafeOutputFormatterCallback;
import com.google.devtools.build.lib.query2.proto.proto2api.Build;
+import com.google.devtools.build.lib.query2.proto.proto2api.Build.Attribute.Discriminator;
import com.google.devtools.build.lib.query2.proto.proto2api.Build.GeneratedFile;
import com.google.devtools.build.lib.query2.proto.proto2api.Build.QueryResult;
import com.google.devtools.build.lib.query2.proto.proto2api.Build.SourceFile;
@@ -97,6 +98,7 @@
private boolean flattenSelects = true;
private boolean includeLocations = true;
private boolean includeRuleInputsAndOutputs = true;
+ private boolean includeSyntheticAttributeHash = false;
@Override
public String getName() {
@@ -114,6 +116,7 @@
this.flattenSelects = options.protoFlattenSelects;
this.includeLocations = options.protoIncludeLocations;
this.includeRuleInputsAndOutputs = options.protoIncludeRuleInputsAndOutputs;
+ this.includeSyntheticAttributeHash = options.protoIncludeSyntheticAttributeHash;
}
private static Predicate<String> newAttributePredicate(List<String> outputAttributes) {
@@ -157,12 +160,12 @@
/** Converts a logical {@link Target} object into a {@link Build.Target} protobuffer. */
public Build.Target toTargetProtoBuffer(Target target) throws InterruptedException {
- return toTargetProtoBuffer(target, null);
+ return toTargetProtoBuffer(target, /*extraDataForAttrHash=*/ "");
}
/** Converts a logical {@link Target} object into a {@link Build.Target} protobuffer. */
@VisibleForTesting
- public Build.Target toTargetProtoBuffer(Target target, Object extraDataForPostProcess)
+ public Build.Target toTargetProtoBuffer(Target target, Object extraDataForAttrHash)
throws InterruptedException {
Build.Target.Builder targetPb = Build.Target.newBuilder();
@@ -174,7 +177,7 @@
if (includeLocations) {
rulePb.setLocation(FormatUtils.getLocation(target, relativeLocations));
}
- addAttributes(rulePb, rule, extraDataForPostProcess);
+ addAttributes(rulePb, rule, extraDataForAttrHash);
String transitiveHashCode = rule.getRuleClassObject().getRuleDefinitionEnvironmentHashCode();
if (transitiveHashCode != null && includeRuleDefinitionEnvironment()) {
// The RuleDefinitionEnvironment is always defined for Skylark rules and
@@ -329,7 +332,7 @@
return targetPb.build();
}
- protected void addAttributes(Build.Rule.Builder rulePb, Rule rule, Object extraDataForPostProcess)
+ protected void addAttributes(Build.Rule.Builder rulePb, Rule rule, Object extraDataForAttrHash)
throws InterruptedException {
Map<Attribute, Build.Attribute> serializedAttributes = Maps.newHashMap();
AggregatingAttributeMapper attributeMapper = AggregatingAttributeMapper.of(rule);
@@ -361,7 +364,15 @@
.sorted(ATTRIBUTE_NAME)
.collect(Collectors.toList()));
- postProcess(rule, rulePb, serializedAttributes, extraDataForPostProcess);
+ if (includeSyntheticAttributeHash()) {
+ rulePb.addAttribute(
+ Build.Attribute.newBuilder()
+ .setName("$internal_attr_hash")
+ .setStringValue(
+ SyntheticAttributeHashCalculator.compute(
+ rule, serializedAttributes, extraDataForAttrHash))
+ .setType(Discriminator.STRING));
+ }
}
protected boolean shouldIncludeAttribute(Rule rule, Attribute attr) {
@@ -385,13 +396,6 @@
}
}
- /** Further customize the proto output */
- protected void postProcess(
- Rule rule,
- Build.Rule.Builder rulePb,
- Map<Attribute, Build.Attribute> serializedAttributes,
- Object extraDataForPostProcess) {}
-
/** Allow filtering of aspect attributes. */
protected boolean includeAspectAttribute(Attribute attr, Collection<Label> value) {
return true;
@@ -401,6 +405,11 @@
return true;
}
+ /** Returns if the "$internal_attr_hash" should be computed and added to the result. */
+ protected boolean includeSyntheticAttributeHash() {
+ return includeSyntheticAttributeHash;
+ }
+
/**
* Coerces the list {@param possibleValues} of values of type {@param attrType} to a single
* value of that type, in the following way:
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculator.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculator.java
new file mode 100644
index 0000000..d0a7008
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculator.java
@@ -0,0 +1,158 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.query2.query.output;
+
+import com.google.common.base.Preconditions;
+import com.google.common.flogger.GoogleLogger;
+import com.google.common.hash.HashingOutputStream;
+import com.google.common.io.BaseEncoding;
+import com.google.common.io.ByteStreams;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
+import com.google.devtools.build.lib.packages.AttributeFormatter;
+import com.google.devtools.build.lib.packages.BuildType.SelectorList;
+import com.google.devtools.build.lib.packages.RawAttributeMapper;
+import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.query2.proto.proto2api.Build;
+import com.google.devtools.build.lib.vfs.DigestHashFunction;
+import com.google.protobuf.CodedOutputStream;
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Contains the logic for condensing the various properties of rules that contribute to their
+ * "affectedness" into a simple hash value. The resulting hash may be compared across queries to
+ * tell if a rule has changed in a potentially meaningful way.
+ */
+class SyntheticAttributeHashCalculator {
+
+ private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
+
+ private SyntheticAttributeHashCalculator() {}
+
+ /**
+ * Returns a hash of various properties of a rule which might contribute to the rule's
+ * "affectedness". This includes, but is not limited to, attribute values and error-state.
+ *
+ * @param rule The rule instance to calculate the hash for.
+ * @param serializedAttributes Any available attribute which have already been serialized. This is
+ * an optimization to avoid re-serializing attributes internally.
+ * @param extraDataForAttrHash Extra data to add to the hash.
+ */
+ static String compute(
+ Rule rule,
+ Map<Attribute, Build.Attribute> serializedAttributes,
+ Object extraDataForAttrHash) {
+ HashingOutputStream hashingOutputStream =
+ new HashingOutputStream(
+ DigestHashFunction.getDefaultUnchecked().getHashFunction(),
+ ByteStreams.nullOutputStream());
+ CodedOutputStream codedOut = CodedOutputStream.newInstance(hashingOutputStream);
+
+ RuleClass ruleClass = rule.getRuleClassObject();
+ if (ruleClass.isSkylark()) {
+ try {
+ codedOut.writeStringNoTag(
+ Preconditions.checkNotNull(ruleClass.getRuleDefinitionEnvironmentHashCode(), rule));
+ } catch (IOException e) {
+ throw new IllegalStateException("Unexpected IO failure writing to digest stream", e);
+ }
+ }
+
+ RawAttributeMapper rawAttributeMapper = RawAttributeMapper.of(rule);
+ for (Attribute attr : rule.getAttributes()) {
+ String attrName = attr.getName();
+
+ if (attrName.equals("generator_location")) {
+ // generator_location can be ignored for the purpose of telling if a rule has changed.
+ continue;
+ }
+
+ Object valueToHash = rawAttributeMapper.getRawAttributeValue(rule, attr);
+
+ if (valueToHash instanceof ComputedDefault) {
+ // ConfiguredDefaults need special handling to detect changes in evaluated values.
+ ComputedDefault computedDefault = (ComputedDefault) valueToHash;
+ if (!computedDefault.dependencies().isEmpty()) {
+ // TODO(b/29038463): We're skipping computed defaults that depend on other configurable
+ // attributes because there currently isn't a way to evaluate such a computed default;
+ // there isn't *one* value it evaluates to.
+ continue;
+ }
+
+ try {
+ valueToHash = computedDefault.getDefault(rawAttributeMapper);
+ } catch (IllegalArgumentException e) {
+ // TODO(mschaller): Catching IllegalArgumentException isn't ideal. It's thrown by
+ // AbstractAttributeMapper#get if the attribute's type doesn't match its value, which
+ // would happen if a ComputedDefault function accessed an attribute whose value was
+ // configurable. We check whether the ComputedDefault declared any configurable
+ // attribute dependencies above, but someone could make a mistake and fail to declare
+ // something. There's no mechanism that enforces correct declaration right now.
+ // This allows us to recover from such an error by skipping an attribute, as opposed to
+ // crashing.
+ logger.atWarning().log(
+ "Recovering from failed evaluation of ComputedDefault attribute value: " + e);
+ continue;
+ }
+ }
+
+ Build.Attribute attrPb;
+ if (valueToHash instanceof SelectorList<?> || !serializedAttributes.containsKey(attr)) {
+ // We didn't already serialize the attribute or it's a SelectorList. Latter may
+ // have been flattened while we want the full representation, so we start from scratch.
+ attrPb =
+ AttributeFormatter.getAttributeProto(
+ attr,
+ valueToHash,
+ /* explicitlySpecified= */ false, // We care about value, not how it was set.
+ /*encodeBooleanAndTriStateAsIntegerAndString=*/ false);
+ } else {
+ attrPb = serializedAttributes.get(attr);
+ }
+
+ try {
+ attrPb.writeTo(codedOut);
+ } catch (IOException e) {
+ throw new IllegalStateException("Unexpected IO failure writing to digest stream", e);
+ }
+ }
+
+ try {
+ // Rules can be considered changed when the containing package goes in/out of error.
+ codedOut.writeBoolNoTag(rule.getPackage().containsErrors());
+ } catch (IOException e) {
+ throw new IllegalStateException("Unexpected IO failure writing to digest stream", e);
+ }
+
+ try {
+ // Include a summary of any package-wide data that applies to this target (e.g. custom make
+ // variables aka `vardef`).
+ codedOut.writeStringNoTag((String) extraDataForAttrHash);
+ } catch (IOException e) {
+ throw new IllegalStateException("Unexpected IO failure writing to digest stream", e);
+ }
+
+ try {
+ // Flush coded out to make sure all bytes make it to the underlying digest stream.
+ codedOut.flush();
+ } catch (IOException e) {
+ throw new IllegalStateException("Unexpected flush failure", e);
+ }
+
+ return BaseEncoding.base64().encode(hashingOutputStream.hash().asBytes());
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculatorTest.java b/src/test/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculatorTest.java
new file mode 100644
index 0000000..94a49a9
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculatorTest.java
@@ -0,0 +1,161 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.query2.query.output;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
+import com.google.devtools.build.lib.query2.proto.proto2api.Build;
+import com.google.devtools.build.lib.query2.proto.proto2api.Build.Attribute.Discriminator;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.RootedPath;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link SyntheticAttributeHashCalculator}. */
+@RunWith(JUnit4.class)
+public class SyntheticAttributeHashCalculatorTest extends PackageLoadingTestCase {
+
+ @Test
+ public void testComputeAttributeChangeChangesHash() throws Exception {
+ Path buildFile = scratch.file("pkg/BUILD");
+
+ scratch.overwriteFile("pkg/BUILD", "genrule(name='x', cmd='touch $@', outs=['y'])");
+ Rule ruleBefore = getRule(buildFile, "x");
+
+ scratch.overwriteFile("pkg/BUILD", "genrule(name='x', cmd='touch $@', outs=['z'])");
+ Rule ruleAfter = getRule(buildFile, "x");
+
+ String hashBefore =
+ SyntheticAttributeHashCalculator.compute(
+ ruleBefore, /*serializedAttributes=*/ ImmutableMap.of(), /*extraDataForAttrHash=*/ "");
+ String hashAfter =
+ SyntheticAttributeHashCalculator.compute(
+ ruleAfter, /*serializedAttributes=*/ ImmutableMap.of(), /*extraDataForAttrHash=*/ "");
+
+ assertThat(hashBefore).isNotEqualTo(hashAfter);
+ }
+
+ @Test
+ public void testComputeLocationDoesntChangeHash() throws Exception {
+ Path buildFile = scratch.file("pkg/BUILD");
+
+ scratch.overwriteFile("pkg/BUILD", "genrule(name='x', cmd='touch $@', outs=['y'])");
+ Rule ruleBefore = getRule(buildFile, "x");
+
+ scratch.overwriteFile(
+ "pkg/BUILD",
+ "genrule(name='rule_that_moves_x', cmd='touch $@', outs=['whatever'])",
+ "genrule(name='x', cmd='touch $@', outs=['y'])");
+ Rule ruleAfter = getRule(buildFile, "x");
+
+ String hashBefore =
+ SyntheticAttributeHashCalculator.compute(
+ ruleBefore, /*serializedAttributes=*/ ImmutableMap.of(), /*extraDataForAttrHash=*/ "");
+ String hashAfter =
+ SyntheticAttributeHashCalculator.compute(
+ ruleAfter, /*serializedAttributes=*/ ImmutableMap.of(), /*extraDataForAttrHash=*/ "");
+
+ assertThat(hashBefore).isEqualTo(hashAfter);
+ }
+
+ @Test
+ public void testComputeSerializedAttributesUsedOverAvailable() throws Exception {
+ Rule rule =
+ getRule(scratch.file("pkg/BUILD", "genrule(name='x', cmd='touch $@', outs=['y'])"), "x");
+
+ String hashBefore =
+ SyntheticAttributeHashCalculator.compute(
+ rule, /*serializedAttributes=*/ ImmutableMap.of(), /*extraDataForAttrHash=*/ "");
+
+ ImmutableMap<Attribute, Build.Attribute> serializedAttributes =
+ ImmutableMap.of(
+ rule.getRuleClassObject().getAttributeByName("cmd"),
+ Build.Attribute.newBuilder()
+ .setName("dummy")
+ .setType(Discriminator.STRING)
+ .setStringValue("hi")
+ .build());
+
+ String hashAfter =
+ SyntheticAttributeHashCalculator.compute(
+ rule, serializedAttributes, /*extraDataForAttrHash*/ "");
+
+ assertThat(hashBefore).isNotEqualTo(hashAfter);
+ }
+
+ @Test
+ public void testComputeExtraDataChangesHash() throws Exception {
+ Rule rule =
+ getRule(scratch.file("pkg/BUILD", "genrule(name='x', cmd='touch $@', outs=['y'])"), "x");
+
+ String hashBefore =
+ SyntheticAttributeHashCalculator.compute(
+ rule, /*serializedAttributes=*/ ImmutableMap.of(), /*extraDataForAttrHash=*/ "");
+
+ String hashAfter =
+ SyntheticAttributeHashCalculator.compute(
+ rule,
+ /*serializedAttributes=*/ ImmutableMap.of(), /*extraDataForAttrHash*/
+ "blahblaah");
+
+ assertThat(hashBefore).isNotEqualTo(hashAfter);
+ }
+
+ @Test
+ public void testComputePackageErrorStatusChangesHash() throws Exception {
+ Path buildFile = scratch.file("pkg/BUILD");
+
+ scratch.overwriteFile("pkg/BUILD", "genrule(name='x', cmd='touch $@', outs=['y'])");
+ Rule ruleBefore = getRule(buildFile, "x");
+
+ // Remove fail-fast handler, we're intentionally creating a package with errors.
+ reporter.removeHandler(failFastHandler);
+ scratch.overwriteFile(
+ "pkg/BUILD",
+ "genrule(name='x', cmd='touch $@', outs=['z'])",
+ "genrule(name='missing_attributes')");
+ Rule ruleAfter = getRule(buildFile, "x");
+ assertThat(ruleAfter.containsErrors()).isTrue();
+
+ String hashBefore =
+ SyntheticAttributeHashCalculator.compute(
+ ruleBefore, /*serializedAttributes=*/ ImmutableMap.of(), /*extraDataForAttrHash=*/ "");
+ String hashAfter =
+ SyntheticAttributeHashCalculator.compute(
+ ruleAfter, /*serializedAttributes=*/ ImmutableMap.of(), /*extraDataForAttrHash=*/ "");
+
+ assertThat(hashBefore).isNotEqualTo(hashAfter);
+ }
+
+ private Rule getRule(Path buildFile, String rule)
+ throws NoSuchPackageException, InterruptedException {
+ Package pkg =
+ packageFactory.createPackageForTesting(
+ PackageIdentifier.createInMainRepo(buildFile.getParentDirectory().getBaseName()),
+ RootedPath.toRootedPath(root, buildFile),
+ getPackageManager(),
+ reporter);
+
+ return pkg.getRule(rule);
+ }
+}
