diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java index 5c7b8de8a1d6c7..812fe8f27b7e08 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java @@ -91,7 +91,7 @@ protected ModuleExtensionContext( this.rootModuleHasNonDevDependency = rootModuleHasNonDevDependency; // Record inputs to the extension that are known prior to evaluation. RepoRecordedInput.EnvVar.wrap(staticEnvVars) - .forEach((input, value) -> recordInput(input, value.orElse(null))); + .forEach((input, value) -> recordInputWithValue(input, value.orElse(null))); repoMappingRecorder.record(staticRepoMappingEntries); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index c83afc25132cf3..cd9e163f8d8f9d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -395,12 +395,18 @@ private static Optional didRecordedInputsChange( BlazeDirectories directories, List recordedInputs) throws InterruptedException, NeedsSkyframeRestartException { - Optional outdated = - RepoRecordedInput.isAnyValueOutdated(env, directories, recordedInputs); - if (env.valuesMissing()) { - throw new NeedsSkyframeRestartException(); + // Check inputs in batches to prevent Skyframe cycles caused by outdated dependencies. + for (ImmutableList batch : + RepoRecordedInput.WithValue.splitIntoBatches(recordedInputs)) { + Optional outdated = RepoRecordedInput.isAnyValueOutdated(env, directories, batch); + if (env.valuesMissing()) { + throw new NeedsSkyframeRestartException(); + } + if (outdated.isPresent()) { + return outdated; + } } - return outdated; + return Optional.empty(); } private SingleExtensionValue createSingleExtensionValue( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java index 89af2cde2a1f14..df98556b784b45 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/cache/LocalRepoContentsCache.java @@ -154,9 +154,9 @@ private Path ensureTrashDir() throws IOException { /** * Moves a freshly fetched repo into the contents cache. * - * @return the repo dir in the contents cache. + * @return the new cache entry */ - public Path moveToCache( + public CandidateRepo moveToCache( Path fetchedRepoDir, Path fetchedRepoMarkerFile, String predeclaredInputHash) throws IOException { Preconditions.checkState(path != null); @@ -183,7 +183,7 @@ public Path moveToCache( // Set up a symlink at the original fetched repo dir path. fetchedRepoDir.deleteTree(); FileSystemUtils.ensureSymbolicLink(fetchedRepoDir, cacheRepoDir); - return cacheRepoDir; + return new CandidateRepo(cacheRecordedInputsFile, cacheRepoDir); } public void acquireSharedLock() throws IOException, InterruptedException { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 5cf3e3069fcf0d..cb7893673dd8b1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -23,13 +23,10 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.flogger.GoogleLogger; import com.google.common.util.concurrent.Futures; -import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.debug.WorkspaceRuleEvent; import com.google.devtools.build.lib.bazel.repository.DecompressorDescriptor; @@ -55,6 +52,7 @@ import com.google.devtools.build.lib.remote.RemoteExternalOverlayFileSystem; import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; +import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.MaybeValue; import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.RepoCacheFriendlyPath; import com.google.devtools.build.lib.rules.repository.RepositoryFunction; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; @@ -71,6 +69,7 @@ import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.ForOverride; import java.io.File; import java.io.IOException; @@ -212,7 +211,7 @@ protected StarlarkBaseExternalContext( // apparent repo names to canonical repo names. See #20721 for why this is necessary. this.repoMappingRecorder = (fromRepo, apparentRepoName, canonicalRepoName) -> - recordInput( + recordInputWithValue( new RepoRecordedInput.RecordedRepoMapping(fromRepo, apparentRepoName), canonicalRepoName.isVisible() ? canonicalRepoName.getName() : null); } @@ -251,7 +250,7 @@ public void storeRepoMappingRecorderInThread(StarlarkThread thread) { repoMappingRecorder.storeInThread(thread); } - protected void recordInput(RepoRecordedInput input, @Nullable String value) { + protected void recordInputWithValue(RepoRecordedInput input, @Nullable String value) { if (recordedInputs.containsKey(input) && !Objects.equals(recordedInputs.get(input), value)) { throw new IllegalStateException( "Conflicting values recorded for input %s: '%s' vs. '%s'" @@ -260,6 +259,23 @@ protected void recordInput(RepoRecordedInput input, @Nullable String value) { recordedInputs.put(input, value); } + @CanIgnoreReturnValue + @Nullable + protected String getValueAndRecordInput(RepoRecordedInput input) + throws InterruptedException, NeedsSkyframeRestartException, IOException { + var maybeValue = input.getValue(env, directories); + if (env.valuesMissing()) { + throw new NeedsSkyframeRestartException(); + } + return switch (maybeValue) { + case MaybeValue.Invalid(String reason) -> throw new IOException(reason); + case MaybeValue.Valid(String value) -> { + recordInputWithValue(input, value); + yield value; + } + }; + } + private boolean cancelPendingAsyncTasks() { boolean hadPendingItems = false; for (AsyncTask task : asyncTasks) { @@ -1489,18 +1505,12 @@ private static T nullIfNone(Object object, Class type) { @Nullable public String getEnvironmentValue(String name, Object defaultValue) throws InterruptedException, NeedsSkyframeRestartException { - // Must look up via Skyframe, rather than solely copy from `this.repoEnvVariables`, in order to - // establish a SkyKey dependency relationship. - var nameAndValue = RepositoryFunction.getEnvVarValues(env, ImmutableSet.of(name)); - if (nameAndValue == null) { - return null; + try { + String value = getValueAndRecordInput(new RepoRecordedInput.EnvVar(name)); + return value != null ? value : nullIfNone(defaultValue, String.class); + } catch (IOException e) { + throw new IllegalStateException("getting EnvVar never throws IOException", e); } - // However, to account for --repo_env we take the value from `this.repoEnvVariables`. - // See https://github.com/bazelbuild/bazel/pull/20787#discussion_r1445571248 . - var entry = Iterables.getOnlyElement(RepoRecordedInput.EnvVar.wrap(nameAndValue).entrySet()); - String envVarValue = repoEnvVariables.get(name); - recordInput(entry.getKey(), envVarValue); - return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class); } @StarlarkMethod( @@ -1661,17 +1671,8 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch) if (repoCacheFriendlyPath == null) { return; } - var recordedInput = new RepoRecordedInput.File(repoCacheFriendlyPath); - var skyKey = recordedInput.getSkyKey(directories); try { - FileValue fileValue = (FileValue) env.getValueOrThrow(skyKey, IOException.class); - if (fileValue == null) { - throw new NeedsSkyframeRestartException(); - } - - recordInput( - recordedInput, - RepoRecordedInput.File.fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue)); + getValueAndRecordInput(new RepoRecordedInput.File(repoCacheFriendlyPath)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } @@ -1683,12 +1684,8 @@ protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch) if (repoCacheFriendlyPath == null) { return; } - var recordedInput = new RepoRecordedInput.Dirents(repoCacheFriendlyPath); - if (env.getValue(recordedInput.getSkyKey(directories)) == null) { - throw new NeedsSkyframeRestartException(); - } try { - recordInput(recordedInput, RepoRecordedInput.Dirents.getDirentsMarkerValue(path)); + getValueAndRecordInput(new RepoRecordedInput.Dirents(repoCacheFriendlyPath)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 751d8305e94a58..6cb1a331464eaf 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -42,7 +42,6 @@ import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper; import com.google.devtools.build.lib.runtime.ProcessWrapper; import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor; -import com.google.devtools.build.lib.skyframe.DirectoryTreeDigestValue; import com.google.devtools.build.lib.util.StringUtilities; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; @@ -587,15 +586,7 @@ public void watchTree(Object path) return; } try { - var recordedInput = new RepoRecordedInput.DirTree(repoCacheFriendlyPath); - DirectoryTreeDigestValue digestValue = - (DirectoryTreeDigestValue) - env.getValueOrThrow(recordedInput.getSkyKey(directories), IOException.class); - if (digestValue == null) { - throw new NeedsSkyframeRestartException(); - } - - recordInput(recordedInput, digestValue.hexDigest()); + getValueAndRecordInput(new RepoRecordedInput.DirTree(repoCacheFriendlyPath)); } catch (IOException e) { throw new RepositoryFunctionException(e, Transience.TRANSIENT); } 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 39a5b43e444dae..b3532d7ff919b9 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 @@ -23,6 +23,8 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; +import com.google.common.collect.Collections2; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.FileValue; @@ -31,20 +33,22 @@ 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.DirectoryListingValue; import com.google.devtools.build.lib.skyframe.EnvironmentVariableValue; import com.google.devtools.build.lib.skyframe.RepoEnvironmentFunction; -import com.google.devtools.build.lib.skyframe.DirectoryListingValue; import com.google.devtools.build.lib.skyframe.DirectoryTreeDigestValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; import com.google.devtools.build.lib.util.Fingerprint; -import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyKey; import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -121,60 +125,45 @@ public static Optional parse(String s) { return Optional.empty(); } + /** + * Splits the given list of recorded input values into batches such that within each batch, all + * recorded inputs's {@link SkyKey}s can be requested together. + */ + public static ImmutableList> splitIntoBatches( + List recordedInputValues) { + var batches = ImmutableList.>builder(); + var currentBatch = new ArrayList(); + for (var recordedInputValue : recordedInputValues) { + if (!recordedInputValue.input().canBeRequestedUnconditionally() + && !currentBatch.isEmpty()) { + batches.add(ImmutableList.copyOf(currentBatch)); + currentBatch.clear(); + } + currentBatch.add(recordedInputValue); + } + if (!currentBatch.isEmpty()) { + batches.add(ImmutableList.copyOf(currentBatch)); + } + return batches.build(); + } + /** Converts this {@link WithValue} to a string in a format compatible with {@link #parse}. */ @Override public String toString() { - return escape(input.toString()) + " " + escape(value); - } - - @VisibleForTesting - static String escape(String str) { - return str == null - ? "\\0" - : str.replace("\\", "\\\\").replace("\n", "\\n").replace(" ", "\\s"); - } - - @VisibleForTesting - @Nullable - static String unescape(String str) { - if (str.equals("\\0")) { - return null; // \0 == null string - } - StringBuilder result = new StringBuilder(); - boolean escaped = false; - for (int i = 0; i < str.length(); i++) { - char c = str.charAt(i); - if (escaped) { - if (c == 'n') { // n means new line - result.append("\n"); - } else if (c == 's') { // s means space - result.append(" "); - } else { // Any other escaped characters are just un-escaped - result.append(c); - } - escaped = false; - } else if (c == '\\') { - escaped = true; - } else { - result.append(c); - } - } - return result.toString(); + return input + " " + escape(value); } } /** - * Returns whether all values are still up-to-date for each recorded input. If Skyframe values are - * missing, the return value should be ignored; callers are responsible for checking {@code - * env.valuesMissing()} and triggering a Skyframe restart if needed. + * Returns whether all values are still up-to-date for each recorded input or a human-readable + * reason for why that's not the case. If Skyframe values are missing, the return value should be + * ignored; callers are responsible for checking {@code env.valuesMissing()} and triggering a + * Skyframe restart if needed. */ public static Optional isAnyValueOutdated( Environment env, BlazeDirectories directories, List recordedInputValues) throws InterruptedException { - env.getValuesAndExceptions( - recordedInputValues.stream() - .map(riv -> riv.input().getSkyKey(directories)) - .collect(toImmutableSet())); + prefetch(env, directories, Collections2.transform(recordedInputValues, WithValue::input)); if (env.valuesMissing()) { return UNDECIDED; } @@ -188,40 +177,146 @@ public static Optional isAnyValueOutdated( return Optional.empty(); } + /** + * Requests the information from Skyframe that is required by future calls to {@link + * #isAnyValueOutdated} for the given set of inputs. + */ + public static void prefetch( + Environment env, BlazeDirectories directories, Collection recordedInputs) + throws InterruptedException { + var keys = + recordedInputs.stream().map(rri -> rri.getSkyKey(directories)).collect(toImmutableSet()); + if (env.valuesMissing()) { + return; + } + var results = env.getValuesAndExceptions(keys); + // Per the contract of Environment.getValuesAndExceptions, we need to access the results to + // actually find all missing values. + for (SkyKey key : keys) { + var unused = results.get(key); + } + } + + /** + * Returns a human-readable reason for why the given {@code oldValue} is no longer up-to-date for + * this recorded input, or an empty Optional if it is still up-to-date. + * + *

The method can request Skyframe evaluations, and if any values are missing, this method can + * return any value (doesn't matter what, although {@link #UNDECIDED} is recommended for clarity) + * and will be reinvoked after a Skyframe restart. + */ + private Optional isOutdated( + Environment env, BlazeDirectories directories, @Nullable String oldValue) + throws InterruptedException { + MaybeValue wrappedNewValue = getValue(env, directories); + if (env.valuesMissing()) { + return UNDECIDED; + } + return switch (wrappedNewValue) { + case MaybeValue.Invalid(String reason) -> Optional.of(reason); + case MaybeValue.Valid(String newValue) -> + Objects.equals(oldValue, newValue) + ? Optional.empty() + : Optional.of(describeChange(oldValue, newValue)); + }; + } + @Override public abstract boolean equals(Object obj); @Override public abstract int hashCode(); + @VisibleForTesting + static String escape(String str) { + return str == null ? "\\0" : str.replace("\\", "\\\\").replace("\n", "\\n").replace(" ", "\\s"); + } + + @VisibleForTesting + @Nullable + static String unescape(String str) { + if (str.equals("\\0")) { + return null; // \0 == null string + } + StringBuilder result = new StringBuilder(); + boolean escaped = false; + for (int i = 0; i < str.length(); i++) { + char c = str.charAt(i); + if (escaped) { + if (c == 'n') { // n means new line + result.append("\n"); + } else if (c == 's') { // s means space + result.append(" "); + } else { // Any other escaped characters are just un-escaped + result.append(c); + } + escaped = false; + } else if (c == '\\') { + escaped = true; + } else { + result.append(c); + } + } + return result.toString(); + } + + /** + * Returns the string representation of this recorded input, in the format suitable for parsing + * back via {@link #parse}. + * + *

The returned string never contains spaces or newlines; those characters are escaped. + */ @Override public final String toString() { - return getParser().getPrefix() + ":" + toStringInternal(); + return getParser().getPrefix() + ":" + escape(toStringInternal()); } + /** + * Represents the possible values returned by {@link #getValue}: either a valid value (which may + * be null), or an invalid value with a reason (e.g. due to I/O failure). + */ + public sealed interface MaybeValue { + MaybeValue VALUES_MISSING = new MaybeValue.Invalid("values missing"); + + /** Represents a valid value, which may be null. */ + record Valid(@Nullable String value) implements MaybeValue {} + + /** Represents an invalid value with a reason (e.g. due to I/O failure). */ + record Invalid(String reason) implements MaybeValue {} + } + + /** + * Returns the current value of this input, which may be null, wrapped in a {@link + * MaybeValue.Valid}, or a {@link MaybeValue.Invalid} if the value is known to be invalid. + * + *

The method can request Skyframe evaluations, and if any values are missing, this method can + * return any value and will be reinvoked after a Skyframe restart. + */ + public abstract MaybeValue getValue(Environment env, BlazeDirectories directories) + throws InterruptedException; + + /** + * Returns a human-readable description of the change from {@code oldValue} to {@code newValue}. + */ + protected abstract String describeChange(String oldValue, String newValue); + /** * Returns the post-colon substring that identifies the specific input: for example, the {@code * MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. */ - public abstract String toStringInternal(); + protected abstract String toStringInternal(); /** Returns the parser object for this type of recorded inputs. */ - public abstract Parser getParser(); + protected abstract Parser getParser(); /** Returns the {@link SkyKey} that is necessary to determine {@link #isOutdated}. */ - public abstract SkyKey getSkyKey(BlazeDirectories directories); + protected abstract SkyKey getSkyKey(BlazeDirectories directories); /** - * Returns a human-readable reason for why the given {@code oldValue} is no longer up-to-date for - * this recorded input, or an empty Optional if it is still up-to-date. This method can assume - * that {@link #getSkyKey(BlazeDirectories)} is already evaluated; it can request further Skyframe - * evaluations, and if any values are missing, this method can return any value (doesn't matter - * what, although {@link #UNDECIDED} is recommended for clarity) and will be reinvoked after a - * Skyframe restart. + * Returns true if the {@link #getValue} can be requested even if previous recorded inputs have + * not been verified to be up to date. */ - public abstract Optional isOutdated( - Environment env, BlazeDirectories directories, @Nullable String oldValue) - throws InterruptedException; + protected abstract boolean canBeRequestedUnconditionally(); private static final Optional UNDECIDED = Optional.of("values missing"); @@ -299,6 +394,11 @@ public final RootedPath getRootedPath(BlazeDirectories directories) { } return RootedPath.toRootedPath(root, path()); } + + /** Returns true if the path points into an external repository. */ + public boolean inExternalRepo() { + return repoName().isPresent() && !repoName().get().isMain(); + } } /** @@ -363,7 +463,8 @@ public String toStringInternal() { * for placing in a repository marker file. The file need not exist, and can be a file or a * directory. */ - public static String fileValueToMarkerValue(RootedPath rootedPath, FileValue fileValue) + @VisibleForTesting + static String fileValueToMarkerValue(RootedPath rootedPath, FileValue fileValue) throws IOException { if (fileValue.isDirectory()) { return "DIR"; @@ -386,23 +487,32 @@ public SkyKey getSkyKey(BlazeDirectories directories) { } @Override - public Optional isOutdated( - Environment env, BlazeDirectories directories, @Nullable String oldValue) + protected boolean canBeRequestedUnconditionally() { + // Requesting files in external repositories can result in cycles if the external repo now + // transitively depends on the requesting repo. + return !path.inExternalRepo(); + } + + @Override + public MaybeValue getValue(Environment env, BlazeDirectories directories) throws InterruptedException { var skyKey = getSkyKey(directories); try { - FileValue fileValue = (FileValue) env.getValueOrThrow(skyKey, IOException.class); + var fileValue = (FileValue) env.getValueOrThrow(skyKey, IOException.class); if (fileValue == null) { - return UNDECIDED; - } - if (!oldValue.equals(fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue))) { - return Optional.of("file info or contents of %s changed".formatted(path)); + return MaybeValue.VALUES_MISSING; } - return Optional.empty(); + return new MaybeValue.Valid( + fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue)); } catch (IOException e) { - return Optional.of("failed to stat %s: %s".formatted(path, e.getMessage())); + return new MaybeValue.Invalid("failed to stat %s: %s".formatted(path, e.getMessage())); } } + + @Override + public String describeChange(String oldValue, String newValue) { + return "file info or contents of %s changed".formatted(path); + } } /** Represents the list of entries under a directory accessed during the fetch. */ @@ -457,38 +567,42 @@ public Parser getParser() { return PARSER; } + private String directoryListingValueToMarkerValue(DirectoryListingValue directoryListingValue) { + var fp = new Fingerprint(); + fp.addStrings( + com.google.common.collect.Streams.stream(directoryListingValue.getDirents()) + .map(Dirent::getName) + .sorted() + .collect(toImmutableList())); + return fp.hexDigestAndReset(); + } + @Override public SkyKey getSkyKey(BlazeDirectories directories) { return DirectoryListingValue.key(path.getRootedPath(directories)); } @Override - public Optional isOutdated( - Environment env, BlazeDirectories directories, @Nullable String oldValue) + protected boolean canBeRequestedUnconditionally() { + // Requesting directories in external repositories can result in cycles if the external repo + // transitively depends on the requesting repo. + return !path.inExternalRepo(); + } + + @Override + public MaybeValue getValue(Environment env, BlazeDirectories directories) throws InterruptedException { - SkyKey skyKey = getSkyKey(directories); - if (env.getValue(skyKey) == null) { - return UNDECIDED; - } - try { - if (!oldValue.equals( - getDirentsMarkerValue(((DirectoryListingValue.Key) skyKey).argument().asPath()))) { - return Optional.of("directory entries of %s changed".formatted(path)); - } - return Optional.empty(); - } catch (IOException e) { - return Optional.of("failed to readdir %s: %s".formatted(path, e.getMessage())); + var skyKey = getSkyKey(directories); + var directoryListingValue = (DirectoryListingValue) env.getValue(skyKey); + if (directoryListingValue == null) { + return MaybeValue.VALUES_MISSING; } + return new MaybeValue.Valid(directoryListingValueToMarkerValue(directoryListingValue)); } - public static String getDirentsMarkerValue(Path path) throws IOException { - Fingerprint fp = new Fingerprint(); - fp.addStrings( - path.getDirectoryEntries().stream() - .map(Path::getBaseName) - .sorted() - .collect(toImmutableList())); - return fp.hexDigestAndReset(); + @Override + public String describeChange(String oldValue, String newValue) { + return "directory entries of %s changed".formatted(path); } } @@ -554,18 +668,32 @@ public SkyKey getSkyKey(BlazeDirectories directories) { } @Override - public Optional isOutdated( - Environment env, BlazeDirectories directories, @Nullable String oldValue) + protected boolean canBeRequestedUnconditionally() { + // Requesting directory trees in external repositories can result in cycles if the external + // repo now transitively depends on the requesting repo. + return !path.inExternalRepo(); + } + + @Override + public MaybeValue getValue(Environment env, BlazeDirectories directories) throws InterruptedException { - DirectoryTreeDigestValue value = - (DirectoryTreeDigestValue) env.getValue(getSkyKey(directories)); - if (value == null) { - return UNDECIDED; - } - if (!oldValue.equals(value.hexDigest())) { - return Optional.of("directory tree at %s changed".formatted(path)); + var skyKey = getSkyKey(directories); + try { + var directoryTreeDigestValue = + (DirectoryTreeDigestValue) env.getValueOrThrow(skyKey, IOException.class); + if (directoryTreeDigestValue == null) { + return MaybeValue.VALUES_MISSING; + } + return new MaybeValue.Valid(directoryTreeDigestValue.hexDigest()); + } catch (IOException e) { + return new MaybeValue.Invalid( + "failed to digest directory tree at %s: %s".formatted(path, e.getMessage())); } - return Optional.empty(); + } + + @Override + public String describeChange(String oldValue, String newValue) { + return "directory tree at %s changed".formatted(path); } } @@ -593,7 +721,7 @@ public static ImmutableMap> wrap( .collect(toImmutableMap(e -> new EnvVar(e.getKey()), Map.Entry::getValue)); } - private EnvVar(String name) { + public EnvVar(String name) { this.name = name; } @@ -629,24 +757,29 @@ public SkyKey getSkyKey(BlazeDirectories directories) { } @Override - public Optional isOutdated( - Environment env, BlazeDirectories directories, @Nullable String oldValue) + protected boolean canBeRequestedUnconditionally() { + // Environment variables are static data injected into Skyframe, so there is no risk of + // cycles. + return true; + } + + @Override + public MaybeValue getValue(Environment env, BlazeDirectories directories) throws InterruptedException { - var envValue = (EnvironmentVariableValue) env.getValue(getSkyKey(directories)); - if (envValue == null) { - return Optional.empty(); - } - String v = envValue.value(); - // Note that `oldValue` can be null if the env var was not set. - if (!Objects.equals(oldValue, v)) { - return Optional.of( - "environment variable %s changed: %s -> %s" - .formatted( - name, - oldValue == null ? "" : "'%s'".formatted(oldValue), - v == null ? "" : "'%s'".formatted(v))); + var value = (EnvironmentVariableValue) env.getValue(getSkyKey(directories)); + if (value == null) { + return MaybeValue.VALUES_MISSING; } - return Optional.empty(); + return new MaybeValue.Valid(value.value()); + } + + @Override + public String describeChange(String oldValue, String newValue) { + return "environment variable %s changed: %s -> %s" + .formatted( + name, + oldValue == null ? "" : "'%s'".formatted(oldValue), + newValue == null ? "" : "'%s'".formatted(newValue)); } } @@ -721,32 +854,32 @@ public SkyKey getSkyKey(BlazeDirectories directories) { } @Override - public Optional isOutdated( - Environment env, BlazeDirectories directories, @Nullable String oldValue) + protected boolean canBeRequestedUnconditionally() { + // Starlark can only request the mapping of the repo it is currently executing from, which + // means that the repo has already been fetched (either to execute the code or to verify the + // transitive .bzl hash). Further cycles aren't possible. + return true; + } + + @Override + public MaybeValue getValue(Environment env, BlazeDirectories directories) throws InterruptedException { - RepositoryMappingValue repoMappingValue = - (RepositoryMappingValue) env.getValue(getSkyKey(directories)); + var repoMappingValue = (RepositoryMappingValue) env.getValue(getSkyKey(directories)); if (Objects.equals(repoMappingValue, RepositoryMappingValue.NOT_FOUND_VALUE)) { - return Optional.of("source repo %s doesn't exist anymore".formatted(sourceRepo)); - } - RepositoryName oldCanonicalName; - try { - oldCanonicalName = RepositoryName.create(oldValue); - } catch (LabelSyntaxException e) { - // malformed old value causes refetch - return Optional.of("invalid recorded repo name: %s".formatted(e.getMessage())); - } - RepositoryName newCanonicalName = repoMappingValue.repositoryMapping().get(apparentName); - if (!oldCanonicalName.equals(newCanonicalName)) { - return Optional.of( - "canonical name for @%s in %s changed: %s -> %s" - .formatted( - apparentName, - sourceRepo, - oldCanonicalName, - newCanonicalName == null ? "" : newCanonicalName)); + return new MaybeValue.Invalid("source repo %s doesn't exist anymore".formatted(sourceRepo)); } - return Optional.empty(); + RepositoryName canonicalName = repoMappingValue.repositoryMapping().get(apparentName); + return new MaybeValue.Valid(canonicalName != null ? canonicalName.getName() : null); + } + + @Override + public String describeChange(String oldValue, String newValue) { + return "canonical name for @%s in %s changed: %s -> %s" + .formatted( + apparentName, + sourceRepo, + oldValue == null ? "" : oldValue, + newValue == null ? "" : newValue); } } @@ -789,9 +922,19 @@ public SkyKey getSkyKey(BlazeDirectories directories) { } @Override - public Optional isOutdated( - Environment env, BlazeDirectories directories, @Nullable String oldValue) { - return Optional.of(reason); + protected boolean canBeRequestedUnconditionally() { + return true; + } + + @Override + public MaybeValue getValue(Environment env, BlazeDirectories directories) { + return new MaybeValue.Invalid(reason); + } + + @Override + public String describeChange(String oldValue, String newValue) { + throw new UnsupportedOperationException( + "the value for this sentinel input is always invalid"); } } } 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 1a2a27ef7e3a5f..878429eb7252d4 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 @@ -308,9 +308,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) } if (repoContentsCache.isEnabled()) { // This repo is eligible for the repo contents cache. - Path cachedRepoDir; + CandidateRepo newCacheEntry; try { - cachedRepoDir = + newCacheEntry = repoContentsCache.moveToCache( repoRoot, digestWriter.markerPath, digestWriter.predeclaredInputHash); } catch (IOException e) { @@ -321,6 +321,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) e), Transience.TRANSIENT); } + Path cachedRepoDir = newCacheEntry.contentsDir(); // Don't forget to register a FileStateValue on the cache repo dir, so that we know to // refetch if the cache entry gets GC'd from under us or the entire cache is deleted. // diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index 4471325f4649ac..4d6657c9178a43 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java @@ -239,12 +239,23 @@ public Optional isAnyRecordedInputOutdated( Map recordedInputValues, Environment env) throws InterruptedException { - return RepoRecordedInput.isAnyValueOutdated( - env, - directories, + var withValues = recordedInputValues.entrySet().stream() .map(e -> new RepoRecordedInput.WithValue(e.getKey(), e.getValue())) - .collect(com.google.common.collect.ImmutableList.toImmutableList())); + .collect(com.google.common.collect.ImmutableList.toImmutableList()); + // Check inputs in batches to prevent Skyframe cycles caused by outdated dependencies: a batch + // ends right before any input that may cause a cycle if requested while earlier inputs are out + // of date. A later batch's deps are only requested after the earlier batches have been + // confirmed up to date across Skyframe restarts (isAnyValueOutdated returns a non-empty reason + // when its batch's values are still missing, short-circuiting this loop). + for (var batch : RepoRecordedInput.WithValue.splitIntoBatches(withValues)) { + Optional outdatedReason = + RepoRecordedInput.isAnyValueOutdated(env, directories, batch); + if (outdatedReason.isPresent()) { + return outdatedReason; + } + } + return Optional.empty(); } public static RootedPath getRootedPathFromLabel(Label label, Environment env) diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInputTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInputTest.java index dae6be132a65a1..ea928c53d6115f 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInputTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInputTest.java @@ -15,8 +15,11 @@ package com.google.devtools.build.lib.rules.repository; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.rules.repository.RepoRecordedInput.WithValue.parse; +import static com.google.devtools.build.lib.rules.repository.RepoRecordedInput.WithValue.splitIntoBatches; import static org.mockito.Mockito.when; +import com.google.common.collect.ImmutableList; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.FileContentsProxy; import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValueWithContentsProxy; @@ -35,8 +38,8 @@ @RunWith(JUnit4.class) public class RepoRecordedInputTest extends BuildViewTestCase { private static void assertMarkerFileEscaping(String testCase) { - String escaped = RepoRecordedInput.WithValue.escape(testCase); - assertThat(RepoRecordedInput.WithValue.unescape(escaped)).isEqualTo(testCase); + String escaped = RepoRecordedInput.escape(testCase); + assertThat(RepoRecordedInput.unescape(escaped)).isEqualTo(testCase); } @Test @@ -70,4 +73,22 @@ public void testFileValueToMarkerValue() throws Exception { String expectedDigest = BaseEncoding.base16().lowerCase().encode(path.asPath().getDigest()); assertThat(RepoRecordedInput.File.fileValueToMarkerValue(path, fv)).isEqualTo(expectedDigest); } + + @Test + public void testSplitIntoBatches() { + assertThat(splitIntoBatches(ImmutableList.of())).isEmpty(); + assertThat( + splitIntoBatches( + ImmutableList.of( + parse("FILE:@@//foo:bar abc").orElseThrow(), + parse("FILE:@@//:baz cba").orElseThrow(), + parse("FILE:@@foo//:baz bac").orElseThrow(), + parse("ENV:KEY value").orElseThrow()))) + .containsExactly( + ImmutableList.of( + parse("FILE:@@//foo:bar abc").orElseThrow(), + parse("FILE:@@//:baz cba").orElseThrow()), + ImmutableList.of( + parse("FILE:@@foo//:baz bac").orElseThrow(), parse("ENV:KEY value").orElseThrow())); + } } diff --git a/src/test/py/bazel/bzlmod/bazel_module_test.py b/src/test/py/bazel/bzlmod/bazel_module_test.py index 5d1e11037cc48d..9aef1ec859e7c6 100644 --- a/src/test/py/bazel/bzlmod/bazel_module_test.py +++ b/src/test/py/bazel/bzlmod/bazel_module_test.py @@ -1570,6 +1570,66 @@ def testInvalidRepoRuleReferencedByTargetDoesNotCrash(self): self.assertNotIn('FATAL', stderr) self.assertIn("compilation of module 'repo.bzl' failed", stderr) + def testReverseDependencyDirection(self): + # Set up two module extensions that retain their predeclared input hashes + # across two builds but still reverse their dependency direction. Depending + # on how repo cache candidates are checked, this could lead to a Skyframe + # cycle. + self.ScratchFile( + 'MODULE.bazel', + [ + 'foo_ext = use_extension("//:repo.bzl", "foo_ext")', + 'use_repo(foo_ext, "foo")', + 'bar_ext = use_extension("//:repo.bzl", "bar_ext")', + 'use_repo(bar_ext, "bar")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'repo.bzl', + [ + 'def _repo_impl(rctx):', + ' rctx.file("output.txt", rctx.attr.content)', + ' rctx.file("BUILD", "exports_files([\'output.txt\'])")', + ' print("JUST FETCHED: %s" % rctx.original_name)', + ' return rctx.repo_metadata(reproducible=True)', + 'repo = repository_rule(', + ' implementation = _repo_impl,', + ' attrs = {"content": attr.string()},', + ')', + 'def _foo_ext_impl(mctx):', + ' deps = mctx.read(Label("@//:foo_deps.txt")).splitlines()', + ' content = "foo"', + ' for dep in deps:', + ' if dep:', + ' content += " + " + mctx.read(Label(dep))', + ' repo(name = "foo", content = content)', + 'foo_ext = module_extension(implementation = _foo_ext_impl)', + 'def _bar_ext_impl(mctx):', + ' deps = mctx.read(Label("@//:bar_deps.txt")).splitlines()', + ' content = "bar"', + ' for dep in deps:', + ' if dep:', + ' content += " + " + mctx.read(Label(dep))', + ' repo(name = "bar", content = content)', + 'bar_ext = module_extension(implementation = _bar_ext_impl)', + ], + ) + + self.ScratchFile('foo_deps.txt', ['@bar//:output.txt']) + self.ScratchFile('bar_deps.txt') + + # First fetch: not cached + _, _, stderr = self.RunBazel(['build', '@foo//:output.txt']) + self.assertIn('JUST FETCHED: bar', '\n'.join(stderr)) + self.assertIn('JUST FETCHED: foo', '\n'.join(stderr)) + + # After expunging and reversing: cached + self.RunBazel(['clean', '--expunge']) + self.ScratchFile('foo_deps.txt') + self.ScratchFile('bar_deps.txt', ['@foo//:output.txt']) + self.RunBazel(['build', '@foo//:output.txt']) + if __name__ == '__main__': absltest.main() diff --git a/src/test/py/bazel/bzlmod/repo_contents_cache_test.py b/src/test/py/bazel/bzlmod/repo_contents_cache_test.py index 60ea22b616d088..274d3cff6fc575 100644 --- a/src/test/py/bazel/bzlmod/repo_contents_cache_test.py +++ b/src/test/py/bazel/bzlmod/repo_contents_cache_test.py @@ -480,12 +480,7 @@ def testReverseDependencyDirection(self): self.RunBazel(['clean', '--expunge']) self.ScratchFile('foo_deps.txt', ['']) self.ScratchFile('bar_deps.txt', ['@foo//:output.txt']) - exit_code, stdout, stderr = self.RunBazel( - ['build', '@foo//:output.txt'], allow_failure=True - ) - # TODO: b/xxxxxxx - This is NOT the intended behavior. - self.AssertNotExitCode(exit_code, 0, stderr, stdout) - self.assertIn('possible dependency cycle detected', '\n'.join(stderr)) + self.RunBazel(['build', '@foo//:output.txt']) def doTestRepoContentsCacheDeleted(self, check_external_repository_files): repo_contents_cache = self.ScratchDir('repo_contents_cache')