In repo rules, don't warn about generator_* attributes being non-canonical
A recent change added the implicit generator_* attributes ("generator_name",
"generator_function", and possibly "generator_location") to all repo rules.
This had the unfortunate side-effect of spamming all users of certain repo
rules with a warning that the values for these attributes are non-canonical.
Since these attributes are implicitly created and supplied by Bazel itself,
and since they do not (or should not) affect repo rule determinism, there is
no action needed on the part of the user. This change hardcodes them to be
excluded from the canonical warning.
Fixes #11040. Closes #11113.
PiperOrigin-RevId: 306478566
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryResolvedEvent.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryResolvedEvent.java
index a77d6fa..ea08824 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryResolvedEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryResolvedEvent.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.events.ExtendedEventHandler.ResolvedEvent;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.Rule;
@@ -245,17 +246,31 @@
}
/**
+ * Attributes that may be defined on a repository rule without affecting its canonical
+ * representation. These may be created implicitly by Bazel.
+ */
+ private static final ImmutableSet<String> IGNORED_ATTRIBUTE_NAMES =
+ ImmutableSet.of("generator_name", "generator_function", "generator_location");
+
+ /**
* Compare two maps from Strings to objects, returning a pair of the map with all entries not in
* the original map or in the original map, but with a different value, and the keys dropped from
* the original map. However, ignore changes where a value is explicitly set to its default.
+ *
+ * <p>Ignores attributes listed in {@code IGNORED_ATTRIBUTE_NAMES}.
*/
static Pair<Map<String, Object>, List<String>> compare(
Map<String, Object> orig, Map<String, Object> defaults, Map<?, ?> modified) {
ImmutableMap.Builder<String, Object> valuesChanged = ImmutableMap.<String, Object>builder();
for (Map.Entry<?, ?> entry : modified.entrySet()) {
if (entry.getKey() instanceof String) {
- Object value = entry.getValue();
String key = (String) entry.getKey();
+ if (IGNORED_ATTRIBUTE_NAMES.contains(key)) {
+ // The dict returned by the repo rule really shouldn't know about these anyway, but
+ // for symmetry we'll ignore them if they happen to be present.
+ continue;
+ }
+ Object value = entry.getValue();
Object old = orig.get(key);
if (old == null) {
Object defaultValue = defaults.get(key);
@@ -271,6 +286,9 @@
}
ImmutableList.Builder<String> keysDropped = ImmutableList.<String>builder();
for (String key : orig.keySet()) {
+ if (IGNORED_ATTRIBUTE_NAMES.contains(key)) {
+ continue;
+ }
if (!modified.containsKey(key)) {
keysDropped.add(key);
}
diff --git a/src/test/shell/bazel/workspace_resolved_test.sh b/src/test/shell/bazel/workspace_resolved_test.sh
index 12bf50b..2857d8f 100755
--- a/src/test/shell/bazel/workspace_resolved_test.sh
+++ b/src/test/shell/bazel/workspace_resolved_test.sh
@@ -824,6 +824,7 @@
EOF
bazel sync --distdir=${EXTREPODIR}/test_WORKSPACE/distdir --experimental_repository_resolved_file=resolved.bzl
+ cat resolved.bzl > /dev/null || fail "resolved.bzl should exist"
bazel sync --distdir=${EXTREPODIR}/test_WORKSPACE/distdir --experimental_repository_hash_file=`pwd`/resolved.bzl \
--experimental_verify_repository_rules='//:rule.bzl%time_rule' \
> "${TEST_log}" 2>&1 && fail "expected failure" || :
@@ -1216,4 +1217,52 @@
expect_log " TEST_TMPDIR/.*/external/bazel_tools/tools/build_defs/repo/http.bzl:"
}
+# Regression test for #11040.
+#
+# Test that a canonical repo warning is generated for explicitly specified
+# attributes whose values differ, and that it is never generated for implicitly
+# created attributes (in particular, the generator_* attributes).
+test_canonical_warning() {
+ EXTREPODIR=`pwd`
+ tar xvf ${TEST_SRCDIR}/test_WORKSPACE_files/archives.tar
+
+ mkdir main
+ touch main/BUILD
+ cat > main/reporule.bzl <<EOF
+def _impl(repository_ctx):
+ repository_ctx.file("a.txt", "A")
+ repository_ctx.file("BUILD.bazel", "exports_files(['a.txt'])")
+ # Don't include "name", test that we warn about it below.
+ return {"myattr": "bar"}
+
+reporule = repository_rule(
+ implementation = _impl,
+ attrs = {
+ "myattr": attr.string()
+ })
+
+# We need to use a macro for the generator_* attributes to be defined.
+def instantiate_reporule(name, **kwargs):
+ reporule(name=name, **kwargs)
+EOF
+ cat > main/WORKSPACE <<EOF
+workspace(name = "main")
+load("//:reporule.bzl", "instantiate_reporule")
+instantiate_reporule(
+ name = "myrepo",
+ myattr = "foo"
+)
+EOF
+
+ cd main
+ # We should get a warning for "myattr" having a changed value and for "name"
+ # being dropped, but not for the generator_* attributes.
+ bazel sync --distdir=${EXTREPODIR}/test_WORKSPACE/distdir >/dev/null 2>$TEST_log
+ bazel clean --expunge
+ expect_log "Rule 'myrepo' indicated that a canonical reproducible form \
+can be obtained by modifying arguments myattr = \"bar\" and dropping \
+\[.*\"name\".*\]"
+ expect_not_log "Rule 'myrepo' indicated .*generator"
+}
+
run_suite "workspace_resolved_test tests"
