[8.8.0] Allow more general exceptions in getConfiguration

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

@@ -145,6 +145,11 @@ public void beforeCommand(
   }
 
   public void afterCommand() {
+    if (cache == null) {
+      // Not all commands cause beforeCommand to be called, but afterCommand is called
+      // unconditionally.
+      return;
+    }
     this.cache = null;
     this.inputPrefetcher = null;
     this.reporter = null;
@@ -270,6 +275,9 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter)
     var repoPath = externalDirectory.getChild(repo.getName());
     var remoteRepo = externalFs.getPath(repoPath);
     var walkResult = walk(remoteRepo);
+    for (var directory : walkResult.directories()) {
+      nativeFs.getPath(directory.asFragment()).createDirectory();
+    }
     var unused =
         getFromFuture(
             inputPrefetcher.prefetchFiles(
@@ -280,11 +288,10 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter)
                 ActionInputPrefetcher.Priority.CRITICAL,
                 ActionInputPrefetcher.Reason.INPUTS));
     // Create symlinks last as some platforms don't allow creating a symlink to a non-existent
-    // target.
+    // target. A symlink may have already been created as an input to an action.
     for (var remoteSymlink : walkResult.symlinks()) {
       var nativeSymlink = nativeFs.getPath(remoteSymlink.asFragment());
-      nativeSymlink.getParentDirectory().createDirectoryAndParents();
-      nativeSymlink.createSymbolicLink(remoteSymlink.readSymbolicLink());
+      FileSystemUtils.ensureSymbolicLink(nativeSymlink, remoteSymlink.readSymbolicLink());
     }
 
     // After the repo has been copied, atomically materialize the marker file. This ensures that the
@@ -297,10 +304,10 @@ private void doMaterialize(RepositoryName repo, ExtendedEventHandler reporter)
     markerFileSibling.renameTo(markerFile);
   }
 
-  private record WalkResult(List<Path> files, List<Path> symlinks) {}
+  private record WalkResult(List<Path> files, List<Path> symlinks, List<Path> directories) {}
 
   private static WalkResult walk(Path root) throws IOException {
-    var result = new WalkResult(new ArrayList<>(), new ArrayList<>());
+    var result = new WalkResult(new ArrayList<>(), new ArrayList<>(), new ArrayList<>());
     walk(root, result);
     return result;
   }
@@ -311,7 +318,10 @@ private static void walk(Path root, WalkResult result) throws IOException {
       switch (dirent.getType()) {
         case FILE -> result.files.add(fromChild);
         case SYMLINK -> result.symlinks.add(fromChild);
-        case DIRECTORY -> walk(fromChild, result);
+        case DIRECTORY -> {
+          result.directories.add(fromChild);
+          walk(fromChild, result);
+        }
         default -> throw new IOException("Unsupported file type: " + dirent);
       }
     }
@@ -515,7 +525,8 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks)
 
   @Override
   public Path resolveSymbolicLinks(PathFragment path) throws IOException {
-    return fsForPath(path).resolveSymbolicLinks(path);
+    // Ensure that the return value doesn't leave the overlay file system.
+    return getPath(fsForPath(path).resolveSymbolicLinks(path).asFragment());
   }
 
   @Nullable

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java

