Prevent --repo_env from triggering unnecessary fetches

philsc · copybara-github · commit 321fe3b6b4e8 · 2021-03-09T08:19:04.000-08:00
It looks like `--repo_env` entries get added to every repository rule's marker data. That is regardless of whether the repository rule declared that it uses the environment variable. This causes unnecessary fetches of the external repositories. This patch aims to fix that by making repository rules only depend on `--repo_env` values that they specify in their `environ` attributes. I also took this opportunity to fix up all instances of `bazel-genfiles` in `starlark_repository_test.sh` to `bazel-bin`. Closes #13126. PiperOrigin-RevId: 361813865
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -331,9 +331,13 @@ protected Map<String, String> declareEnvironmentDependencies(
       return null;
     }
 
+    // Only depend on --repo_env values that are specified in the "environ" attribute.
     Map<String, String> repoEnv = new LinkedHashMap<String, String>(environ);
-    for (Map.Entry<String, String> value : repoEnvOverride.entrySet()) {
-      repoEnv.put(value.getKey(), value.getValue());
+    for (String key : keys) {
+      String value = repoEnvOverride.get(key);
+      if (value != null) {
+        repoEnv.put(key, value);
+      }
     }
 
     // Add the dependencies to the marker file
@@ -362,9 +366,13 @@ protected boolean verifyEnvironMarkerData(Map<String, String> markerData, Enviro
       return false;
     }
 
+    // Only depend on --repo_env values that are specified in the "environ" attribute.
     Map<String, String> repoEnv = new LinkedHashMap<>(environ);
-    for (Map.Entry<String, String> value : repoEnvOverride.entrySet()) {
-      repoEnv.put(value.getKey(), value.getValue());
+    for (String key : keys) {
+      String value = repoEnvOverride.get(key);
+      if (value != null) {
+        repoEnv.put(key, value);
+      }
     }
 
     // Verify that all environment variable in the marker file are also in keys
diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh
@@ -120,15 +120,15 @@ EOF
   bazel build //zoo:ball-pit1 >& $TEST_log || fail "Failed to build"
   expect_log "bleh"
   expect_log "Tra-la!"  # Invalidation
-  cat bazel-genfiles/zoo/ball-pit1.txt >$TEST_log
+  cat bazel-bin/zoo/ball-pit1.txt >$TEST_log
   expect_log "Tra-la!"
 
   bazel build //zoo:ball-pit1 >& $TEST_log || fail "Failed to build"
   expect_not_log "Tra-la!"  # No invalidation
 
   bazel build //zoo:ball-pit2 >& $TEST_log || fail "Failed to build"
   expect_not_log "Tra-la!"  # No invalidation
-  cat bazel-genfiles/zoo/ball-pit2.txt >$TEST_log
+  cat bazel-bin/zoo/ball-pit2.txt >$TEST_log
   expect_log "Tra-la!"
 
   # Test invalidation of the WORKSPACE file
@@ -154,15 +154,15 @@ EOF
   bazel build //zoo:ball-pit1 >& $TEST_log || fail "Failed to build"
   expect_log "blah"
   expect_log "Tra-la-la!"  # Invalidation
-  cat bazel-genfiles/zoo/ball-pit1.txt >$TEST_log
+  cat bazel-bin/zoo/ball-pit1.txt >$TEST_log
   expect_log "Tra-la-la!"
 
   bazel build //zoo:ball-pit1 >& $TEST_log || fail "Failed to build"
   expect_not_log "Tra-la-la!"  # No invalidation
 
   bazel build //zoo:ball-pit2 >& $TEST_log || fail "Failed to build"
   expect_not_log "Tra-la-la!"  # No invalidation
-  cat bazel-genfiles/zoo/ball-pit2.txt >$TEST_log
+  cat bazel-bin/zoo/ball-pit2.txt >$TEST_log
   expect_log "Tra-la-la!"
 }
 
@@ -355,7 +355,7 @@ EOF
   bazel build @foo//:bar >& $TEST_log || fail "Failed to build"
   expect_log "foo"
   expect_not_log "Workspace name in .*/WORKSPACE (.*) does not match the name given in the repository's definition (@foo)"
-  cat bazel-genfiles/external/foo/bar.txt >$TEST_log
+  cat bazel-bin/external/foo/bar.txt >$TEST_log
   expect_log "foo"
 }
 
