From ae47399fc7abe7e70621f5083c7358e1460c8fda Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 23 May 2026 21:04:24 +0200 Subject: [PATCH 1/2] Create `external/_main` under the execroot This is needed to correctly resolve rewritten cross-repo symlinks that arise as action inputs on Windows, which resolves symlinks logically (unlike Unix). Skipped on filesystems that can only emulate symlinks (Windows without Developer Mode / symlink privilege): a junction would create a cycle through the `bazel-` convenience symlink for any tool that walks the workspace, and is also unnecessary since `replantSymlinks` falls back to non-relative targets under the same condition. Fixes #29515 --- .../lib/bazel/BazelRepositoryModule.java | 3 +- .../lib/bazel/repository/RepositoryUtils.java | 5 +- .../build/lib/buildtool/ExecutionTool.java | 1 + .../build/lib/buildtool/SymlinkForest.java | 69 ++++++++- .../build/lib/cmdline/LabelConstants.java | 8 + .../lib/skyframe/IncrementalPackageRoots.java | 1 + .../bazel/bzlmod/repo_contents_cache_test.py | 140 ++++++++++++++++++ 7 files changed, 218 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index d3ee92b7053142..eb9db8aa15d334 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java @@ -68,7 +68,6 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.RepositoryOverride; -import com.google.devtools.build.lib.bazel.repository.RepositoryUtils; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache; import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager; import com.google.devtools.build.lib.bazel.repository.downloader.UrlRewriter; @@ -569,7 +568,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { || externalRoot.getFileSystem() instanceof RemoteExternalOverlayFileSystem) { try { FileSystemUtils.ensureSymbolicLink( - externalRoot.getChild(RepositoryUtils.WORKSPACE_SYMLINK_NAME), env.getWorkspace()); + externalRoot.getChild(LabelConstants.WORKSPACE_SYMLINK_NAME), env.getWorkspace()); } catch (IOException e) { env.getReporter().handle(Event.error(e.getMessage())); env.getBlazeModuleEnvironment() diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryUtils.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryUtils.java index ea181729bc0e25..d0e875136deb55 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryUtils.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryUtils.java @@ -38,8 +38,6 @@ /** Utility methods related to repo fetching. */ public class RepositoryUtils { - public static final String WORKSPACE_SYMLINK_NAME = "_main"; - private RepositoryUtils() {} public static boolean isValidRepoRoot(Path directory) { @@ -102,7 +100,8 @@ public static boolean replantSymlinks( boolean portableSymlinksOnly = true; try { Collection symlinks = FileSystemUtils.traverseTree(repoDir, Path::isSymbolicLink); - Path workspaceSymlinkUnderExternal = externalRepoRoot.getChild(WORKSPACE_SYMLINK_NAME); + Path workspaceSymlinkUnderExternal = + externalRepoRoot.getChild(LabelConstants.WORKSPACE_SYMLINK_NAME); FileSystemUtils.ensureSymbolicLink(workspaceSymlinkUnderExternal, workspace); for (Path symlink : symlinks) { PathFragment target = symlink.readSymbolicLink(); diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index e4adff5bc1a567..5c6cd5deb88028 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -652,6 +652,7 @@ private void prepare(PackageRoots packageRoots) throws AbruptExitException, Inte new SymlinkForest( packageRoots.getPackageRootsMap(), getExecRoot(), + env.getWorkspace(), runtime.getProductName(), request .getOptions(BuildLanguageOptions.class) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java b/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java index 9d970aff0bb726..e6b6443e418e3a 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java @@ -51,6 +51,7 @@ public class SymlinkForest { private final ImmutableMap packageRoots; private final Path execroot; + @Nullable private final Path workspace; private final String productName; private final String prefix; private final boolean siblingRepositoryLayout; @@ -58,7 +59,20 @@ public class SymlinkForest { /** Constructor for a symlink forest creator without non-symlinked directories parameter. */ public SymlinkForest( ImmutableMap packageRoots, Path execroot, String productName) { - this(packageRoots, execroot, productName, false); + this(packageRoots, execroot, /* workspace= */ null, productName, false); + } + + /** + * Convenience constructor that omits the workspace path. Intended for tests that do not exercise + * the planting of the {@code _main} symlink under the execroot's external directory. + */ + @VisibleForTesting + public SymlinkForest( + ImmutableMap packageRoots, + Path execroot, + String productName, + boolean siblingRepositoryLayout) { + this(packageRoots, execroot, /* workspace= */ null, productName, siblingRepositoryLayout); } /** @@ -68,15 +82,20 @@ public SymlinkForest( * * @param packageRoots source package roots to which to create symlinks * @param execroot path where to plant the symlink forest + * @param workspace path to the main workspace, used to materialize a {@code _main} symlink + * alongside other external repository symlinks under the execroot. May be {@code null} in + * test settings where this symlink is not needed. * @param productName {@code BlazeRuntime#getProductName()} */ public SymlinkForest( ImmutableMap packageRoots, Path execroot, + @Nullable Path workspace, String productName, boolean siblingRepositoryLayout) { this.packageRoots = packageRoots; this.execroot = execroot; + this.workspace = workspace; this.productName = productName; this.prefix = productName + "-"; this.siblingRepositoryLayout = siblingRepositoryLayout; @@ -127,7 +146,7 @@ private void plantSymlinkForExternalRepo( throws IOException { Optional plantedSymlink = plantSingleSymlinkForExternalRepo( - repository, source, execroot, siblingRepositoryLayout, externalRepoLinks); + repository, source, execroot, workspace, siblingRepositoryLayout, externalRepoLinks); plantedSymlink.ifPresent(plantedSymlinks::add); } @@ -452,6 +471,7 @@ public static Optional plantSingleSymlinkForExternalRepo( RepositoryName repository, Path source, Path execroot, + @Nullable Path workspace, boolean siblingRepositoryLayout, Set alreadyPlantedExternalRepoLinks) throws IOException { @@ -465,8 +485,28 @@ public static Optional plantSingleSymlinkForExternalRepo( // to /external/ Path execrootLink = execroot.getRelative(repository.getExecPath(siblingRepositoryLayout)); - if (!siblingRepositoryLayout && alreadyPlantedExternalRepoLinks.isEmpty()) { - execroot.getRelative(LabelConstants.EXTERNAL_PATH_PREFIX).createDirectoryAndParents(); + if (!siblingRepositoryLayout) { + Path externalDir = execroot.getRelative(LabelConstants.EXTERNAL_PATH_PREFIX); + // Always ensure the external directory exists before any repo symlink is planted into it. + // createDirectoryAndParents is idempotent and thus safe under concurrent invocations (as + // in IncrementalPackageRoots). + externalDir.createDirectoryAndParents(); + // Materialize /external/_main as a symlink to the workspace, mirroring the + // /external/_main symlink created during repo fetching (see + // RepositoryUtils.replantSymlinks). This is needed for external repositories whose + // cross-repo symlinks were replanted as relative paths through ../_main/...: on Windows, + // where symlinks resolve using logical paths, such relative targets dangle when the repo + // is accessed via /external//... unless _main also exists alongside the + // repo there. See https://github.com/bazelbuild/bazel/issues/29515. + if (workspace != null) { + Path workspaceLink = externalDir.getChild(LabelConstants.WORKSPACE_SYMLINK_NAME); + // Gate via the shared set so the symlink is created at most once even under concurrent + // invocations. No external repo can be named "_main" so this entry never collides with + // a real repo's execroot link. + if (alreadyPlantedExternalRepoLinks.add(workspaceLink)) { + createWorkspaceLinkUnderExecroot(workspaceLink, workspace); + } + } } // Prevent re-creating existing symlinks. if (!alreadyPlantedExternalRepoLinks.add(execrootLink)) { @@ -476,6 +516,27 @@ public static Optional plantSingleSymlinkForExternalRepo( return Optional.of(execrootLink); } + /** + * Creates the {@code _main} workspace symlink under the execroot's external directory. + * + *

