Make runfiles incrementally correct with `--nobuild_runfile_links`
Fixes three separate but related issues with `--nobuild_runfile_links`:
* On all OSes, flipping `--enable_runfiles` from on to off did not result in runfiles being cleared due to a missing call to `SymlinkTreeHelper#clearRunfilesDirectory` that was only added to `SymlinkTreeStrategy` in f84329e007e2421c92805b50da135af411ee754c.
* On Windows only, flipping `--enable_runfiles` from off to on did not result in runfiles being created since the logic in `RunfilesTreeUpdater` would see a copy of the manifest instead of a symlink. This is fixed by additionally checking whether the runfiles directory contains the first runfile on Windows. If runfiles were disabled before, it won't, otherwise it will.
* On all OSes, `--noenable_runfiles` was ignored by `bazel run`, with runfiles always being created. This is fixed by using `RunfilesTreeUpdater` instead of custom and incorrect logic.
With the fixed behavior, the runfiles tree for tests is now cleared right before the test spawn is executed, which makes the test action unable to create its working directory, the subdirectory of the runfiles directory corresponding to the workspace name. To fix this and also get rid of the inconsistency of having another action write into the runfiles tree, this logic is moved into the `SymlinkTreeHelper` and thus applied to all runfiles trees.
Work towards #20676
Fixes #19333
Closes #20695.
PiperOrigin-RevId: 595369235
Change-Id: I32efc0e6a1d29291470ce5eafcc69bbd9ab276b9
diff --git a/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java
index 9d6a4be..ac36f4d 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java
@@ -49,6 +49,9 @@
/** Returns whether the runfile symlinks should be materialized during the build. */
boolean isBuildRunfileLinks();
+
+ /** Returns the name of the workspace that the build is occurring in. */
+ String getWorkspaceName();
}
/** Returns the runfiles trees to be materialized on the inputs of the action. */
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
index ab927eb..84d0907 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -343,8 +343,9 @@
}
/** Returns the name of the workspace that the build is occurring in. */
- public PathFragment getWorkspaceName() {
- return runfiles.getSuffix();
+ @Override
+ public String getWorkspaceName() {
+ return runfiles.getSuffix().getPathString();
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java
index 773b219..c78930d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java
@@ -166,6 +166,11 @@
return buildRunfileLinks;
}
+ @Override
+ public String getWorkspaceName() {
+ return runfiles.getSuffix().getPathString();
+ }
+
/** Softly caches the result of {@link Runfiles#getRunfilesInputs}. */
private static final class RunfilesCacher implements Supplier<Map<PathFragment, Artifact>> {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java
index d25f749..6e10dbf 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java
@@ -116,15 +116,11 @@
* Ensures that all directories used to run test are in the correct state and their content will
* not result in stale files.
*/
- protected void prepareFileSystem(
- TestRunnerAction testAction, Path execRoot, Path tmpDir, Path workingDirectory)
+ protected void prepareFileSystem(TestRunnerAction testAction, Path execRoot, Path tmpDir)
throws IOException {
if (tmpDir != null) {
recreateDirectory(tmpDir);
}
- if (workingDirectory != null) {
- workingDirectory.createDirectoryAndParents();
- }
ResolvedPaths resolvedPaths = testAction.resolve(execRoot);
if (testAction.isCoverageMode()) {
@@ -142,7 +138,7 @@
* not result in stale files. Only use this if no local tmp and working directory are required.
*/
protected void prepareFileSystem(TestRunnerAction testAction, Path execRoot) throws IOException {
- prepareFileSystem(testAction, execRoot, null, null);
+ prepareFileSystem(testAction, execRoot, null);
}
/** Removes directory if it exists and recreates it. */
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 2365251..fcda8a4 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
@@ -181,7 +181,8 @@
}
actionContextRegistryBuilder.register(
SymlinkTreeActionContext.class,
- new SymlinkTreeStrategy(env.getOutputService(), env.getBlazeWorkspace().getBinTools()));
+ new SymlinkTreeStrategy(
+ env.getOutputService(), 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/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD
index a312e80..3f32d62 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD
@@ -195,10 +195,10 @@
":symlink_tree_helper",
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/actions",
- "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:runfiles_supplier",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
+ "//src/main/java/com/google/devtools/build/lib/util:os",
"//src/main/java/com/google/devtools/build/lib/util/io:out-err",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
@@ -373,13 +373,11 @@
"//src/main/java/com/google/devtools/build/lib/actions:action_input_helper",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
- "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/buildeventstream",
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/events",
- "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util/io",
"//src/main/java/com/google/devtools/build/lib/vfs",
@@ -387,7 +385,6 @@
"//src/main/protobuf:failure_details_java_proto",
"//src/main/protobuf:test_status_java_proto",
"//third_party:guava",
- "//third_party:jsr305",
"//third_party/protobuf:protobuf_java",
"//third_party/protobuf:protobuf_java_util",
],
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 b9bccc7..e12efcd 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
@@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.exec;
+import static com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode.SKIP;
+import static java.nio.charset.StandardCharsets.ISO_8859_1;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableMap;
@@ -21,12 +23,16 @@
import com.google.devtools.build.lib.actions.RunfilesSupplier.RunfilesTree;
import com.google.devtools.build.lib.analysis.RunfilesSupport;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
+import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.io.OutErr;
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.lib.vfs.XattrProvider;
+import java.io.BufferedReader;
import java.io.IOException;
+import java.io.InputStreamReader;
import java.util.Arrays;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
@@ -122,11 +128,17 @@
// implying the symlinks exist and are already up to date. If the output manifest is a
// symbolic link, it is likely a symbolic link to the input manifest, so we cannot trust it as
// an up-to-date check.
- if (!outputManifest.isSymbolicLink()
+ // On Windows, where symlinks may be silently replaced by copies, a previous run in SKIP mode
+ // could have resulted in an output manifest that is an identical copy of the input manifest,
+ // which we must not treat as up to date, but we also don't want to unnecessarily rebuild the
+ // runfiles directory all the time. Instead, check for the presence of the first runfile in
+ // the manifest. If it is present, we can be certain that the previous mode wasn't SKIP.
+ if (tree.getSymlinksMode() != SKIP
+ && !outputManifest.isSymbolicLink()
&& Arrays.equals(
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(outputManifest, xattrProvider),
- DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(
- inputManifest, xattrProvider))) {
+ DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(inputManifest, xattrProvider))
+ && (OS.getCurrent() != OS.WINDOWS || isRunfilesDirectoryPopulated(runfilesDirPath))) {
return;
}
} catch (IOException e) {
@@ -138,11 +150,12 @@
}
SymlinkTreeHelper helper =
- new SymlinkTreeHelper(inputManifest, runfilesDirPath, /* filesetTree= */ false);
+ new SymlinkTreeHelper(
+ inputManifest, runfilesDirPath, /* filesetTree= */ false, tree.getWorkspaceName());
switch (tree.getSymlinksMode()) {
case SKIP:
- helper.linkManifest();
+ helper.clearRunfilesDirectory();
break;
case EXTERNAL:
helper.createSymlinksUsingCommand(execRoot, binTools, env, outErr);
@@ -153,4 +166,19 @@
break;
}
}
+
+ private boolean isRunfilesDirectoryPopulated(Path runfilesDirPath) {
+ Path outputManifest = RunfilesSupport.outputManifestPath(runfilesDirPath);
+ String relativeRunfilePath;
+ try (BufferedReader reader =
+ new BufferedReader(new InputStreamReader(outputManifest.getInputStream(), ISO_8859_1))) {
+ // If it is created at all, the manifest always contains at least one line.
+ relativeRunfilePath = reader.readLine().split(" ", -1)[0];
+ } catch (IOException e) {
+ // Instead of failing outright, just assume the runfiles directory is not populated.
+ return false;
+ }
+ // The runfile could be a dangling symlink.
+ return runfilesDirPath.getRelative(relativeRunfilePath).exists(Symlinks.NOFOLLOW);
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
index a6b74ae..05141b4 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -136,11 +136,8 @@
localResourcesSupplier);
Path execRoot = actionExecutionContext.getExecRoot();
ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
- Path runfilesDir = pathResolver.convertPath(action.getExecutionSettings().getRunfilesDir());
Path tmpDir = pathResolver.convertPath(tmpDirRoot.getChild(TestStrategy.getTmpDirName(action)));
- Path workingDirectory = runfilesDir.getRelative(action.getRunfilesPrefix());
- return new StandaloneTestRunnerSpawn(
- action, actionExecutionContext, spawn, tmpDir, workingDirectory, execRoot);
+ return new StandaloneTestRunnerSpawn(action, actionExecutionContext, spawn, tmpDir, execRoot);
}
private static ImmutableList<Pair<String, Path>> renameOutputs(
@@ -556,7 +553,6 @@
private final ActionExecutionContext actionExecutionContext;
private final Spawn spawn;
private final Path tmpDir;
- private final Path workingDirectory;
private final Path execRoot;
StandaloneTestRunnerSpawn(
@@ -564,13 +560,11 @@
ActionExecutionContext actionExecutionContext,
Spawn spawn,
Path tmpDir,
- Path workingDirectory,
Path execRoot) {
this.testAction = testAction;
this.actionExecutionContext = actionExecutionContext;
this.spawn = spawn;
this.tmpDir = tmpDir;
- this.workingDirectory = workingDirectory;
this.execRoot = execRoot;
}
@@ -581,7 +575,7 @@
@Override
public TestAttemptResult execute() throws InterruptedException, IOException, ExecException {
- prepareFileSystem(testAction, execRoot, tmpDir, workingDirectory);
+ prepareFileSystem(testAction, execRoot, tmpDir);
return beginTestAttempt(testAction, spawn, actionExecutionContext, execRoot);
}
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 504b9b8..a85a8df 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
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.exec;
-
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
@@ -57,6 +56,7 @@
private final Path inputManifest;
private final Path symlinkTreeRoot;
private final boolean filesetTree;
+ private final String workspaceName;
/**
* Creates SymlinkTreeHelper instance. Can be used independently of SymlinkTreeAction.
@@ -65,11 +65,14 @@
* @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
* tree.
+ * @param workspaceName the name of the workspace, used to create the workspace subdirectory
*/
- public SymlinkTreeHelper(Path inputManifest, Path symlinkTreeRoot, boolean filesetTree) {
+ public SymlinkTreeHelper(
+ Path inputManifest, Path symlinkTreeRoot, boolean filesetTree, String workspaceName) {
this.inputManifest = inputManifest;
this.symlinkTreeRoot = symlinkTreeRoot;
this.filesetTree = filesetTree;
+ this.workspaceName = workspaceName;
}
private Path getOutputManifest() {
@@ -118,11 +121,26 @@
parentDir.addSymlink(entry.getKey().getBaseName(), entry.getValue());
}
root.syncTreeRecursively(symlinkTreeRoot);
+ createWorkspaceSubdirectory();
+ }
+ }
+
+ /**
+ * Ensures that the runfiles directory is empty except for the symlinked MANIFEST and the
+ * workspace subdirectory. This is the expected state with --noenable_runfiles.
+ */
+ public void clearRunfilesDirectory() throws ExecException {
+ deleteRunfilesDirectory();
+ linkManifest();
+ try {
+ createWorkspaceSubdirectory();
+ } catch (IOException e) {
+ throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_CREATION_IO_EXCEPTION);
}
}
/** Deletes the contents of the runfiles directory. */
- public void clearRunfilesDirectory() throws ExecException {
+ private void deleteRunfilesDirectory() throws ExecException {
try (SilentCloseable c = Profiler.instance().profile("Clear symlink tree")) {
symlinkTreeRoot.deleteTreesBelow();
} catch (IOException e) {
@@ -131,7 +149,7 @@
}
/** Links the output manifest to the input manifest. */
- public void linkManifest() throws ExecException {
+ private void linkManifest() throws ExecException {
// Pretend we created the runfiles tree by symlinking the output manifest to the input manifest.
Path outputManifest = getOutputManifest();
try {
@@ -143,6 +161,14 @@
}
}
+ private void createWorkspaceSubdirectory() throws IOException {
+ // Always create the subdirectory corresponding to the workspace (i.e., the main repository).
+ // This is required by tests as their working directory, even with --noenable_runfiles. But if
+ // the test action creates the directory and then proceeds to execute the test spawn, this logic
+ // would remove it. For the sake of consistency, always create the directory instead.
+ symlinkTreeRoot.getRelative(workspaceName).createDirectory();
+ }
+
/**
* Creates a symlink tree using a CommandBuilder. This means that the symlink tree will always be
* present on the developer's workstation. Useful when running commands locally.
@@ -170,6 +196,11 @@
Execution.newBuilder().setCode(Code.SYMLINK_TREE_CREATION_COMMAND_EXCEPTION))
.build());
}
+ try {
+ createWorkspaceSubdirectory();
+ } catch (IOException e) {
+ throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_CREATION_IO_EXCEPTION);
+ }
}
}
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 18d3592..f0c8d39 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
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.exec;
-
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.base.Preconditions;
@@ -56,10 +55,12 @@
private final OutputService outputService;
private final BinTools binTools;
+ private final String workspaceName;
- public SymlinkTreeStrategy(OutputService outputService, BinTools binTools) {
+ public SymlinkTreeStrategy(OutputService outputService, BinTools binTools, String workspaceName) {
this.outputService = outputService;
this.binTools = binTools;
+ this.workspaceName = workspaceName;
}
@Override
@@ -97,17 +98,14 @@
}
outputService.createSymlinkTree(
- symlinks,
- action.getOutputManifest().getExecPath().getParentDirectory());
+ symlinks, action.getOutputManifest().getExecPath().getParentDirectory());
createOutput(action, actionExecutionContext, inputManifest);
} else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.SKIP) {
// 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.
- var helper = createSymlinkTreeHelper(action, actionExecutionContext);
- helper.clearRunfilesDirectory();
- helper.linkManifest();
+ createSymlinkTreeHelper(action, actionExecutionContext).clearRunfilesDirectory();
} else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.INTERNAL
&& !action.isFilesetTree()) {
try {
@@ -163,12 +161,13 @@
}
}
- private static SymlinkTreeHelper createSymlinkTreeHelper(
+ private SymlinkTreeHelper createSymlinkTreeHelper(
SymlinkTreeAction action, ActionExecutionContext actionExecutionContext) {
return new SymlinkTreeHelper(
actionExecutionContext.getInputPath(action.getInputManifest()),
actionExecutionContext.getInputPath(action.getOutputManifest()).getParentDirectory(),
- action.isFilesetTree());
+ action.isFilesetTree(),
+ workspaceName);
}
private static EnvironmentalExecException createLinkFailureException(
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD
index d321c74..beb3fff 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD
@@ -58,7 +58,7 @@
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/exec:execution_options",
- "//src/main/java/com/google/devtools/build/lib/exec:symlink_tree_helper",
+ "//src/main/java/com/google/devtools/build/lib/exec:runfiles_tree_updater",
"//src/main/java/com/google/devtools/build/lib/exec:test_policy",
"//src/main/java/com/google/devtools/build/lib/exec/local:options",
"//src/main/java/com/google/devtools/build/lib/packages",
@@ -91,7 +91,6 @@
"//src/main/java/com/google/devtools/build/lib/util:command",
"//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
"//src/main/java/com/google/devtools/build/lib/util:exit_code",
- "//src/main/java/com/google/devtools/build/lib/util:filetype",
"//src/main/java/com/google/devtools/build/lib/util:interrupted_failure_details",
"//src/main/java/com/google/devtools/build/lib/util:os",
"//src/main/java/com/google/devtools/build/lib/util:shell_escaper",
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
index f8d92cc..f2f36a9 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
@@ -32,7 +32,6 @@
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.CommandLine;
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
-import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.analysis.AliasProvider;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -62,7 +61,7 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.exec.ExecutionOptions;
-import com.google.devtools.build.lib.exec.SymlinkTreeHelper;
+import com.google.devtools.build.lib.exec.RunfilesTreeUpdater;
import com.google.devtools.build.lib.exec.TestPolicy;
import com.google.devtools.build.lib.packages.InputFile;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -91,7 +90,6 @@
import com.google.devtools.build.lib.util.CommandFailureUtils;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.ExitCode;
-import com.google.devtools.build.lib.util.FileType;
import com.google.devtools.build.lib.util.InterruptedFailureDetails;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.OptionsUtils;
@@ -158,8 +156,6 @@
"'run' only works with tests with one shard ('--test_sharding_strategy=disabled' is okay) "
+ "and without --runs_per_test";
- private static final FileType RUNFILES_MANIFEST = FileType.of(".runfiles_manifest");
-
private static final ImmutableList<String> ENV_VARIABLES_TO_CLEAR =
ImmutableList.of(
// These variables are all used by runfiles libraries to locate the runfiles directory or
@@ -468,6 +464,7 @@
// target to run needs to be preserved, as it acts as the working directory.
Path targetToRunRunfilesDir = null;
RunfilesSupport targetToRunRunfilesSupport = null;
+ RunfilesTreeUpdater runfilesTreeUpdater = RunfilesTreeUpdater.forCommandEnvironment(env);
for (ConfiguredTarget target : topLevelTargets) {
FilesToRunProvider provider = target.getProvider(FilesToRunProvider.class);
RunfilesSupport runfilesSupport = provider == null ? null : provider.getRunfilesSupport();
@@ -481,7 +478,8 @@
env,
runfilesSupport,
env.getSkyframeExecutor()
- .getConfiguration(env.getReporter(), target.getConfigurationKey()));
+ .getConfiguration(env.getReporter(), target.getConfigurationKey()),
+ runfilesTreeUpdater);
if (target == targetToRun) {
targetToRunRunfilesDir = runfilesDir;
targetToRunRunfilesSupport = runfilesSupport;
@@ -935,9 +933,9 @@
private static Path ensureRunfilesBuilt(
CommandEnvironment env,
RunfilesSupport runfilesSupport,
- BuildConfigurationValue configuration)
+ BuildConfigurationValue configuration,
+ RunfilesTreeUpdater runfilesTreeUpdater)
throws RunfilesException, InterruptedException {
- Artifact manifest = Preconditions.checkNotNull(runfilesSupport.getRunfilesManifest());
PathFragment runfilesDir = runfilesSupport.getRunfilesDirectoryExecPath();
Path workingDir = env.getExecRoot().getRelative(runfilesDir);
// On Windows, runfiles tree is disabled.
@@ -962,22 +960,10 @@
e);
}
- // When runfiles are not generated, getManifest() returns the
- // .runfiles_manifest file, otherwise it returns the MANIFEST file. This is
- // a handy way to check whether runfiles were built or not.
- if (!RUNFILES_MANIFEST.matches(manifest.getFilename())) {
- return workingDir;
- }
-
- SymlinkTreeHelper helper =
- new SymlinkTreeHelper(manifest.getPath(), runfilesSupport.getRunfilesDirectory(), false);
try {
- helper.createSymlinksUsingCommand(
- env.getExecRoot(),
- env.getBlazeWorkspace().getBinTools(),
- /* shellEnvironment= */ ImmutableMap.of(),
- /* outErr= */ null);
- } catch (EnvironmentalExecException e) {
+ runfilesTreeUpdater.updateRunfiles(
+ runfilesSupport, /* env= */ ImmutableMap.of(), /* outErr= */ null);
+ } catch (ExecException | IOException e) {
throw new RunfilesException(
"Failed to create runfiles symlinks: " + e.getMessage(),
Code.RUNFILES_SYMLINKS_CREATION_FAILURE,
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 393d43c..9477752 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
@@ -43,7 +43,8 @@
BinTools binTools =
BinTools.forUnitTesting(execRoot, ImmutableList.of(SymlinkTreeHelper.BUILD_RUNFILES));
Command command =
- new SymlinkTreeHelper(inputManifestPath, execRoot.getRelative("output/MANIFEST"), false)
+ new SymlinkTreeHelper(
+ inputManifestPath, execRoot.getRelative("output/MANIFEST"), false, "__main__")
.createCommand(execRoot, binTools, ImmutableMap.of());
assertThat(command.getEnvironment()).isEmpty();
assertThat(command.getWorkingDirectory()).isEqualTo(execRoot.getPathFile());
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 3c94ee7..73244e2 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 @@
StoredEventHandler eventHandler = new StoredEventHandler();
when(context.getContext(SymlinkTreeActionContext.class))
- .thenReturn(new SymlinkTreeStrategy(outputService, null));
+ .thenReturn(new SymlinkTreeStrategy(outputService, null, "__main__"));
when(context.getInputPath(any())).thenAnswer((i) -> ((Artifact) i.getArgument(0)).getPath());
when(context.getPathResolver()).thenReturn(ArtifactPathResolver.IDENTITY);
when(context.getEventHandler()).thenReturn(eventHandler);
@@ -130,7 +130,7 @@
StoredEventHandler eventHandler = new StoredEventHandler();
when(context.getContext(SymlinkTreeActionContext.class))
- .thenReturn(new SymlinkTreeStrategy(outputService, null));
+ .thenReturn(new SymlinkTreeStrategy(outputService, 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 be7cb4c..5fa63e9 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,7 @@
this.execRoot = execRoot;
addContext(FileWriteActionContext.class, new FileWriteStrategy());
addContext(TemplateExpansionContext.class, new LocalTemplateExpansionStrategy());
- addContext(SymlinkTreeActionContext.class, new SymlinkTreeStrategy(null, binTools));
+ addContext(SymlinkTreeActionContext.class, new SymlinkTreeStrategy(null, binTools, "__main__"));
addContext(SpawnStrategyResolver.class, new SpawnStrategyResolver());
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
index b2b5d56..8a4d43c 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -2500,6 +2500,11 @@
public boolean isBuildRunfileLinks() {
return false;
}
+
+ @Override
+ public String getWorkspaceName() {
+ return "__main__";
+ }
};
return new RunfilesSupplier() {
@Override
diff --git a/src/test/py/bazel/runfiles_test.py b/src/test/py/bazel/runfiles_test.py
index 0719e70..c215f80 100644
--- a/src/test/py/bazel/runfiles_test.py
+++ b/src/test/py/bazel/runfiles_test.py
@@ -339,6 +339,145 @@
self.assertEqual(stdout[0], "Hello, Bazel!")
self.assertEqual(stdout[1], "Hello, World!")
+ def setUpRunfilesDirectoryIncrementalityTest(self):
+ self.ScratchFile("MODULE.bazel")
+ self.ScratchFile(
+ "BUILD",
+ [
+ "sh_test(",
+ " name = 'test',",
+ " srcs = ['test.sh'],",
+ " data = ['data.txt'],",
+ ")",
+ ],
+ )
+ self.ScratchFile("data.txt")
+ self.ScratchFile("test.sh", ["[[ -f data.txt ]]"], executable=True)
+ self.ScratchFile(
+ ".bazelrc",
+ [
+ "startup --nowindows_enable_symlinks",
+ "common --spawn_strategy=local",
+ ],
+ )
+
+ def testRunfilesDirectoryIncrementalityEnableRunfilesFlippedOn(self):
+ self.setUpRunfilesDirectoryIncrementalityTest()
+
+ exit_code, _, _ = self.RunBazel(
+ ["test", ":test", "--noenable_runfiles"], allow_failure=True
+ )
+ self.assertEqual(exit_code, 3)
+ exit_code, _, _ = self.RunBazel(["test", ":test", "--enable_runfiles"])
+ self.assertEqual(exit_code, 0)
+
+ def testRunfilesDirectoryIncrementalityEnableRunfilesFlippedOff(self):
+ self.setUpRunfilesDirectoryIncrementalityTest()
+
+ exit_code, _, _ = self.RunBazel(["test", ":test", "--enable_runfiles"])
+ self.assertEqual(exit_code, 0)
+ exit_code, _, _ = self.RunBazel(
+ ["test", ":test", "--noenable_runfiles"], allow_failure=True
+ )
+ self.assertEqual(exit_code, 3)
+
+ def testRunfilesDirectoryIncrementalityNoBuildRunfileLinksEnableRunfilesFlippedOn(
+ self,
+ ):
+ self.setUpRunfilesDirectoryIncrementalityTest()
+
+ exit_code, _, _ = self.RunBazel(
+ ["test", ":test", "--nobuild_runfile_links", "--noenable_runfiles"],
+ allow_failure=True,
+ )
+ self.assertEqual(exit_code, 3)
+ exit_code, _, _ = self.RunBazel(
+ ["test", ":test", "--nobuild_runfile_links", "--enable_runfiles"]
+ )
+ self.assertEqual(exit_code, 0)
+
+ def testRunfilesDirectoryIncrementalityNoBuildRunfileLinksEnableRunfilesFlippedOff(
+ self,
+ ):
+ self.setUpRunfilesDirectoryIncrementalityTest()
+
+ exit_code, _, _ = self.RunBazel(
+ ["test", ":test", "--nobuild_runfile_links", "--enable_runfiles"]
+ )
+ self.assertEqual(exit_code, 0)
+ exit_code, _, _ = self.RunBazel(
+ ["test", ":test", "--nobuild_runfile_links", "--noenable_runfiles"],
+ allow_failure=True,
+ )
+ self.assertEqual(exit_code, 3)
+
+ def testRunfilesDirectoryIncrementalityEnableRunfilesFlippedOnRun(self):
+ self.setUpRunfilesDirectoryIncrementalityTest()
+
+ exit_code, _, _ = self.RunBazel(
+ ["run", ":test", "--noenable_runfiles"], allow_failure=True
+ )
+ self.assertNotEqual(exit_code, 0)
+ exit_code, _, _ = self.RunBazel(["run", ":test", "--enable_runfiles"])
+ self.assertEqual(exit_code, 0)
+
+ def testRunfilesDirectoryIncrementalityEnableRunfilesFlippedOffRun(self):
+ self.setUpRunfilesDirectoryIncrementalityTest()
+
+ exit_code, _, _ = self.RunBazel(["run", ":test", "--enable_runfiles"])
+ self.assertEqual(exit_code, 0)
+ exit_code, _, _ = self.RunBazel(
+ ["run", ":test", "--noenable_runfiles"], allow_failure=True
+ )
+ self.assertNotEqual(exit_code, 0)
+
+ def testRunfilesDirectoryIncrementalityNoBuildRunfileLinksEnableRunfilesFlippedOnRun(
+ self,
+ ):
+ self.setUpRunfilesDirectoryIncrementalityTest()
+
+ exit_code, _, _ = self.RunBazel(
+ ["run", ":test", "--nobuild_runfile_links", "--noenable_runfiles"],
+ allow_failure=True,
+ )
+ self.assertNotEqual(exit_code, 0)
+ exit_code, _, _ = self.RunBazel(
+ ["run", ":test", "--nobuild_runfile_links", "--enable_runfiles"]
+ )
+ self.assertEqual(exit_code, 0)
+
+ def testRunfilesDirectoryIncrementalityNoBuildRunfileLinksEnableRunfilesFlippedOffRun(
+ self,
+ ):
+ self.setUpRunfilesDirectoryIncrementalityTest()
+
+ exit_code, _, _ = self.RunBazel(
+ ["run", ":test", "--nobuild_runfile_links", "--enable_runfiles"]
+ )
+ self.assertEqual(exit_code, 0)
+ exit_code, _, _ = self.RunBazel(
+ ["run", ":test", "--nobuild_runfile_links", "--noenable_runfiles"],
+ allow_failure=True,
+ )
+ self.assertNotEqual(exit_code, 0)
+
+ def testTestsRunWithNoBuildRunfileLinksAndNoEnableRunfiles(self):
+ self.ScratchFile("MODULE.bazel")
+ self.ScratchFile(
+ "BUILD",
+ [
+ "sh_test(",
+ " name = 'test',",
+ " srcs = ['test.sh'],",
+ ")",
+ ],
+ )
+ self.ScratchFile("test.sh", executable=True)
+ self.ScratchFile(".bazelrc", ["common --spawn_strategy=local"])
+ self.RunBazel(
+ ["test", ":test", "--nobuild_runfile_links", "--noenable_runfiles"]
+ )
+
if __name__ == "__main__":
absltest.main()
diff --git a/src/test/shell/integration/python_stub_test.sh b/src/test/shell/integration/python_stub_test.sh
index 49bf0bf..de3f1e3 100755
--- a/src/test/shell/integration/python_stub_test.sh
+++ b/src/test/shell/integration/python_stub_test.sh
@@ -142,8 +142,9 @@
touch python_through_bash/inner.py
- bazel run --nobuild_runfile_links //python_through_bash:outer \
- &> $TEST_log || fail "bazel run failed"
+ # The inner Python script requires runfiles, so force them on Windows.
+ bazel run --nobuild_runfile_links --enable_runfiles \
+ //python_through_bash:outer &> $TEST_log || fail "bazel run failed"
expect_log "I am Python"
}