@@ -892,8 +892,8 @@ build:bar --repo_env=FOO=bar
 EOF
 
   bazel build --config=foo //:repoenv //:unrelated
-  cp `bazel info bazel-genfiles 2>/dev/null`/repoenv.txt repoenv1.txt
-  cp `bazel info bazel-genfiles 2> /dev/null`/unrelated.txt unrelated1.txt
+  cp `bazel info bazel-bin 2>/dev/null`/repoenv.txt repoenv1.txt
+  cp `bazel info bazel-bin 2> /dev/null`/unrelated.txt unrelated1.txt
   echo; cat repoenv1.txt; echo; cat unrelated1.txt; echo
 
   grep -q 'FOO=foo' repoenv1.txt \
@@ -904,8 +904,8 @@ EOF
   FOO=CHANGED bazel build --config=foo //:repoenv //:unrelated
   # nothing should change, as actions don't see FOO and for repositories
   # the value is fixed by --repo_env
-  cp `bazel info bazel-genfiles 2>/dev/null`/repoenv.txt repoenv2.txt
-  cp `bazel info bazel-genfiles 2> /dev/null`/unrelated.txt unrelated2.txt
+  cp `bazel info bazel-bin 2>/dev/null`/repoenv.txt repoenv2.txt
+  cp `bazel info bazel-bin 2> /dev/null`/unrelated.txt unrelated2.txt
   echo; cat repoenv2.txt; echo; cat unrelated2.txt; echo
 
   diff repoenv1.txt repoenv2.txt \
@@ -916,8 +916,8 @@ EOF
   bazel build --config=bar //:repoenv //:unrelated
   # The new config should be picked up, but the unrelated target should
   # not be rerun
-  cp `bazel info bazel-genfiles 3>/dev/null`/repoenv.txt repoenv3.txt
-  cp `bazel info bazel-genfiles 3> /dev/null`/unrelated.txt unrelated3.txt
+  cp `bazel info bazel-bin 3>/dev/null`/repoenv.txt repoenv3.txt
+  cp `bazel info bazel-bin 3> /dev/null`/unrelated.txt unrelated3.txt
   echo; cat repoenv3.txt; echo; cat unrelated3.txt; echo
 
   grep -q 'FOO=bar' repoenv3.txt \
@@ -926,6 +926,49 @@ EOF
       || fail "Expected unrelated action to not be rerun"
 }
 
