From d62e0a0f32188e1875bb8e62ef4377ea4dc1aab2 Mon Sep 17 00:00:00 2001 From: Xdng Yng Date: Thu, 22 Aug 2024 11:02:31 -0700 Subject: [PATCH] Consider invalid entries in the marker file reason for refetch 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 signal a refetch. Fixes https://github.com/bazelbuild/bazel/issues/23322. Closes #23336. PiperOrigin-RevId: 666416942 Change-Id: Ie5507654f69825d93f9523d9c209d417fb3cdaf6 --- .../rules/repository/RepoRecordedInput.java | 102 +++++++++++++++--- .../RepositoryDelegatorFunction.java | 12 ++- .../shell/bazel/starlark_repository_test.sh | 40 +++++++ 3 files changed, 134 insertions(+), 20 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..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 @@ -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,25 +77,32 @@ 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. */ 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 NEVER_UP_TO_DATE; + } for (Parser parser : new Parser[] { File.PARSER, Dirents.PARSER, DirTree.PARSER, EnvVar.PARSER, RecordedRepoMapping.PARSER @@ -103,7 +111,7 @@ public static RepoRecordedInput parse(String s) { return parser.parse(parts.get(1)); } } - return null; + return NEVER_UP_TO_DATE; } /** @@ -172,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 @@ -213,12 +257,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)); @@ -264,7 +308,12 @@ public String getPrefix() { @Override public RepoRecordedInput parse(String s) { - return new File(RepoCacheFriendlyPath.parse(s)); + try { + return new File(RepoCacheFriendlyPath.parse(s)); + } catch (LabelSyntaxException e) { + // malformed inputs cause refetch + return NEVER_UP_TO_DATE; + } } }; @@ -356,7 +405,12 @@ public String getPrefix() { @Override public RepoRecordedInput parse(String s) { - return new Dirents(RepoCacheFriendlyPath.parse(s)); + try { + return new Dirents(RepoCacheFriendlyPath.parse(s)); + } catch (LabelSyntaxException e) { + // malformed inputs cause refetch + return NEVER_UP_TO_DATE; + } } }; @@ -439,7 +493,12 @@ public String getPrefix() { @Override public RepoRecordedInput parse(String s) { - return new DirTree(RepoCacheFriendlyPath.parse(s)); + try { + return new DirTree(RepoCacheFriendlyPath.parse(s)); + } catch (LabelSyntaxException e) { + // malformed inputs cause refetch + return NEVER_UP_TO_DATE; + } } }; @@ -576,8 +635,12 @@ public String getPrefix() { @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) { + // malformed inputs cause refetch + return NEVER_UP_TO_DATE; + } } }; @@ -632,9 +695,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) { + // 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..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; @@ -710,12 +713,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 e11e457b30a104..baf767932016ee 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2864,6 +2864,46 @@ 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} + + # 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() { create_new_workspace cat > $(setup_module_dot_bazel) <