Skip to content

Commit

Permalink
Fix unconditional Skyframe invalidation with --lockfile_mode=update
Browse files Browse the repository at this point in the history
Ensure that `BazelLockFileModule` only updates `MODULE.bazel.lock` if the content of the file needs to change. Every such update changes the file's metadata, which results in Skyframe invalidation of, in particular, all configurations. This broke `bazel config`, which uses `MemoizingEvaluator#getDoneValues()` to directly observe Skyframe state.

Since this type of invalidation can also be caused by users and deviates from the usual "incremental correctness" guarantees provided by Bazel, let `config` show a descriptive error when no configurations are found.

Fixes bazelbuild#19823

Closes bazelbuild#19842.

PiperOrigin-RevId: 574133346
Change-Id: I5886c91fc6b7b938a7dee59ea75aa7b8afb5b161
  • Loading branch information
fmeum authored and copybara-github committed Oct 17, 2023
1 parent d15ea92 commit 78db9ae
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,13 @@ public void afterCommand() throws AbruptExitException {
combineModuleExtensions(lockfile.getModuleExtensions(), oldExtensionUsages))
.build();

// Write the new value to the file
updateLockfile(lockfilePath, lockfile);
// Write the new value to the file, but only if needed. This is not just a performance
// optimization: whenever the lockfile is updated, most Skyframe nodes will be marked as dirty
// on the next build, which breaks commands such as `bazel config` that rely on
// com.google.devtools.build.skyframe.MemoizingEvaluator#getDoneValues.
if (!lockfile.equals(oldLockfile)) {
updateLockfile(lockfilePath, lockfile);
}
this.moduleResolutionEvent = null;
this.extensionResolutionEventsMap.clear();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,15 @@ public int hashCode() {
public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult options) {
ImmutableSortedMap<BuildConfigurationKey, BuildConfigurationValue> configurations =
findConfigurations(env);
if (configurations.isEmpty()) {
String message =
"No configurations found. This can happen if the 'config' subcommand is used after "
+ "files, including their metadata, have changed since the last invocation of "
+ "another subcommand. Try running a 'build' or 'cquery' directly followed by "
+ "'config'.";
env.getReporter().handle(Event.error(message));
return createFailureResult(message, Code.CONFIGURATION_NOT_FOUND);
}

try (PrintWriter writer =
new PrintWriter(
Expand Down
11 changes: 4 additions & 7 deletions src/test/shell/integration/configured_query_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,6 @@ fi

add_to_bazelrc "build --package_path=%workspace%"

# TODO: enable Bzlmod for this Test
# https://github.com/bazelbuild/bazel/issues/19823
disable_bzlmod

#### TESTS #############################################################

function test_basic_query() {
Expand Down Expand Up @@ -897,8 +893,8 @@ EOF
--starlark:expr="str(target.label) + '%foo'" > output \
2>"$TEST_log" || fail "Expected success"

assert_contains "^@//$pkg:pylib%foo$" output
assert_contains "^@//$pkg:pylibtwo%foo$" output
assert_contains "^@@\?//$pkg:pylib%foo$" output
assert_contains "^@@\?//$pkg:pylibtwo%foo$" output

bazel cquery "//$pkg:all" --output=starlark \
--noincompatible_unambiguous_label_stringification \
Expand Down Expand Up @@ -1294,7 +1290,7 @@ EOF
2>"$TEST_log" || fail "Expected success"
assert_contains "//$pkg:srcfile.txt:providers=.*FileProvider.*FilesToRunProvider.*LicensesProvider.*VisibilityProvider" \
output
assert_contains "VisibilityProvider.label:@//$pkg:srcfile.txt" output
assert_contains "VisibilityProvider.label:@@\?//$pkg:srcfile.txt" output
}

function test_starlark_output_providers_starlark_provider() {
Expand Down Expand Up @@ -1394,6 +1390,7 @@ sh_library(name='japanese')
EOF

mkdir -p $dir/main
write_default_lockfile $dir/main/MODULE.bazel.lock
cat > $dir/main/WORKSPACE <<EOF
local_repository(name = "repo", path = "../repo")
EOF
Expand Down

0 comments on commit 78db9ae

Please sign in to comment.