@@ -268,10 +268,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
           try {
             if (remoteRepoContentsCache.lookupCache(
                 repositoryName, repoRoot, digestWriter.predeclaredInputHash, env.getListener())) {
-              env.getListener()
-                  .handle(
-                      Event.debug(
-                          "Got %s from the remote repo contents cache".formatted(repositoryName)));
               return new RepositoryDirectoryValue.Success(
                   repoRoot, /* isFetchingDelayed= */ false, excludeRepoFromVendoring);
             }

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -1920,19 +1920,17 @@ public BuildConfigurationValue getConfiguration(
       Map.Entry<SkyKey, ErrorInfo> firstError = Iterables.get(evalResult.errorMap().entrySet(), 0);
       ErrorInfo error = firstError.getValue();
       Throwable e = error.getException();
-      // Wrap loading failed exceptions
-      if (e != null && e instanceof NoSuchThingException noSuchThingException) {
-        e = new InvalidConfigurationException(noSuchThingException.getDetailedExitCode(), e);
+      if (e instanceof InvalidConfigurationException invalidConfigurationException) {
+        throw invalidConfigurationException;
+      } else if (e instanceof DetailedException detailedException) {
+        throw new InvalidConfigurationException(detailedException.getDetailedExitCode(), e);
       } else if (e == null && !error.getCycleInfo().isEmpty()) {
         cyclesReporter.reportCycles(error.getCycleInfo(), firstError.getKey(), eventHandler);
-        e =
-            new InvalidConfigurationException(
+        throw new InvalidConfigurationException(
                 "cannot load build configuration because of this cycle", Code.CYCLE);
       }
-      if (e != null) {
-        Throwables.throwIfInstanceOf(e, InvalidConfigurationException.class);
-      }
-      throw new IllegalStateException("Unknown error during configuration creation evaluation", e);
+      throw new IllegalStateException(
+          "Unknown error during configuration creation evaluation", e);
     }
 
     // Prepare and return the results.

src/main/java/com/google/devtools/build/lib/vfs/Path.java

@@ -948,7 +948,8 @@ public void prefetchPackageAsync(int maxDirs) {
   private void checkSameFileSystem(Path that) {
     if (this.fileSystem != that.fileSystem) {
       throw new IllegalArgumentException(
-          "Files are on different filesystems: " + this + ", " + that);
+          "Files are on different filesystems: %s (on %s), %s (on %s)"
+              .formatted(this, this.fileSystem, that, that.fileSystem));
     }
   }
 }

src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py

@@ -314,6 +314,10 @@ def testAccessFromOtherRepo_read(self):
         [
             'def _repo_impl(rctx):',
             '  rctx.file("BUILD", "filegroup(name=\'haha\')")',
+            # Verify that directories are materialized correctly.
+            '  rctx.file("subdir/file.txt", "hello")',
+            '  rctx.file("subdir/empty_dir/.keep")',
+            '  rctx.delete("subdir/empty_dir/.keep")',
             '  print("JUST FETCHED")',
             '  return rctx.repo_metadata(reproducible=True)',
             'repo = repository_rule(_repo_impl)',
@@ -345,12 +349,17 @@ def testAccessFromOtherRepo_read(self):
     _, _, stderr = self.RunBazel(['build', '@my_repo//:haha'])
     self.assertNotIn('JUST FETCHED', '\n'.join(stderr))
     self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD')))
+    self.assertFalse(os.path.exists(os.path.join(repo_dir, 'subdir')))
 
     # Fetch other: my_repo materialized
     _, _, stderr = self.RunBazel(['build', '@other//:haha'])
     self.assertNotIn('JUST FETCHED', '\n'.join(stderr))
     self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD')))
     self.assertTrue(os.path.exists(os.path.join(other_repo_dir, 'BUILD')))
+    self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/file.txt')))
+    with open(os.path.join(repo_dir, 'subdir/file.txt')) as f:
+      self.assertEqual(f.read(), 'hello')
+    self.assertTrue(os.path.exists(os.path.join(repo_dir, 'subdir/empty_dir')))
 
     # Materialized repo is not refetched after a shutdown
     self.RunBazel(['shutdown'])
@@ -473,7 +482,11 @@ def testUseRepoFileInBuildRule_actionUsesCache(self):
     with open(self.Path('bazel-bin/main/out.txt')) as f:
       self.assertEqual(f.read(), 'hello')
 
-  def testUseRepoFileInBuildRule_actionDoesNotUseCache(self):
+  def do_testUseRepoFileInBuildRule_actionDoesNotUseCache(
+      self, extra_flags=None
+  ):
+    if extra_flags is None:
+      extra_flags = []
     self.ScratchFile(
         'MODULE.bazel',
         [
@@ -509,7 +522,7 @@ def testUseRepoFileInBuildRule_actionDoesNotUseCache(self):
     repo_dir = self.RepoDir('my_repo')
 
     # First fetch: not cached
-    _, _, stderr = self.RunBazel(['build', '//main:use_data'])
+    _, _, stderr = self.RunBazel(['build', '//main:use_data'] + extra_flags)
     self.assertIn('JUST FETCHED', '\n'.join(stderr))
     self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD')))
     self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt')))
@@ -519,14 +532,25 @@ def testUseRepoFileInBuildRule_actionDoesNotUseCache(self):
 
     # After expunging: repo and build action cached
     self.RunBazel(['clean', '--expunge'])
-    _, _, stderr = self.RunBazel(['build', '//main:use_data'])
+    _, _, stderr = self.RunBazel(['build', '//main:use_data'] + extra_flags)
     self.assertNotIn('JUST FETCHED', '\n'.join(stderr))
     self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD')))
     self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt')))
     self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt')))
     with open(self.Path('bazel-bin/main/out.txt')) as f:
       self.assertEqual(f.read(), 'hello')
 
+  def testUseRepoFileInBuildRule_actionDoesNotUseCache(self):
+    self.do_testUseRepoFileInBuildRule_actionDoesNotUseCache()
+
+  def testUseRepoFileInBuildRule_actionDoesNotUseCache_withExplicitSandboxBase(
+      self,
+  ):
+    tmpdir = self.ScratchDir('sandbox_base')
+    self.do_testUseRepoFileInBuildRule_actionDoesNotUseCache(
+        extra_flags=['--sandbox_base=' + tmpdir]
+    )
+
   def testLostRemoteFile_build(self):
     # Create a repo with two BUILD files (one in a subpackage), build a target
     # from one to cause it to be cached, then build that target again after