Repository: bazelbuild/bazel
Issue: Warning: Rule '<every remote repository rule>' indicated that a canonical reproducible form can be obtained by dropping arguments ["generator_name", "generator_function"]
Labels: type: bug
P0

I'm seeing these warnings for every remote repository rule on Bazel@HEAD (5c1c0d1ed8643c90fba239d1bc642e4ebd044c2c).

Here are the warnings for [gitiles](https://gerrit.googlesource.com/gitiles) repository:

```
  $ bazel build :gitiles
Starting local Bazel server and connecting to it...
INFO: Invocation ID: 0e7d47af-ed3f-4882-8129-8ead508a664b
DEBUG: Rule 'com_googlesource_gerrit_bazlets' indicated that a canonical reproducible form can be obtained by modifying arguments shallow_since = "1581093008 -0500" and dropping ["generator_name", "generator_function"]
DEBUG: Repository com_googlesource_gerrit_bazlets instantiated at:
  no stack (--record_rule_instantiation_callstack not enabled)
Repository rule git_repository defined at:
  /home/davido/.cache/bazel/_bazel_davido/9fdffb7f2a398de2e1a63eaee2ea7c50/external/bazel_tools/tools/build_defs/repo/git.bzl:195:18: in <toplevel>
DEBUG: Rule 'rules_java' indicated that a canonical reproducible form can be obtained by dropping arguments ["generator_name", "generator_function"]
DEBUG: Repository rules_java instantiated at:
  no stack (--record_rule_instantiation_callstack not enabled)
Repository rule http_archive defined at:
  /home/davido/.cache/bazel/_bazel_davido/9fdffb7f2a398de2e1a63eaee2ea7c50/external/bazel_tools/tools/build_defs/repo/http.bzl:336:16: in <toplevel>
DEBUG: Rule 'rules_cc' indicated that a canonical reproducible form can be obtained by dropping arguments ["generator_name", "generator_function"]
DEBUG: Repository rules_cc instantiated at:
  no stack (--record_rule_instantiation_callstack not enabled)
Repository rule http_archive defined at:
  /home/davido/.cache/bazel/_bazel_davido/9fdffb7f2a398de2e1a63eaee2ea7c50/external/bazel_tools/tools/build_defs/repo/http.bzl:336:16: in <toplevel>
DEBUG: Rule 'remote_java_tools_linux' indicated that a canonical reproducible form can be obtained by dropping arguments ["generator_name", "generator_function"]
DEBUG: Repository remote_java_tools_linux instantiated at:
  no stack (--record_rule_instantiation_callstack not enabled)
Repository rule http_archive defined at:
  /home/davido/.cache/bazel/_bazel_davido/9fdffb7f2a398de2e1a63eaee2ea7c50/external/bazel_tools/tools/build_defs/repo/http.bzl:336:16: in <toplevel>
DEBUG: Rule 'rules_proto' indicated that a canonical reproducible form can be obtained by dropping arguments ["generator_name", "generator_function"]
DEBUG: Repository rules_proto instantiated at:
  no stack (--record_rule_instantiation_callstack not enabled)
Repository rule http_archive defined at:
  /home/davido/.cache/bazel/_bazel_davido/9fdffb7f2a398de2e1a63eaee2ea7c50/external/bazel_tools/tools/build_defs/repo/http.bzl:336:16: in <toplevel>
DEBUG: Rule 'remotejdk11_linux' indicated that a canonical reproducible form can be obtained by dropping arguments ["generator_name", "generator_function"]
DEBUG: Repository remotejdk11_linux instantiated at:
  no stack (--record_rule_instantiation_callstack not enabled)
Repository rule http_archive defined at:
  /home/davido/.cache/bazel/_bazel_davido/9fdffb7f2a398de2e1a63eaee2ea7c50/external/bazel_tools/tools/build_defs/repo/http.bzl:336:16: in <toplevel>
```

Issue comments:
Bisecting the problem:

ebb86fcdc202c440a362a54f6b0de8a049dfa84b is the first bad commit
"bazel packages: record Starlark stack in RuleClass at creation"

//CC @alandonovan 
cc @laurentlb This is a UI regression.
cc/ +Alan Donovan <adonovan@google.com>

On Sat, Apr 4, 2020 at 8:33 AM Jin <notifications@github.com> wrote:

> cc @laurentlb <https://github.com/laurentlb> This is a UI regression.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/bazelbuild/bazel/issues/11040#issuecomment-609021971>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAXHHHHWXUN4Q2UYR5MPQMLRK4SH5ANCNFSM4LV42EQA>
> .
>

Sorry for the regression. My change (ebb86fc) had the necessary effect of plumbing the callstack from Starlark all the way to repository_rule instantiation, which means that repository_rules now have generator_* attributes, just like ordinary package rules. Though I forgot to mention it in the change description, this was an intentional behavior change that seemed like a good thing.

Is it a good thing?  If so, then the RepositoryResolvedEvent machinery (which I don't understand at all) will need to change in some way to accommodate it. If not, then I will need to suppress the creation of generator_* attributes for repository rules.

To me, the first of these approaches seems simpler (though perhaps not easier), because it moves complexity out of the core.

@laurentl @susinmotion 
@brandjon  Who owns the WORKSPACE.resolved hack^Wmachinery?
I'm afraid no one owns (or is familiar) with this machinery anymore.
Both approaches seem reasonable to me.
According to repository rule [docs](https://docs.bazel.build/versions/3.0.0/skylark/repository_rules.html#implementation-function), repo rule functions may return a dict indicating what params would need to be passed in order to make the call to the repo rule reproducible in the future. Since repo rules now have new implicit attributes, this logic will identify their absence in any returned dict as indicating a problem.

I'll go ahead and make the repo rule machinery explicitly ignore those particular builtin attributes by hardcoding their names. I'm unaware of any more general mechanism we have for identifying implicit attributes (aside from special character names like `$`, which I don't think applies here).
Got it; your solution sounds reasonable. Thanks for investigating.
FTR, I was going to give the same treatment to the `name` attribute as well, since it too is implicitly available on every repository rule. But on further reflection, that seems like a bad idea since the returned dictionary feeds into the resolved file machinery (we presumably wouldn't want to encourage users to omit a critical field there).

Pull request review comments:

Patch that resolved the issue:
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 a77d6fa8816fd4..5520eaa70b5d1b 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
@@ -244,18 +244,32 @@ public static String getRuleDefinitionInformation(Rule rule) {
     return buf.toString();
   }
 
+  /**
+   * 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 +285,9 @@ static Pair<Map<String, Object>, List<String>> compare(
     }
     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);
       }


Test patch:
diff --git a/src/test/shell/bazel/workspace_resolved_test.sh b/src/test/shell/bazel/workspace_resolved_test.sh
index 12bf50be264349..9a82cf744e4173 100755
--- a/src/test/shell/bazel/workspace_resolved_test.sh
+++ b/src/test/shell/bazel/workspace_resolved_test.sh
@@ -1216,4 +1216,51 @@ EOF
   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
+  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"