From 2afdb021aca4ed74e94d0e9d812c24fa14896518 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 28 Nov 2025 06:48:24 -0800 Subject: [PATCH] Fix `RemoteExternalOverlayFileSystem#resolveSymbolicLinks` Ensures that the returned `Path` is still in the overlay file system. Also make the error message emitted by `Path#checkSameFileSystem` more informative. This is motivated by and helped discover the above as the fix for the following crash observed when using the remote repo contents cache with an explicit `--sandbox_base`: ``` Caused by: java.lang.IllegalArgumentException: Files are on different filesystems: /dev/shm/bazel-sandbox.b10976335efa519b0184f3091ac8e21f7beefb92142303f9ab2c3341f45a2f28/linux-sandbox/18/execroot/_main/external/c-ares+/configs/ares_build.h (on com.google.devtools.build.lib.unix.UnixFileSystem@5e0a8154), /home/ubuntu/.cache/bazel/_bazel_ubuntu/123/execroot/_main/external/c-ares+/configs/ares_build.h (on com.google.devtools.build.lib.remote.RemoteExternalOverlayFileSystem@6cd9bfda) at com.google.devtools.build.lib.vfs.Path.checkSameFileSystem(Path.java:964) at com.google.devtools.build.lib.vfs.Path.createSymbolicLink(Path.java:523) at com.google.devtools.build.lib.vfs.Path.createSymbolicLink(Path.java:535) at com.google.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn.copyFile(SymlinkedSandboxedSpawn.java:129) ``` Alternative to https://github.com/bazelbuild/bazel/pull/27721 Closes #27802. PiperOrigin-RevId: 837832265 Change-Id: I3b73167496b011aef66954d59ca3804b4b64996f --- .../RemoteExternalOverlayFileSystem.java | 3 ++- .../google/devtools/build/lib/vfs/Path.java | 3 ++- .../bzlmod/remote_repo_contents_cache_test.py | 21 ++++++++++++++++--- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java index 99204b885a4551..55bbc762419273 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java @@ -542,7 +542,8 @@ public PathFragment resolveOneLink(PathFragment path) throws IOException { @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 diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Path.java b/src/main/java/com/google/devtools/build/lib/vfs/Path.java index 82b235ae1204e8..4460907b04f4fc 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/Path.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/Path.java @@ -960,7 +960,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)); } } } diff --git a/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py b/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py index 12bf9e29486280..c901e9b216c900 100644 --- a/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py +++ b/src/test/py/bazel/bzlmod/remote_repo_contents_cache_test.py @@ -482,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', [ @@ -518,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'))) @@ -528,7 +532,7 @@ 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'))) @@ -536,6 +540,17 @@ def testUseRepoFileInBuildRule_actionDoesNotUseCache(self): 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