Skip to content

Commit

Permalink
Make repo marker file parsing robust to version changes
Browse files Browse the repository at this point in the history
Bazel 7.1.0 and 7.2.0 contains a bug where + characters in labels in the repository marker files cannot be parsed. This was fixed in commit d62e0a0. To reduce the rusk of future bugs in the same area, this change skips parsing the file if the first line shows that the content will not be used anyway, which should be reasonably safe if introducing new formats.

Improves #23336 that fixed #23322.

Closes #23642.

PiperOrigin-RevId: 679330823
Change-Id: I8123bdde047735ced3eabed3df68112d44318b08
  • Loading branch information
moroten authored and copybara-github committed Sep 26, 2024
1 parent c4eba38 commit c3393da
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,10 @@ static String unescape(String str) {
}

private static class DigestWriter {
// Input value map to force repo invalidation.
private static final ImmutableMap<RepoRecordedInput, String> NOT_UP_TO_DATE =
ImmutableMap.of(RepoRecordedInput.NEVER_UP_TO_DATE, "");

private final BlazeDirectories directories;
private final Path markerPath;
private final Rule rule;
Expand Down Expand Up @@ -681,43 +685,41 @@ byte[] areRepositoryAndMarkerFileConsistent(
return null;
}

Map<RepoRecordedInput, String> recordedInputValues = new TreeMap<>();
String content;
try {
content = FileSystemUtils.readContent(markerPath, UTF_8);
String markerRuleKey = readMarkerFile(content, recordedInputValues);
boolean verified = false;
if (Preconditions.checkNotNull(ruleKey).equals(markerRuleKey)) {
verified = handler.verifyRecordedInputs(rule, directories, recordedInputValues, env);
if (env.valuesMissing()) {
return null;
}
String content = FileSystemUtils.readContent(markerPath, UTF_8);
Map<RepoRecordedInput, String> recordedInputValues =
readMarkerFile(content, Preconditions.checkNotNull(ruleKey));
if (!handler.verifyRecordedInputs(rule, directories, recordedInputValues, env)) {
return null;
}

if (verified) {
return new Fingerprint().addString(content).digestAndReset();
} else {
if (env.valuesMissing()) {
return null;
}
return new Fingerprint().addString(content).digestAndReset();
} catch (IOException e) {
throw new RepositoryFunctionException(e, Transience.TRANSIENT);
}
}

@Nullable
private static String readMarkerFile(
String content, Map<RepoRecordedInput, String> recordedInputValues) {
String markerRuleKey = null;
private static Map<RepoRecordedInput, String> readMarkerFile(
String content, String expectedRuleKey) {
Iterable<String> lines = Splitter.on('\n').split(content);

boolean firstLine = true;
@Nullable Map<RepoRecordedInput, String> recordedInputValues = null;
boolean firstLineVerified = false;
for (String line : lines) {
if (line.isEmpty()) {
continue;
}
if (firstLine) {
markerRuleKey = line;
firstLine = false;
if (!firstLineVerified) {
if (!line.equals(expectedRuleKey)) {
// Break early, need to reload anyway. This also detects marker file version changes
// so that unknown formats are not parsed.
return NOT_UP_TO_DATE;
}
firstLineVerified = true;
recordedInputValues = new TreeMap<>();
} else {
int sChar = line.indexOf(' ');
if (sChar > 0) {
Expand All @@ -728,12 +730,13 @@ private static String readMarkerFile(
}
}
// On parse failure, just forget everything else and mark the whole input out of date.
recordedInputValues.clear();
recordedInputValues.put(RepoRecordedInput.NEVER_UP_TO_DATE, "");
break;
return NOT_UP_TO_DATE;
}
}
return markerRuleKey;
if (!firstLineVerified) {
return NOT_UP_TO_DATE;
}
return Preconditions.checkNotNull(recordedInputValues);
}

private String computeRuleKey(Rule rule, StarlarkSemantics starlarkSemantics) {
Expand Down
8 changes: 8 additions & 0 deletions src/test/shell/bazel/starlark_repository_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2895,6 +2895,14 @@ EOF
bazel shutdown
bazel build @foo >& $TEST_log || fail "expected bazel to succeed"
expect_log "I see: nothing"

# Test to clear the marker file.
echo > ${marker_file}

# Running Bazel again shouldn't crash, and should result in a refetch.
bazel shutdown
bazel build @foo >& $TEST_log || fail "expected bazel to succeed"
expect_log "I see: nothing"
}

function test_file_watching_in_undefined_repo() {
Expand Down

0 comments on commit c3393da

Please sign in to comment.