From 4e89bc299e6e655094c749700146ea7acca4a97c Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 14 Jan 2025 03:05:18 -0800 Subject: [PATCH] Create symlink trees through the native filesystem, not the action filesystem. The comment added in SymlinkTreeStrategy explains why this is required. Fixes https://github.com/bazelbuild/bazel/issues/24867. PiperOrigin-RevId: 715305548 Change-Id: I376d360a0d072c0d5912e14e3115a7fb3b5f2281 --- .../build/lib/buildtool/ExecutionTool.java | 5 ++- .../build/lib/exec/RunfilesTreeUpdater.java | 2 +- .../build/lib/exec/SymlinkTreeHelper.java | 15 ++++---- .../build/lib/exec/SymlinkTreeStrategy.java | 34 ++++++++++++------- .../build/lib/exec/SymlinkTreeHelperTest.java | 14 ++++---- .../lib/exec/SymlinkTreeStrategyTest.java | 4 +-- .../lib/exec/util/TestExecutorBuilder.java | 4 ++- 7 files changed, 45 insertions(+), 33 deletions(-) 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 db7b064284d7d6..87f2869760b449 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 @@ -186,7 +186,10 @@ public class ExecutionTool { actionContextRegistryBuilder.register( SymlinkTreeActionContext.class, new SymlinkTreeStrategy( - env.getOutputService(), env.getBlazeWorkspace().getBinTools(), env.getWorkspaceName())); + env.getOutputService(), + env.getExecRoot(), + env.getBlazeWorkspace().getBinTools(), + env.getWorkspaceName())); // TODO(philwo) - the ExecutionTool should not add arbitrary dependencies on its own, instead // these dependencies should be added to the ActionContextConsumer of the module that actually // depends on them. diff --git a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java index 629a84200aa8f6..38ac043a5b3db7 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java +++ b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java @@ -121,7 +121,6 @@ private void updateRunfilesTree( if (!inputManifest.exists()) { return; } - Path outputManifest = execRoot.getRelative(RunfilesSupport.outputManifestExecPath(tree.getExecPath())); try { @@ -155,6 +154,7 @@ private void updateRunfilesTree( new SymlinkTreeHelper( execRoot, inputManifest, + outputManifest, runfilesDir, /* filesetTree= */ false, tree.getWorkspaceName()); diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java index 84b357d2552fd1..cb4ee3f9950db8 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java @@ -56,6 +56,7 @@ public final class SymlinkTreeHelper { private final Path execRoot; private final Path inputManifest; + private final Path outputManifest; private final Path symlinkTreeRoot; private final boolean filesetTree; private final String workspaceName; @@ -63,20 +64,23 @@ public final class SymlinkTreeHelper { /** * Creates SymlinkTreeHelper instance. Can be used independently of SymlinkTreeAction. * - * @param inputManifest exec path to the input runfiles manifest - * @param symlinkTreeRoot the root of the symlink tree to be created - * @param filesetTree true if this is fileset symlink tree, false if this is a runfiles symlink + * @param inputManifest path to the input runfiles manifest + * @param outputManifest path to the output runfiles manifest + * @param symlinkTreeRoot path to the root of the symlink tree + * @param filesetTree true if this is a fileset symlink tree, false if this is a runfiles symlink * tree. * @param workspaceName the name of the workspace, used to create the workspace subdirectory */ public SymlinkTreeHelper( Path execRoot, Path inputManifest, + Path outputManifest, Path symlinkTreeRoot, boolean filesetTree, String workspaceName) { this.execRoot = ensureNonSnapshotting(execRoot); this.inputManifest = ensureNonSnapshotting(inputManifest); + this.outputManifest = ensureNonSnapshotting(outputManifest); this.symlinkTreeRoot = ensureNonSnapshotting(symlinkTreeRoot); this.filesetTree = filesetTree; this.workspaceName = workspaceName; @@ -92,10 +96,6 @@ private static Path ensureNonSnapshotting(Path path) { return path; } - private Path getOutputManifest() { - return symlinkTreeRoot.getChild("MANIFEST"); - } - interface TargetPathFunction { /** Obtains a symlink target path from a T. */ PathFragment get(T target) throws IOException; @@ -190,7 +190,6 @@ private void deleteRunfilesDirectory() throws ExecException { /** Links the output manifest to the input manifest. */ private void linkManifest() throws ExecException { // Pretend we created the runfiles tree by symlinking the output manifest to the input manifest. - Path outputManifest = getOutputManifest(); try { symlinkTreeRoot.createDirectoryAndParents(); outputManifest.delete(); diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java index c80b4579bacac5..219b6dc812261b 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java @@ -54,11 +54,14 @@ public final class SymlinkTreeStrategy implements SymlinkTreeActionContext { (artifact) -> artifact == null ? null : artifact.getPath().asFragment(); private final OutputService outputService; + private final Path execRoot; private final BinTools binTools; private final String workspaceName; - public SymlinkTreeStrategy(OutputService outputService, BinTools binTools, String workspaceName) { + public SymlinkTreeStrategy( + OutputService outputService, Path execRoot, BinTools binTools, String workspaceName) { this.outputService = outputService; + this.execRoot = execRoot; this.binTools = binTools; this.workspaceName = workspaceName; } @@ -92,10 +95,10 @@ public void createSymlinks( // Delete symlinks possibly left over by a previous invocation with a different mode. // This is required because only the output manifest is considered an action output, so // Skyframe does not clear the directory for us. - createSymlinkTreeHelper(action, actionExecutionContext).clearRunfilesDirectory(); + createSymlinkTreeHelper(action).clearRunfilesDirectory(); } else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.INTERNAL) { try { - SymlinkTreeHelper helper = createSymlinkTreeHelper(action, actionExecutionContext); + SymlinkTreeHelper helper = createSymlinkTreeHelper(action); if (action.isFilesetTree()) { helper.createFilesetSymlinksDirectly(getFilesetMap(action, actionExecutionContext)); } else { @@ -111,7 +114,7 @@ public void createSymlinks( } else { Map resolvedEnv = new LinkedHashMap<>(); action.getEnvironment().resolve(resolvedEnv, actionExecutionContext.getClientEnv()); - createSymlinkTreeHelper(action, actionExecutionContext) + createSymlinkTreeHelper(action) .createSymlinksUsingCommand( binTools, resolvedEnv, actionExecutionContext.getFileOutErr()); } @@ -121,7 +124,7 @@ public void createSymlinks( } } - private static ImmutableMap getFilesetMap( + private ImmutableMap getFilesetMap( SymlinkTreeAction action, ActionExecutionContext actionExecutionContext) { ImmutableList filesetLinks; try { @@ -135,9 +138,7 @@ private static ImmutableMap getFilesetMap( } return SymlinkTreeHelper.processFilesetLinks( - filesetLinks, - action.getWorkspaceNameForFileset(), - actionExecutionContext.getExecRoot().asFragment()); + filesetLinks, action.getWorkspaceNameForFileset(), execRoot.asFragment()); } private static Map getRunfilesMap(SymlinkTreeAction action) { @@ -160,12 +161,19 @@ private static void createOutput( } } - private SymlinkTreeHelper createSymlinkTreeHelper( - SymlinkTreeAction action, ActionExecutionContext actionExecutionContext) { + private SymlinkTreeHelper createSymlinkTreeHelper(SymlinkTreeAction action) { + // Do not indirect paths through the action filesystem, for two reasons: + // (1) we always want to create the symlinks on disk, even if the action filesystem creates them + // in memory (at the time of writing, no action filesystem implementations do so, but this + // may change in the future). + // (2) current action filesystem implementations are not a true overlay filesystem, so errors + // might occur in an incremental build when the parent directory of a symlink exists on disk + // but not in memory (see https://github.com/bazelbuild/bazel/issues/24867). return new SymlinkTreeHelper( - actionExecutionContext.getExecRoot(), - actionExecutionContext.getInputPath(action.getInputManifest()), - actionExecutionContext.getInputPath(action.getOutputManifest()).getParentDirectory(), + execRoot, + action.getInputManifest().getPath(), + action.getOutputManifest().getPath(), + action.getOutputManifest().getPath().getParentDirectory(), action.isFilesetTree(), workspaceName); } diff --git a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java index 4cf5c4eeb0dde2..3fa708e2c88ec7 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java @@ -63,15 +63,13 @@ public void setUp() throws Exception { public void checkCreatedSpawn() { Path execRoot = fs.getPath("/my/workspace"); Path inputManifestPath = execRoot.getRelative("input_manifest"); + Path outputManifestPath = execRoot.getRelative("output/MANIFEST"); + Path symlinkTreeRoot = execRoot.getRelative("output"); BinTools binTools = BinTools.forUnitTesting(execRoot, ImmutableList.of(SymlinkTreeHelper.BUILD_RUNFILES)); Command command = new SymlinkTreeHelper( - execRoot, - inputManifestPath, - execRoot.getRelative("output/MANIFEST"), - false, - "__main__") + execRoot, inputManifestPath, outputManifestPath, symlinkTreeRoot, false, "__main__") .createCommand(binTools, ImmutableMap.of()); assertThat(command.getEnvironment()).isEmpty(); assertThat(command.getWorkingDirectory()).isEqualTo(execRoot.getPathFile()); @@ -80,7 +78,7 @@ public void checkCreatedSpawn() { assertThat(commandLine.get(0)).endsWith(SymlinkTreeHelper.BUILD_RUNFILES); assertThat(commandLine.get(1)).isEqualTo("--allow_relative"); assertThat(commandLine.get(2)).isEqualTo("input_manifest"); - assertThat(commandLine.get(3)).isEqualTo("output/MANIFEST"); + assertThat(commandLine.get(3)).isEqualTo("output"); } @Test @@ -134,8 +132,10 @@ public void createSymlinksDirectly( @TestParameter TreeType treeType, @TestParameter boolean replace) throws Exception { Path treeRoot = execRoot.getRelative("foo.runfiles"); Path inputManifestPath = execRoot.getRelative("foo.runfiles_manifest"); + Path outputManifestPath = execRoot.getRelative("foo.runfiles/MANIFEST"); SymlinkTreeHelper helper = - new SymlinkTreeHelper(execRoot, inputManifestPath, treeRoot, false, WORKSPACE_NAME); + new SymlinkTreeHelper( + execRoot, inputManifestPath, outputManifestPath, treeRoot, false, WORKSPACE_NAME); Artifact file = ActionsTestUtil.createArtifact(outputRoot, "file"); Artifact symlink = ActionsTestUtil.createUnresolvedSymlinkArtifact(outputRoot, "symlink"); diff --git a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java index 210f0ac3452d9c..2a55962ba5ee36 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java @@ -67,7 +67,7 @@ public void outputServiceInteraction() throws Exception { StoredEventHandler eventHandler = new StoredEventHandler(); when(context.getContext(SymlinkTreeActionContext.class)) - .thenReturn(new SymlinkTreeStrategy(outputService, null, "__main__")); + .thenReturn(new SymlinkTreeStrategy(outputService, getExecRoot(), null, "__main__")); when(context.getInputPath(any())).thenAnswer((i) -> ((Artifact) i.getArgument(0)).getPath()); when(context.getPathResolver()).thenReturn(ArtifactPathResolver.IDENTITY); when(context.getEventHandler()).thenReturn(eventHandler); @@ -131,7 +131,7 @@ public void inprocessSymlinkCreation() throws Exception { when(context.getExecRoot()).thenReturn(getExecRoot()); when(context.getContext(SymlinkTreeActionContext.class)) - .thenReturn(new SymlinkTreeStrategy(outputService, null, "__main__")); + .thenReturn(new SymlinkTreeStrategy(outputService, getExecRoot(), null, "__main__")); when(context.getInputPath(any())).thenAnswer((i) -> ((Artifact) i.getArgument(0)).getPath()); when(context.getEventHandler()).thenReturn(eventHandler); when(outputService.canCreateSymlinkTree()).thenReturn(false); diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java index 5fa63e98c6b978..7cd1a44c8ce9ee 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java @@ -71,7 +71,9 @@ public TestExecutorBuilder(FileSystem fileSystem, Path execRoot, BinTools binToo this.execRoot = execRoot; addContext(FileWriteActionContext.class, new FileWriteStrategy()); addContext(TemplateExpansionContext.class, new LocalTemplateExpansionStrategy()); - addContext(SymlinkTreeActionContext.class, new SymlinkTreeStrategy(null, binTools, "__main__")); + addContext( + SymlinkTreeActionContext.class, + new SymlinkTreeStrategy(null, execRoot, binTools, "__main__")); addContext(SpawnStrategyResolver.class, new SpawnStrategyResolver()); }