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/BUILD b/src/main/java/com/google/devtools/build/lib/buildtool/BUILD index 9e43e35a83398b..32fb1b24331267 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BUILD +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BUILD @@ -84,6 +84,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/concurrent:thread_safety", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", + "//src/main/java/com/google/devtools/build/lib/util:os", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//third_party:flogger", 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..eda3aa10b24b1a 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 @@ -29,11 +29,13 @@ import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.util.AbruptExitException; +import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.io.IOException; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -51,6 +53,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 +61,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 +84,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 +148,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 +473,7 @@ public static Optional plantSingleSymlinkForExternalRepo( RepositoryName repository, Path source, Path execroot, + @Nullable Path workspace, boolean siblingRepositoryLayout, Set alreadyPlantedExternalRepoLinks) throws IOException { @@ -465,8 +487,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 +518,38 @@ public static Optional plantSingleSymlinkForExternalRepo( return Optional.of(execrootLink); } + /** + * Creates the {@code _main} workspace symlink under the execroot's external directory. + * + *

On Windows we go through {@link Files#createSymbolicLink} to produce a real symlink rather + * than letting Bazel's {@link Path#createSymbolicLink} fall back to a junction. A junction looks + * like a directory to tools that don't special-case junctions (e.g. Python's {@code shutil}, + * most workspace-walkers), so it would introduce a cycle through the {@code bazel-} + * convenience symlink and break those tools. If real symlinks aren't available (no Developer + * Mode / symlink privilege), skip silently — that's the same condition under which {@link + * com.google.devtools.build.lib.bazel.repository.RepositoryUtils#replantSymlinks} doesn't + * produce the relative {@code ../_main/...} targets that motivate this symlink in the first + * place. + */ + private static void createWorkspaceLinkUnderExecroot(Path workspaceLink, Path workspace) + throws IOException { + if (OS.getCurrent() != OS.WINDOWS) { + workspaceLink.createSymbolicLink(workspace); + return; + } + var workspaceLinkNioPath = + workspaceLink.getFileSystem().getNioPath(workspaceLink.asFragment()); + var workspaceNioPath = workspaceLink.getFileSystem().getNioPath(workspace.asFragment()); + if (workspaceLinkNioPath == null || workspaceNioPath == null) { + return; + } + try { + Files.createSymbolicLink(workspaceLinkNioPath, workspaceNioPath); + } catch (IOException | UnsupportedOperationException e) { + // Real symlinks aren't available; see method javadoc. + } + } + 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..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', @@ -763,6 +780,191 @@ 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"],', + # 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) > $@",', + ')', + ], + } + 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!') + + # 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}', 'clean', '--expunge'], + 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. 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) + 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 + # `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!') + + # 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() + + 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()