Skip to content

Commit

Permalink
Prevent --repo_env from triggering unnecessary fetches
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@philsc @philwo
philsc authored and philwo committed Mar 15, 2021
1 parent 5f94563 commit 9fae75a
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 26 deletions.
16 changes: 12 additions & 4 deletions src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,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
Expand Down Expand Up @@ -365,9 +369,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
Expand Down
87 changes: 65 additions & 22 deletions src/test/shell/bazel/starlark_repository_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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!"
}

Expand Down Expand Up @@ -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"
}

Expand Down Expand Up @@ -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 \
Expand All @@ -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 \
Expand All @@ -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 \
Expand All @@ -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")
Expand Down Expand Up @@ -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"
}

Expand Down Expand Up @@ -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
Expand All @@ -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'
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"
}

Expand Down Expand Up @@ -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"
}

Expand Down

0 comments on commit 9fae75a

Please sign in to comment.