From 321fe3b6b4e892821ee7dbf2d17dd8ae6a541913 Mon Sep 17 00:00:00 2001 From: Philipp Schrader Date: Tue, 9 Mar 2021 08:18:02 -0800 Subject: [PATCH] Prevent --repo_env from triggering unnecessary fetches 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 --- .../rules/repository/RepositoryFunction.java | 16 +++- .../shell/bazel/starlark_repository_test.sh | 87 ++++++++++++++----- 2 files changed, 77 insertions(+), 26 deletions(-) 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 index f0be814f5cc65b..16cc990f9d9748 100644 --- 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 declareEnvironmentDependencies( return null; } + // Only depend on --repo_env values that are specified in the "environ" attribute. Map repoEnv = new LinkedHashMap(environ); - for (Map.Entry 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 markerData, Enviro return false; } + // Only depend on --repo_env values that are specified in the "environ" attribute. Map repoEnv = new LinkedHashMap<>(environ); - for (Map.Entry 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 index 086c6f1249b8e4..0fbbeae8f99abb 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -120,7 +120,7 @@ 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" @@ -128,7 +128,7 @@ EOF 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,7 +154,7 @@ 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" @@ -162,7 +162,7 @@ EOF 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 </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" }