+function test_repo_env_inverse() {
+  # This test makes sure that a repository rule that has no dependencies on
+  # environment variables does _not_ get refetched when --repo_env changes.
+  setup_starlark_repository
+
+  cat > test.bzl <<'EOF'
+def _impl(ctx):
+  # Record a time stamp to verify that the rule is not rerun.
+  ctx.execute(["bash", "-c", "date +%s >> env.txt"])
+  ctx.file("BUILD", 'exports_files(["env.txt"])')
+
+repo = repository_rule(
+  implementation = _impl,
+)
+EOF
+  cat > BUILD <<'EOF'
+genrule(
+  name = "repoenv",
+  outs = ["repoenv.txt"],
+  srcs = ["@foo//:env.txt"],
+  cmd = "cp $< $@",
+)
+EOF
+  cat > .bazelrc <<EOF
+build:foo --repo_env=FOO=foo
+build:bar --repo_env=FOO=bar
+EOF
+
+  bazel build --config=foo //:repoenv
+  cp `bazel info bazel-bin 2>/dev/null`/repoenv.txt repoenv1.txt
+  echo; cat repoenv1.txt; echo;
+
+  sleep 2 # ensure any rerun will have a different time stamp
+
+  bazel build --config=bar //:repoenv
+  # The new config should not trigger a rerun of repoenv.
+  cp `bazel info bazel-bin 2>/dev/null`/repoenv.txt repoenv2.txt
+  echo; cat repoenv2.txt; echo;
+
+  diff repoenv1.txt repoenv2.txt \
+      || fail "Expected repository to not change"
+}
+
 function test_repo_env_invalidation() {
     # regression test for https://github.com/bazelbuild/bazel/issues/8869
     WRKDIR=$(mktemp -d "${TEST_TMPDIR}/testXXXXXX")
@@ -962,18 +1005,18 @@ genrule(
 EOF
 
     bazel build //:repotime
-    cp `bazel info bazel-genfiles 2>/dev/null`/repotime.txt time1.txt
+    cp `bazel info bazel-bin 2>/dev/null`/repotime.txt time1.txt
 
     sleep 2;
     bazel build --repo_env=foo=bar //:repotime
-    cp `bazel info bazel-genfiles 2>/dev/null`/repotime.txt time2.txt
+    cp `bazel info bazel-bin 2>/dev/null`/repotime.txt time2.txt
     diff time1.txt time2.txt && fail "Expected repo to be refetched" || :
 
     bazel shutdown
     sleep 2;
 
     bazel build --repo_env=foo=bar //:repotime
-    cp `bazel info bazel-genfiles 2>/dev/null`/repotime.txt time3.txt
+    cp `bazel info bazel-bin 2>/dev/null`/repotime.txt time3.txt
     diff time2.txt time3.txt || fail "Expected repo to not be refetched"
 }
 
@@ -1387,9 +1430,9 @@ EOF
   echo "initial" > reference.txt.shadow
 
   bazel build //:source //:configure
-  grep 'initial' `bazel info bazel-genfiles`/source.txt \
+  grep 'initial' `bazel info bazel-bin`/source.txt \
        || fail '//:source not generated properly'
-  grep 'initial' `bazel info bazel-genfiles`/configure.txt \
+  grep 'initial' `bazel info bazel-bin`/configure.txt \
        || fail '//:configure not generated properly'
 
   echo "new value" > reference.txt.shadow
@@ -1401,9 +1444,9 @@ EOF
       && fail "Expected 'source' not to be synced" || :
 
   bazel build //:source //:configure
-  grep -q 'initial' `bazel info bazel-genfiles`/source.txt \
+  grep -q 'initial' `bazel info bazel-bin`/source.txt \
        || fail '//:source did not keep its old value'
-  grep -q 'new value' `bazel info bazel-genfiles`/configure.txt \
+  grep -q 'new value' `bazel info bazel-bin`/configure.txt \
        || fail '//:configure not synced properly'
 }
 
@@ -1752,9 +1795,9 @@ load("@netrc//:data.bzl", "netrc")
 ) for name in ["login", "password"]]
 EOF
   bazel build //:login //:password
-  grep 'myusername' `bazel info bazel-genfiles`/login.txt \
+  grep 'myusername' `bazel info bazel-bin`/login.txt \
        || fail "Username not parsed correctly"
-  grep 'mysecret' `bazel info bazel-genfiles`/password.txt \
+  grep 'mysecret' `bazel info bazel-bin`/password.txt \
        || fail "Password not parsed correctly"
 
   # Also check the precise value of parsed file
@@ -1789,7 +1832,7 @@ genrule(
 )
 EOF
   bazel build //:check_expected
-  grep 'OK' `bazel info bazel-genfiles`/check_expected.txt \
+  grep 'OK' `bazel info bazel-bin`/check_expected.txt \
        || fail "Parsed dict not equal to expected value"
 }
 
@@ -1895,7 +1938,7 @@ genrule(
 )
 EOF
   bazel build //:check_expected
-  grep 'OK' `bazel info bazel-genfiles`/check_expected.txt \
+  grep 'OK' `bazel info bazel-bin`/check_expected.txt \
        || fail "Authentication merged incorrectly"
 }
 
