[8.8.0] Materialize important outputs from remote external repos

src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java

@@ -17,6 +17,7 @@
 
 import com.google.common.util.concurrent.ListenableFuture;
 import java.io.IOException;
+import javax.annotation.Nullable;
 
 /** Prefetches files to local disk. */
 public interface ActionInputPrefetcher {
@@ -34,7 +35,7 @@ public interface MetadataSupplier {
       new ActionInputPrefetcher() {
         @Override
         public ListenableFuture<Void> prefetchFiles(
-            ActionExecutionMetadata action,
+            @Nullable ActionExecutionMetadata action,
             Iterable<? extends ActionInput> inputs,
             MetadataSupplier metadataSupplier,
             Priority priority,
@@ -90,7 +91,7 @@ enum Reason {
    * @return future success if prefetch is finished or {@link IOException}.
    */
   ListenableFuture<Void> prefetchFiles(
-      ActionExecutionMetadata action,
+      @Nullable ActionExecutionMetadata action,
       Iterable<? extends ActionInput> inputs,
       MetadataSupplier metadataSupplier,
       Priority priority,

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);
   }
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java

@@ -395,12 +395,18 @@ private static Optional<String> didRecordedInputsChange(
       BlazeDirectories directories,
       List<RepoRecordedInput.WithValue> recordedInputs)
       throws InterruptedException, NeedsSkyframeRestartException {
-    Optional<String> 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<RepoRecordedInput.WithValue> batch :
+        RepoRecordedInput.WithValue.splitIntoBatches(recordedInputs)) {
+      Optional<String> 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(

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 {

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> T nullIfNone(Object object, Class<T> 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);
     }

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);
     }

src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java

@@ -329,7 +329,7 @@ protected void prefetchVirtualActionInput(VirtualActionInput input) throws IOExc
    */
   @Override
   public ListenableFuture<Void> prefetchFiles(
-      ActionExecutionMetadata action,
+      @Nullable ActionExecutionMetadata action,
       Iterable<? extends ActionInput> inputs,
       MetadataSupplier metadataSupplier,
       Priority priority,

src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java

@@ -190,24 +190,21 @@ private void addRunfiles(ProviderCollection buildTarget) {
     }
     var runfiles = runfilesSupport.getRunfiles();
     for (Artifact runfile : runfiles.getArtifacts().toList()) {
-      if (runfile.isSourceArtifact()) {
-        continue;
+      if (mayBeRemote(runfile)) {
+        addOutputToDownload(runfile);
       }
-      addOutputToDownload(runfile);
     }
     for (var symlink : runfiles.getSymlinks().toList()) {
       var artifact = symlink.getArtifact();
-      if (artifact.isSourceArtifact()) {
-        continue;
+      if (mayBeRemote(artifact)) {
+        addOutputToDownload(artifact);
       }
-      addOutputToDownload(artifact);
     }
     for (var symlink : runfiles.getRootSymlinks().toList()) {
       var artifact = symlink.getArtifact();
-      if (artifact.isSourceArtifact()) {
-        continue;
+      if (mayBeRemote(artifact)) {
+        addOutputToDownload(artifact);
       }
-      addOutputToDownload(artifact);
     }
   }
 
@@ -292,6 +289,19 @@ private boolean matchesPattern(PathFragment execPath) {
     return false;
   }
 
+  /**
+   * Returns whether this {@link ActionInput} could conceivably be only available remotely.
+   *
+   * <p>Use this as a quick check to avoid unnecessary extra work for artifacts that are definitely
+   * local.
+   */
+  public static boolean mayBeRemote(ActionInput actionInput) {
+    return !(actionInput instanceof Artifact artifact
+        && artifact.isSourceArtifact()
+        // Source artifacts in the main repo don't need to be fetched.
+        && (artifact.getOwner() == null || artifact.getOwner().getRepository().isMain()));
+  }
+
   /** Returns whether this {@link ActionInput} should be downloaded. */
   @Override
   public boolean shouldDownloadOutput(ActionInput output, RemoteFileArtifactValue metadata) {

src/main/java/com/google/devtools/build/lib/remote/RemoteRepoContentsCacheImpl.java

@@ -22,6 +22,7 @@
 import build.bazel.remote.execution.v2.Action;
 import build.bazel.remote.execution.v2.Command;
 import build.bazel.remote.execution.v2.Directory;
+import build.bazel.remote.execution.v2.Platform;
 import build.bazel.remote.execution.v2.Tree;
 import com.google.common.base.Splitter;
 import com.google.common.base.Throwables;
@@ -88,6 +89,7 @@ public final class RemoteRepoContentsCacheImpl implements RemoteRepoContentsCach
           .addOutputPaths(REPO_DIRECTORY_PATH)
           .addOutputFiles(MARKER_FILE_PATH)
           .addOutputDirectories(REPO_DIRECTORY_PATH)
+          .setPlatform(Platform.getDefaultInstance())
           .build();
   private static final Directory INPUT_ROOT = Directory.getDefaultInstance();
 
@@ -118,6 +120,7 @@ public RemoteRepoContentsCacheImpl(
         Action.newBuilder()
             .setCommandDigest(digestUtil.compute(COMMAND))
             .setInputRootDigest(digestUtil.compute(INPUT_ROOT))
+            .setPlatform(Platform.getDefaultInstance())
             .build();
   }