Skipped when the filesystem can only emulate symlinks (via junctions or copies on Windows + * without Developer Mode / symlink privilege): a junction would create a cycle through the + * {@code bazel-} convenience symlink for tools that walk the workspace, and a file + * copy doesn't apply to a directory target. This is also correct from a functional standpoint: + * the relative {@code ../_main/...} targets that motivate this symlink are only produced when + * {@link com.google.devtools.build.lib.bazel.repository.RepositoryUtils#replantSymlinks} can + * create real symlinks, which is gated on the same filesystem capability. + */ + private static void createWorkspaceLinkUnderExecroot(Path workspaceLink, Path workspace) + throws IOException { + if (!workspaceLink + .getFileSystem() + .supportsSymbolicLinksNatively(workspaceLink.asFragment())) { + return; + } + workspaceLink.createSymbolicLink(workspace); + } + private static PackageIdentifier createInRepo( PackageIdentifier repo, PathFragment packageFragment) { return PackageIdentifier.create(repo.getRepository(), packageFragment); diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java b/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java index 164467e4bbe2a5..d9130b1f3b50e3 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java @@ -52,6 +52,14 @@ public class LabelConstants { // With this prefix, non-main repositories are symlinked under // $output_base/execution_root/__main__/external public static final PathFragment EXTERNAL_PATH_PREFIX = PathFragment.create("external"); + + /** + * The name of the symlink to the main workspace planted under {@code /external/} + * (and, for executions that may resolve symlinks logically, under {@code /external/}). + * It allows repo-rule symlinks pointing into the main workspace to be replanted as relative + * paths through {@code ../_main/...}. + */ + public static final String WORKSPACE_SYMLINK_NAME = "_main"; // With this prefix, non-main repositories are sibling symlinks of // $output_base/execution_root/__main__ public static final PathFragment EXPERIMENTAL_EXTERNAL_PATH_PREFIX = PathFragment.create(".."); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/IncrementalPackageRoots.java b/src/main/java/com/google/devtools/build/lib/skyframe/IncrementalPackageRoots.java index 0bb6aa762e1f82..90584075532057 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/IncrementalPackageRoots.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/IncrementalPackageRoots.java @@ -283,6 +283,7 @@ private Void plantSingleSymlinkForPackage( pkgId.getRepository(), pkg.sourceRoot().asPath(), execroot, + singleSourceRoot.asPath(), useSiblingRepositoryLayout, lazilyPlantedSymlinksRef); } else if (!maybeConflictingBaseNamesLowercase.isEmpty()) { 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 204f676710c940..0418259cf4a2a1 100644 --- a/src/test/py/bazel/bzlmod/repo_contents_cache_test.py +++ b/src/test/py/bazel/bzlmod/repo_contents_cache_test.py @@ -763,6 +763,146 @@ def testCachedRepoWithSymlinks_symlinksEnabledOnWindows(self): # by the replanting logic, so cross-repo symlinks prevent caching. self.doTestCachedRepoWithSymlinks(expect_cross_repo_cached=False) + def doTestRepoWithWorkspaceSymlinkSurvivesWorkspaceMove(self): + """Regression test for https://github.com/bazelbuild/bazel/issues/29515. + + A reproducible repo that symlinks a file from the main workspace has those + symlinks replanted to relative paths through `../_main/...`. On Windows + (which resolves symlinks logically) those targets dangle when the repo is + read through the execroot unless a corresponding `_main` symlink exists + under `/external/` too. After the fix, the build must succeed + even when the cached repo is reused from a completely different workspace + (with the original workspace and output base nuked between builds). + + The workspace is set up under a subdirectory of `_test_cwd` (rather than + in `_test_cwd` itself) so that we can delete it without breaking the + `tempfile.TemporaryFile(dir=self._test_cwd)` calls inside `RunBazel`. + """ + # Files that make up the workspace. Each file is written to both the + # original and the new workspace; the original is later deleted entirely. + ws_files = { + 'payload/BUILD.bazel': [], + 'payload/hello.txt': ['Hello from workspace!'], + 'MODULE.bazel': [ + 'ext = use_extension("extension.bzl", "ext")', + 'use_repo(ext, "linked", "same_repo_only")', + ], + 'extension.bzl': [ + 'def _linked_impl(ctx):', + ' ctx.file("REPO.bazel")', + ' # Workspace symlink: replantSymlinks rewrites this as', + ' # ../_main/payload/hello.txt before the repo is (potentially)', + ' # placed in the contents cache.', + ' ctx.symlink(', + ' ctx.path(Label("@@//payload:hello.txt")),', + ' "linked_hello.txt",', + ' )', + ' ctx.file("BUILD", "exports_files([\\"linked_hello.txt\\"])")', + ' return ctx.repo_metadata(reproducible = True)', + 'linked = repository_rule(implementation = _linked_impl)', + '', + 'def _same_repo_only_impl(ctx):', + ' ctx.file("REPO.bazel")', + ' ctx.file("data", "Hello from same-repo!")', + ' # Same-repo absolute symlink: replanted to a portable relative', + ' # path, so this repo is eligible for the contents cache.', + ' ctx.symlink(ctx.path("data"), "sym_same_repo")', + ' ctx.file("BUILD", "exports_files([\\"sym_same_repo\\"])")', + ' return ctx.repo_metadata(reproducible = True)', + 'same_repo_only = repository_rule(', + ' implementation = _same_repo_only_impl,', + ')', + '', + 'def _ext_impl(ctx):', + ' linked(name = "linked")', + ' same_repo_only(name = "same_repo_only")', + 'ext = module_extension(implementation = _ext_impl)', + ], + 'BUILD': [ + 'genrule(', + ' name = "use_linked",', + ' srcs = [', + ' "@linked//:linked_hello.txt",', + ' "@same_repo_only//:sym_same_repo",', + ' ],', + ' outs = ["output.txt"],', + # $(SRCS) expands to execroot-relative paths, which is the access + # pattern that triggers the bug being regressed. + ' cmd = "cat $(SRCS) > $@",', + ')', + ], + } + for rel_path, lines in ws_files.items(): + self.ScratchFile('original_ws/' + rel_path, lines) + + # First build in the original workspace: fetches and (where eligible) + # caches the repos. Use an explicit output base so we can delete it. + original_ws = os.path.join(self._test_cwd, 'original_ws') + original_output_base = tempfile.mkdtemp(dir=self._tests_root) + self.RunBazel( + [ + f'--output_base={original_output_base}', + 'build', + '//:use_linked', + ], + cwd=original_ws, + ) + output = os.path.join(original_ws, 'bazel-bin/output.txt') + self.AssertFileContentContains(output, 'Hello from workspace!') + self.AssertFileContentContains(output, 'Hello from same-repo!') + + # Shut the server down so the output base can be deleted on Windows + # (where running processes hold file handles). + self.RunBazel( + [f'--output_base={original_output_base}', 'shutdown'], + cwd=original_ws, + ) + + # Set up a fresh workspace from scratch (no copy from the original to + # ensure the original's bazel-* convenience symlinks don't leak in). + new_ws = os.path.join(self._test_cwd, 'new_ws') + for rel_path, lines in ws_files.items(): + self.ScratchFile('new_ws/' + rel_path, lines) + + # Nuke the original workspace and its output base. After this point only + # the contents cache and the new workspace remain on disk. + shutil.rmtree(original_ws) + shutil.rmtree(original_output_base) + + # Build from the new workspace with a fresh output base. The + # `same_repo_only` repo must come straight from the cache; the + # `linked` repo with the workspace symlink must work (either from the + # cache or refetched) and resolve correctly against the new workspace. + new_output_base = tempfile.mkdtemp(dir=self._tests_root) + self.RunBazel( + [ + f'--output_base={new_output_base}', + 'build', + '//:use_linked', + ], + cwd=new_ws, + ) + output = os.path.join(new_ws, 'bazel-bin/output.txt') + self.AssertFileContentContains(output, 'Hello from workspace!') + self.AssertFileContentContains(output, 'Hello from same-repo!') + + def testRepoWithWorkspaceSymlinkSurvivesWorkspaceMove(self): + self.doTestRepoWithWorkspaceSymlinkSurvivesWorkspaceMove() + + def testRepoWithWorkspaceSymlinkSurvivesWorkspaceMove_symlinksEnabledOnWindows( + self, + ): + if not self.IsWindows(): + self.skipTest('This test is only relevant on Windows') + self.ScratchFile( + '.bazelrc', + [ + 'startup --windows_enable_symlinks', + ], + mode='a', + ) + self.doTestRepoWithWorkspaceSymlinkSurvivesWorkspaceMove() + if __name__ == '__main__': absltest.main() From 5c9e19f27174e56179661ca4e435e0337567d09c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 24 May 2026 23:04:50 +0200 Subject: [PATCH 2/2] Verify test catches bug: revert Java fix, keep test --- .../lib/bazel/BazelRepositoryModule.java | 3 +- .../lib/bazel/repository/RepositoryUtils.java | 5 +- .../build/lib/buildtool/ExecutionTool.java | 1 - .../build/lib/buildtool/SymlinkForest.java | 69 +--------------- .../build/lib/cmdline/LabelConstants.java | 8 -- .../lib/skyframe/IncrementalPackageRoots.java | 1 - .../bazel/bzlmod/repo_contents_cache_test.py | 80 ++++++++++++++++--- 7 files changed, 80 insertions(+), 87 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index eb9db8aa15d334..d3ee92b7053142 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java @@ -68,6 +68,7 @@ import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode; import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.RepositoryOverride; +import com.google.devtools.build.lib.bazel.repository.RepositoryUtils; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache; import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager; import com.google.devtools.build.lib.bazel.repository.downloader.UrlRewriter; @@ -568,7 +569,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { || externalRoot.getFileSystem() instanceof RemoteExternalOverlayFileSystem) { try { FileSystemUtils.ensureSymbolicLink( - externalRoot.getChild(LabelConstants.WORKSPACE_SYMLINK_NAME), env.getWorkspace()); + externalRoot.getChild(RepositoryUtils.WORKSPACE_SYMLINK_NAME), env.getWorkspace()); } catch (IOException e) { env.getReporter().handle(Event.error(e.getMessage())); env.getBlazeModuleEnvironment() diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryUtils.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryUtils.java index d0e875136deb55..ea181729bc0e25 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryUtils.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryUtils.java @@ -38,6 +38,8 @@ /** Utility methods related to repo fetching. */ public class RepositoryUtils { + public static final String WORKSPACE_SYMLINK_NAME = "_main"; + private RepositoryUtils() {} public static boolean isValidRepoRoot(Path directory) { @@ -100,8 +102,7 @@ public static boolean replantSymlinks( boolean portableSymlinksOnly = true; try { Collection symlinks = FileSystemUtils.traverseTree(repoDir, Path::isSymbolicLink); - Path workspaceSymlinkUnderExternal = - externalRepoRoot.getChild(LabelConstants.WORKSPACE_SYMLINK_NAME); + Path workspaceSymlinkUnderExternal = externalRepoRoot.getChild(WORKSPACE_SYMLINK_NAME); FileSystemUtils.ensureSymbolicLink(workspaceSymlinkUnderExternal, workspace); for (Path symlink : symlinks) { PathFragment target = symlink.readSymbolicLink(); diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index 5c6cd5deb88028..e4adff5bc1a567 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -652,7 +652,6 @@ private void prepare(PackageRoots packageRoots) throws AbruptExitException, Inte new SymlinkForest( packageRoots.getPackageRootsMap(), getExecRoot(), - env.getWorkspace(), runtime.getProductName(), request .getOptions(BuildLanguageOptions.class) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java b/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java index e6b6443e418e3a..9d970aff0bb726 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java @@ -51,7 +51,6 @@ public class SymlinkForest { private final ImmutableMap packageRoots; private final Path execroot; - @Nullable private final Path workspace; private final String productName; private final String prefix; private final boolean siblingRepositoryLayout; @@ -59,20 +58,7 @@ public class SymlinkForest { /** Constructor for a symlink forest creator without non-symlinked directories parameter. */ public SymlinkForest( ImmutableMap packageRoots, Path execroot, String productName) { - this(packageRoots, execroot, /* workspace= */ null, productName, false); - } - - /** - * Convenience constructor that omits the workspace path. Intended for tests that do not exercise - * the planting of the {@code _main} symlink under the execroot's external directory. - */ - @VisibleForTesting - public SymlinkForest( - ImmutableMap packageRoots, - Path execroot, - String productName, - boolean siblingRepositoryLayout) { - this(packageRoots, execroot, /* workspace= */ null, productName, siblingRepositoryLayout); + this(packageRoots, execroot, productName, false); } /** @@ -82,20 +68,15 @@ public SymlinkForest( * * @param packageRoots source package roots to which to create symlinks * @param execroot path where to plant the symlink forest - * @param workspace path to the main workspace, used to materialize a {@code _main} symlink - * alongside other external repository symlinks under the execroot. May be {@code null} in - * test settings where this symlink is not needed. * @param productName {@code BlazeRuntime#getProductName()} */ public SymlinkForest( ImmutableMap packageRoots, Path execroot, - @Nullable Path workspace, String productName, boolean siblingRepositoryLayout) { this.packageRoots = packageRoots; this.execroot = execroot; - this.workspace = workspace; this.productName = productName; this.prefix = productName + "-"; this.siblingRepositoryLayout = siblingRepositoryLayout; @@ -146,7 +127,7 @@ private void plantSymlinkForExternalRepo( throws IOException { Optional plantedSymlink = plantSingleSymlinkForExternalRepo( - repository, source, execroot, workspace, siblingRepositoryLayout, externalRepoLinks); + repository, source, execroot, siblingRepositoryLayout, externalRepoLinks); plantedSymlink.ifPresent(plantedSymlinks::add); } @@ -471,7 +452,6 @@ public static Optional plantSingleSymlinkForExternalRepo( RepositoryName repository, Path source, Path execroot, - @Nullable Path workspace, boolean siblingRepositoryLayout, Set alreadyPlantedExternalRepoLinks) throws IOException { @@ -485,28 +465,8 @@ public static Optional plantSingleSymlinkForExternalRepo( // to /external/ Path execrootLink = execroot.getRelative(repository.getExecPath(siblingRepositoryLayout)); - if (!siblingRepositoryLayout) { - Path externalDir = execroot.getRelative(LabelConstants.EXTERNAL_PATH_PREFIX); - // Always ensure the external directory exists before any repo symlink is planted into it. - // createDirectoryAndParents is idempotent and thus safe under concurrent invocations (as - // in IncrementalPackageRoots). - externalDir.createDirectoryAndParents(); - // Materialize /external/_main as a symlink to the workspace, mirroring the - // /external/_main symlink created during repo fetching (see - // RepositoryUtils.replantSymlinks). This is needed for external repositories whose - // cross-repo symlinks were replanted as relative paths through ../_main/...: on Windows, - // where symlinks resolve using logical paths, such relative targets dangle when the repo - // is accessed via /external//... unless _main also exists alongside the - // repo there. See https://github.com/bazelbuild/bazel/issues/29515. - if (workspace != null) { - Path workspaceLink = externalDir.getChild(LabelConstants.WORKSPACE_SYMLINK_NAME); - // Gate via the shared set so the symlink is created at most once even under concurrent - // invocations. No external repo can be named "_main" so this entry never collides with - // a real repo's execroot link. - if (alreadyPlantedExternalRepoLinks.add(workspaceLink)) { - createWorkspaceLinkUnderExecroot(workspaceLink, workspace); - } - } + if (!siblingRepositoryLayout && alreadyPlantedExternalRepoLinks.isEmpty()) { + execroot.getRelative(LabelConstants.EXTERNAL_PATH_PREFIX).createDirectoryAndParents(); } // Prevent re-creating existing symlinks. if (!alreadyPlantedExternalRepoLinks.add(execrootLink)) { @@ -516,27 +476,6 @@ public static Optional plantSingleSymlinkForExternalRepo( return Optional.of(execrootLink); } - /** - * Creates the {@code _main} workspace symlink under the execroot's external directory. - * - *

