Skip to content

Commit 8eaf6a9

Browse files
fmeumcopybara-github
authored andcommitted
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 #27721 Closes #27802. PiperOrigin-RevId: 837832265 Change-Id: I3b73167496b011aef66954d59ca3804b4b64996f
1 parent 882d668 commit 8eaf6a9

File tree

3 files changed

+22
-5
lines changed

3 files changed

+22
-5
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -542,7 +542,8 @@ public PathFragment resolveOneLink(PathFragment path) throws IOException {
542542

543543
@Override
544544
public Path resolveSymbolicLinks(PathFragment path) throws IOException {
545-
return fsForPath(path).resolveSymbolicLinks(path);
545+
// Ensure that the return value doesn't leave the overlay file system.
546+
return getPath(fsForPath(path).resolveSymbolicLinks(path).asFragment());
546547
}
547548

548549
@Nullable

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -895,7 +895,8 @@ public void prefetchPackageAsync(int maxDirs) {
895895
private void checkSameFileSystem(Path that) {
896896
if (this.fileSystem != that.fileSystem) {
897897
throw new IllegalArgumentException(
898-
"Files are on different filesystems: " + this + ", " + that);
898+
"Files are on different filesystems: %s (on %s), %s (on %s)"
899+
.formatted(this, this.fileSystem, that, that.fileSystem));
899900
}
900901
}
901902
}

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

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,11 @@ def testUseRepoFileInBuildRule_actionUsesCache(self):
482482
with open(self.Path('bazel-bin/main/out.txt')) as f:
483483
self.assertEqual(f.read(), 'hello')
484484

485-
def testUseRepoFileInBuildRule_actionDoesNotUseCache(self):
485+
def do_testUseRepoFileInBuildRule_actionDoesNotUseCache(
486+
self, extra_flags=None
487+
):
488+
if extra_flags is None:
489+
extra_flags = []
486490
self.ScratchFile(
487491
'MODULE.bazel',
488492
[
@@ -518,7 +522,7 @@ def testUseRepoFileInBuildRule_actionDoesNotUseCache(self):
518522
repo_dir = self.RepoDir('my_repo')
519523

520524
# First fetch: not cached
521-
_, _, stderr = self.RunBazel(['build', '//main:use_data'])
525+
_, _, stderr = self.RunBazel(['build', '//main:use_data'] + extra_flags)
522526
self.assertIn('JUST FETCHED', '\n'.join(stderr))
523527
self.assertTrue(os.path.exists(os.path.join(repo_dir, 'BUILD')))
524528
self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt')))
@@ -528,14 +532,25 @@ def testUseRepoFileInBuildRule_actionDoesNotUseCache(self):
528532

529533
# After expunging: repo and build action cached
530534
self.RunBazel(['clean', '--expunge'])
531-
_, _, stderr = self.RunBazel(['build', '//main:use_data'])
535+
_, _, stderr = self.RunBazel(['build', '//main:use_data'] + extra_flags)
532536
self.assertNotIn('JUST FETCHED', '\n'.join(stderr))
533537
self.assertFalse(os.path.exists(os.path.join(repo_dir, 'BUILD')))
534538
self.assertTrue(os.path.exists(os.path.join(repo_dir, 'data.txt')))
535539
self.assertTrue(os.path.exists(self.Path('bazel-bin/main/out.txt')))
536540
with open(self.Path('bazel-bin/main/out.txt')) as f:
537541
self.assertEqual(f.read(), 'hello')
538542

543+
def testUseRepoFileInBuildRule_actionDoesNotUseCache(self):
544+
self.do_testUseRepoFileInBuildRule_actionDoesNotUseCache()
545+
546+
def testUseRepoFileInBuildRule_actionDoesNotUseCache_withExplicitSandboxBase(
547+
self,
548+
):
549+
tmpdir = self.ScratchDir('sandbox_base')
550+
self.do_testUseRepoFileInBuildRule_actionDoesNotUseCache(
551+
extra_flags=['--sandbox_base=' + tmpdir]
552+
)
553+
539554
def testLostRemoteFile_build(self):
540555
# Create a repo with two BUILD files (one in a subpackage), build a target
541556
# from one to cause it to be cached, then build that target again after

0 commit comments

Comments
 (0)