From 0a6880d2f322be240ffb0d619dd97311148afc34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Mon, 19 Aug 2024 14:07:28 -0400 Subject: [PATCH 1/3] Ignore invalid entries in the marker file Especially due to https://github.com/bazelbuild/bazel/issues/23127, older marker files may contain entries that don't even parse correctly. Instead of crashing Bazel, we should just ignore such entries. Fixes https://github.com/bazelbuild/bazel/issues/23322. --- .../rules/repository/RepoRecordedInput.java | 56 +++++++++++++++---- .../shell/bazel/starlark_repository_test.sh | 44 +++++++++++++++ 2 files changed, 89 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index 7dc9bdeb96b9b8..a529ec93a7b10e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.cmdline.LabelConstants; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.LabelValidator; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction; @@ -76,8 +77,10 @@ public abstract static class Parser { /** * Parses a recorded input from the post-colon substring that identifies the specific input: for - * example, the {@code MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. + * example, the {@code MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. Returns null if the parsed + * part is invalid. */ + @Nullable public abstract RepoRecordedInput parse(String s); } @@ -95,6 +98,9 @@ public abstract static class Parser { @Nullable public static RepoRecordedInput parse(String s) { List parts = Splitter.on(':').limit(2).splitToList(s); + if (parts.size() < 2) { + return null; + } for (Parser parser : new Parser[] { File.PARSER, Dirents.PARSER, DirTree.PARSER, EnvVar.PARSER, RecordedRepoMapping.PARSER @@ -213,12 +219,12 @@ public final String toString() { return repoName().map(repoName -> repoName + "//" + path()).orElse(path().toString()); } - public static RepoCacheFriendlyPath parse(String s) { + public static RepoCacheFriendlyPath parse(String s) throws LabelSyntaxException { if (LabelValidator.isAbsolute(s)) { int doubleSlash = s.indexOf("//"); int skipAts = s.startsWith("@@") ? 2 : s.startsWith("@") ? 1 : 0; return createInsideWorkspace( - RepositoryName.createUnvalidated(s.substring(skipAts, doubleSlash)), + RepositoryName.create(s.substring(skipAts, doubleSlash)), PathFragment.create(s.substring(doubleSlash + 2))); } return createOutsideWorkspace(PathFragment.create(s)); @@ -262,9 +268,15 @@ public String getPrefix() { return "FILE"; } + @Nullable @Override public RepoRecordedInput parse(String s) { - return new File(RepoCacheFriendlyPath.parse(s)); + try { + return new File(RepoCacheFriendlyPath.parse(s)); + } catch (LabelSyntaxException e) { + // ignore malformed input + return null; + } } }; @@ -354,9 +366,15 @@ public String getPrefix() { return "DIRENTS"; } + @Nullable @Override public RepoRecordedInput parse(String s) { - return new Dirents(RepoCacheFriendlyPath.parse(s)); + try { + return new Dirents(RepoCacheFriendlyPath.parse(s)); + } catch (LabelSyntaxException e) { + // Ignore malformed input. + return null; + } } }; @@ -437,9 +455,15 @@ public String getPrefix() { return "DIRTREE"; } + @Nullable @Override public RepoRecordedInput parse(String s) { - return new DirTree(RepoCacheFriendlyPath.parse(s)); + try { + return new DirTree(RepoCacheFriendlyPath.parse(s)); + } catch (LabelSyntaxException e) { + // ignore malformed input + return null; + } } }; @@ -573,11 +597,16 @@ public String getPrefix() { return "REPO_MAPPING"; } + @Nullable @Override public RepoRecordedInput parse(String s) { List parts = Splitter.on(',').limit(2).splitToList(s); - return new RecordedRepoMapping( - RepositoryName.createUnvalidated(parts.get(0)), parts.get(1)); + try { + return new RecordedRepoMapping(RepositoryName.create(parts.get(0)), parts.get(1)); + } catch (LabelSyntaxException | IndexOutOfBoundsException e) { + // ignore malformed input + return null; + } } }; @@ -632,9 +661,14 @@ public boolean isUpToDate( throws InterruptedException { RepositoryMappingValue repoMappingValue = (RepositoryMappingValue) env.getValue(getSkyKey(directories)); - return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE - && RepositoryName.createUnvalidated(oldValue) - .equals(repoMappingValue.getRepositoryMapping().get(apparentName)); + try { + return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE + && RepositoryName.create(oldValue) + .equals(repoMappingValue.getRepositoryMapping().get(apparentName)); + } catch (LabelSyntaxException e) { + // ignore malformed old value + return false; + } } } } diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index e11e457b30a104..09d489f66e7aef 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2864,6 +2864,50 @@ EOF expect_log "I see: something" } +function test_bad_marker_file_ignored() { + # when reading a file in another repo, we should watch it. + local outside_dir="${TEST_TMPDIR}/outside_dir" + mkdir -p "${outside_dir}" + echo nothing > ${outside_dir}/data.txt + + create_new_workspace + cat > $(setup_module_dot_bazel) < r.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: nothing" + + local marker_file=$(bazel info output_base)/external/@+_repo_rules+foo.marker + # the marker file for this repo should contain a reference to "@@+_repo_rules+bar". Mangle that. + sed -i'' -e 's/@@+_repo_rules+bar/@@LOL@@LOL/g' ${marker_file} + # add some more nonsensical lines for good measure... + echo 'BLAH:BLEH:BLOO BLEE' > ${marker_file} + echo 'no colons at all' > ${marker_file} + echo 'REPO_MAPPING:bad**bad,worse**worse gaaa' + echo 'REPO_MAPPING:bad,worse worst**worst' + + # Running Bazel again shouldn't crash. + bazel shutdown + bazel build @foo >& $TEST_log || fail "expected bazel to succeed" +} + function test_file_watching_in_undefined_repo() { create_new_workspace cat > $(setup_module_dot_bazel) < Date: Mon, 19 Aug 2024 19:40:33 -0400 Subject: [PATCH 2/3] refetch instead of ignoring invalid inputs --- .../rules/repository/RepoRecordedInput.java | 76 ++++++++++++++----- .../RepositoryDelegatorFunction.java | 9 ++- .../shell/bazel/starlark_repository_test.sh | 8 +- 3 files changed, 63 insertions(+), 30 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index a529ec93a7b10e..604bb584c84ca7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java @@ -80,26 +80,28 @@ public abstract static class Parser { * example, the {@code MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. Returns null if the parsed * part is invalid. */ - @Nullable public abstract RepoRecordedInput parse(String s); } private static final Comparator COMPARATOR = - Comparator.comparing((RepoRecordedInput rri) -> rri.getParser().getPrefix()) - .thenComparing(RepoRecordedInput::toStringInternal); + (o1, o2) -> + o1 == o2 + ? 0 + : Comparator.comparing((RepoRecordedInput rri) -> rri.getParser().getPrefix()) + .thenComparing(RepoRecordedInput::toStringInternal) + .compare(o1, o2); /** * Parses a recorded input from its string representation. * * @param s the string representation - * @return The parsed recorded input object, or {@code null} if the string representation is - * invalid + * @return The parsed recorded input object, or {@link #NEVER_UP_TO_DATE} if the string + * representation is invalid */ - @Nullable public static RepoRecordedInput parse(String s) { List parts = Splitter.on(':').limit(2).splitToList(s); if (parts.size() < 2) { - return null; + return NEVER_UP_TO_DATE; } for (Parser parser : new Parser[] { @@ -109,7 +111,7 @@ public static RepoRecordedInput parse(String s) { return parser.parse(parts.get(1)); } } - return null; + return NEVER_UP_TO_DATE; } /** @@ -178,6 +180,42 @@ public abstract boolean isUpToDate( Environment env, BlazeDirectories directories, @Nullable String oldValue) throws InterruptedException; + /** A sentinel "input" that's always out-of-date to signify parse failure. */ + public static final RepoRecordedInput NEVER_UP_TO_DATE = + new RepoRecordedInput() { + @Override + public boolean equals(Object obj) { + return this == obj; + } + + @Override + public int hashCode() { + return 12345678; + } + + @Override + public String toStringInternal() { + throw new UnsupportedOperationException("this sentinel input should never be serialized"); + } + + @Override + public Parser getParser() { + throw new UnsupportedOperationException("this sentinel input should never be parsed"); + } + + @Override + public SkyKey getSkyKey(BlazeDirectories directories) { + // Return a random SkyKey to satisfy the contract. + return PrecomputedValue.STARLARK_SEMANTICS.getKey(); + } + + @Override + public boolean isUpToDate( + Environment env, BlazeDirectories directories, @Nullable String oldValue) { + return false; + } + }; + /** * Represents a filesystem path stored in a way that is repo-cache-friendly. That is, if the path * happens to point inside the current Bazel workspace (in either the main repo or an external @@ -268,14 +306,13 @@ public String getPrefix() { return "FILE"; } - @Nullable @Override public RepoRecordedInput parse(String s) { try { return new File(RepoCacheFriendlyPath.parse(s)); } catch (LabelSyntaxException e) { - // ignore malformed input - return null; + // malformed inputs cause refetch + return NEVER_UP_TO_DATE; } } }; @@ -366,14 +403,13 @@ public String getPrefix() { return "DIRENTS"; } - @Nullable @Override public RepoRecordedInput parse(String s) { try { return new Dirents(RepoCacheFriendlyPath.parse(s)); } catch (LabelSyntaxException e) { - // Ignore malformed input. - return null; + // malformed inputs cause refetch + return NEVER_UP_TO_DATE; } } }; @@ -455,14 +491,13 @@ public String getPrefix() { return "DIRTREE"; } - @Nullable @Override public RepoRecordedInput parse(String s) { try { return new DirTree(RepoCacheFriendlyPath.parse(s)); } catch (LabelSyntaxException e) { - // ignore malformed input - return null; + // malformed inputs cause refetch + return NEVER_UP_TO_DATE; } } }; @@ -597,15 +632,14 @@ public String getPrefix() { return "REPO_MAPPING"; } - @Nullable @Override public RepoRecordedInput parse(String s) { List parts = Splitter.on(',').limit(2).splitToList(s); try { return new RecordedRepoMapping(RepositoryName.create(parts.get(0)), parts.get(1)); } catch (LabelSyntaxException | IndexOutOfBoundsException e) { - // ignore malformed input - return null; + // malformed inputs cause refetch + return NEVER_UP_TO_DATE; } } }; @@ -666,7 +700,7 @@ public boolean isUpToDate( && RepositoryName.create(oldValue) .equals(repoMappingValue.getRepositoryMapping().get(apparentName)); } catch (LabelSyntaxException e) { - // ignore malformed old value + // malformed old value causes refetch return false; } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index 6ac3f346914e09..46554761d197ad 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -710,12 +710,15 @@ private static String readMarkerFile( int sChar = line.indexOf(' '); if (sChar > 0) { RepoRecordedInput input = RepoRecordedInput.parse(unescape(line.substring(0, sChar))); - if (input == null) { - // ignore invalid entries. + if (!input.equals(RepoRecordedInput.NEVER_UP_TO_DATE)) { + recordedInputValues.put(input, unescape(line.substring(sChar + 1))); continue; } - recordedInputValues.put(input, unescape(line.substring(sChar + 1))); } + // 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 markerRuleKey; diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 09d489f66e7aef..baf767932016ee 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2897,15 +2897,11 @@ EOF local marker_file=$(bazel info output_base)/external/@+_repo_rules+foo.marker # the marker file for this repo should contain a reference to "@@+_repo_rules+bar". Mangle that. sed -i'' -e 's/@@+_repo_rules+bar/@@LOL@@LOL/g' ${marker_file} - # add some more nonsensical lines for good measure... - echo 'BLAH:BLEH:BLOO BLEE' > ${marker_file} - echo 'no colons at all' > ${marker_file} - echo 'REPO_MAPPING:bad**bad,worse**worse gaaa' - echo 'REPO_MAPPING:bad,worse worst**worst' - # Running Bazel again shouldn't crash. + # 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() { From e6076f5da29b849261d42069131a0b5f4ed0f03b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Wed, 21 Aug 2024 18:02:44 -0400 Subject: [PATCH 3/3] ignore empty lines in the marker file --- .../lib/rules/repository/RepositoryDelegatorFunction.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index 46554761d197ad..0c8a22c3e08469 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -703,6 +703,9 @@ private static String readMarkerFile( boolean firstLine = true; for (String line : lines) { + if (line.isEmpty()) { + continue; + } if (firstLine) { markerRuleKey = line; firstLine = false;