Skipped when the filesystem can only emulate symlinks (via junctions or copies on Windows - * without Developer Mode / symlink privilege): a junction would create a cycle through the - * {@code bazel-} convenience symlink for tools that walk the workspace, and a file - * copy doesn't apply to a directory target. This is also correct from a functional standpoint: - * the relative {@code ../_main/...} targets that motivate this symlink are only produced when - * {@link com.google.devtools.build.lib.bazel.repository.RepositoryUtils#replantSymlinks} can - * create real symlinks, which is gated on the same filesystem capability. - */ - private static void createWorkspaceLinkUnderExecroot(Path workspaceLink, Path workspace) - throws IOException { - if (!workspaceLink - .getFileSystem() - .supportsSymbolicLinksNatively(workspaceLink.asFragment())) { - return; - } - workspaceLink.createSymbolicLink(workspace); - } - private static PackageIdentifier createInRepo( PackageIdentifier repo, PathFragment packageFragment) { return PackageIdentifier.create(repo.getRepository(), packageFragment); diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java b/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java index d9130b1f3b50e3..164467e4bbe2a5 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java @@ -52,14 +52,6 @@ public class LabelConstants { // With this prefix, non-main repositories are symlinked under // $output_base/execution_root/__main__/external public static final PathFragment EXTERNAL_PATH_PREFIX = PathFragment.create("external"); - - /** - * The name of the symlink to the main workspace planted under {@code /external/} - * (and, for executions that may resolve symlinks logically, under {@code /external/}). - * It allows repo-rule symlinks pointing into the main workspace to be replanted as relative - * paths through {@code ../_main/...}. - */ - public static final String WORKSPACE_SYMLINK_NAME = "_main"; // With this prefix, non-main repositories are sibling symlinks of // $output_base/execution_root/__main__ public static final PathFragment EXPERIMENTAL_EXTERNAL_PATH_PREFIX = PathFragment.create(".."); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/IncrementalPackageRoots.java b/src/main/java/com/google/devtools/build/lib/skyframe/IncrementalPackageRoots.java index 90584075532057..0bb6aa762e1f82 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/IncrementalPackageRoots.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/IncrementalPackageRoots.java @@ -283,7 +283,6 @@ private Void plantSingleSymlinkForPackage( pkgId.getRepository(), pkg.sourceRoot().asPath(), execroot, - singleSourceRoot.asPath(), useSiblingRepositoryLayout, lazilyPlantedSymlinksRef); } else if (!maybeConflictingBaseNamesLowercase.isEmpty()) { 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 0418259cf4a2a1..93d015b257387f 100644 --- a/src/test/py/bazel/bzlmod/repo_contents_cache_test.py +++ b/src/test/py/bazel/bzlmod/repo_contents_cache_test.py @@ -101,6 +101,23 @@ def assertRepoNotCached(self, repo_dir): except OSError: pass # Not a symlink means not cached, which is expected + def _rmtreeWithRetry(self, path, attempts=10, delay=0.5): + """Like shutil.rmtree, but retries on Windows file-lock errors. + + `bazel shutdown` returns before the OS has finished releasing file + handles in the output base, so a follow-up rmtree may transiently hit + PermissionError on Windows. We retry a few times before giving up. + """ + last_err = None + for _ in range(attempts): + try: + shutil.rmtree(path) + return + except OSError as e: + last_err = e + time.sleep(delay) + raise last_err + def testCachedAfterCleanExpunge(self): self.ScratchFile( 'MODULE.bazel', @@ -826,9 +843,14 @@ def doTestRepoWithWorkspaceSymlinkSurvivesWorkspaceMove(self): ' "@same_repo_only//:sym_same_repo",', ' ],', ' outs = ["output.txt"],', - # $(SRCS) expands to execroot-relative paths, which is the access - # pattern that triggers the bug being regressed. - ' cmd = "cat $(SRCS) > $@",', + # cmd_bat uses native cmd.exe with execroot-relative Windows paths + # to match the original reproducer; on Windows the bug only + # manifests through that resolver, not through MSYS bash/cat which + # dereferences symlinks differently. + ' cmd = "cat $(execpath @linked//:linked_hello.txt) ' + '$(execpath @same_repo_only//:sym_same_repo) > $@",', + ' cmd_bat = "type $(execpath @linked//:linked_hello.txt) ' + '$(execpath @same_repo_only//:sym_same_repo) > $@",', ')', ], } @@ -851,10 +873,12 @@ def doTestRepoWithWorkspaceSymlinkSurvivesWorkspaceMove(self): self.AssertFileContentContains(output, 'Hello from workspace!') self.AssertFileContentContains(output, 'Hello from same-repo!') - # Shut the server down so the output base can be deleted on Windows - # (where running processes hold file handles). + # Let Bazel itself delete the output base via `clean --expunge` (which + # also shuts the server down). On Windows direct rmtree of a still-warm + # output base races with the OS releasing file handles, but the Bazel + # client knows how to wait for its own server to fully exit. self.RunBazel( - [f'--output_base={original_output_base}', 'shutdown'], + [f'--output_base={original_output_base}', 'clean', '--expunge'], cwd=original_ws, ) @@ -864,10 +888,14 @@ def doTestRepoWithWorkspaceSymlinkSurvivesWorkspaceMove(self): for rel_path, lines in ws_files.items(): self.ScratchFile('new_ws/' + rel_path, lines) - # Nuke the original workspace and its output base. After this point only - # the contents cache and the new workspace remain on disk. + # Nuke the original workspace. The original output base is already gone + # (clean --expunge above). After this point only the contents cache and + # the new workspace remain on disk. shutil.rmtree(original_ws) - shutil.rmtree(original_output_base) + if os.path.exists(original_output_base): + # clean --expunge may leave behind an empty shell on some platforms; + # remove what's left, retrying a few times for Windows file-handle lag. + self._rmtreeWithRetry(original_output_base) # Build from the new workspace with a fresh output base. The # `same_repo_only` repo must come straight from the cache; the @@ -886,6 +914,40 @@ def doTestRepoWithWorkspaceSymlinkSurvivesWorkspaceMove(self): self.AssertFileContentContains(output, 'Hello from workspace!') self.AssertFileContentContains(output, 'Hello from same-repo!') + # On platforms that natively support symbolic links (always on Unix; on + # Windows only with Developer Mode), the fix materializes a `_main` + # symlink under `/external/` pointing at the workspace, so the + # relative `../_main/...` targets that replantSymlinks produces resolve + # correctly when read through the execroot. Verify it's there. + # + # The behavioral check above (the build itself) only catches the bug on + # Windows with real symlinks (which Bazel CI Windows runners don't have), + # so this structural assertion is what actually guards the fix on the + # Linux/macOS test runs. + _, stdout, _ = self.RunBazel( + [f'--output_base={new_output_base}', 'info', 'execution_root'], + cwd=new_ws, + ) + execroot = stdout[0].strip() + workspace_link = os.path.join(execroot, 'external', '_main') + if not self.IsWindows() or self._realSymlinksWork(): + self.assertTrue( + os.path.lexists(workspace_link), + msg=f'expected workspace symlink to be planted at {workspace_link}', + ) + + def _realSymlinksWork(self): + """Best-effort check for whether the OS can create a real symlink here.""" + probe_dir = tempfile.mkdtemp(dir=self._tests_root) + target = os.path.join(probe_dir, 'target') + os.mkdir(target) + link = os.path.join(probe_dir, 'link') + try: + os.symlink(target, link, target_is_directory=True) + return True + except OSError: + return False + def testRepoWithWorkspaceSymlinkSurvivesWorkspaceMove(self): self.doTestRepoWithWorkspaceSymlinkSurvivesWorkspaceMove()