Delete the runfiles symlink tree left over by a previous --enable_run…

…files build when building with --noenable_runfiles.

Also add profiler spans around potentially expensive operations.

Fixes bazelbuild#19333.

PiperOrigin-RevId: 560075122
Change-Id: Ic474971c62b6099c416012bfa5eeea730fa372a4

src/main/java/com/google/devtools/build/lib/exec/BUILD

@@ -401,6 +401,7 @@ java_library(
         "//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:fileset_output_symlink",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util:command",

src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java

@@ -23,6 +23,8 @@
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
+import com.google.devtools.build.lib.profiler.Profiler;
+import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.server.FailureDetails.Execution;
 import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
@@ -77,7 +79,6 @@ private Path getOutputManifest() {
   /** Creates a symlink tree by making VFS calls. */
   public void createSymlinksDirectly(Path symlinkTreeRoot, Map<PathFragment, Artifact> symlinks)
       throws IOException {
-    Preconditions.checkState(!filesetTree);
     // Our strategy is to minimize mutating file system operations as much as possible. Ideally, if
     // there is an existing symlink tree with the expected contents, we don't make any changes. Our
     // algorithm goes as follows:
@@ -108,13 +109,25 @@ public void createSymlinksDirectly(Path symlinkTreeRoot, Map<PathFragment, Artif
     //
     // 3. For every remaining entry in the node, create the corresponding file, symlink, or
     //    directory on disk. If it is a directory, recurse into that directory.
-    Directory root = new Directory();
-    for (Map.Entry<PathFragment, Artifact> entry : symlinks.entrySet()) {
-      // This creates intermediate directory nodes as a side effect.
-      Directory parentDir = root.walk(entry.getKey().getParentDirectory());
-      parentDir.addSymlink(entry.getKey().getBaseName(), entry.getValue());
+    try (SilentCloseable c = Profiler.instance().profile("Create symlink tree in-process")) {
+      Preconditions.checkState(!filesetTree);
+      Directory root = new Directory();
+      for (Map.Entry<PathFragment, Artifact> entry : symlinks.entrySet()) {
+        // This creates intermediate directory nodes as a side effect.
+        Directory parentDir = root.walk(entry.getKey().getParentDirectory());
+        parentDir.addSymlink(entry.getKey().getBaseName(), entry.getValue());
+      }
+      root.syncTreeRecursively(symlinkTreeRoot);
+    }
+  }
+
+  /** Deletes the contents of the runfiles directory. */
+  public void clearRunfilesDirectory() throws ExecException {
+    try (SilentCloseable c = Profiler.instance().profile("Clear symlink tree")) {
+      symlinkTreeRoot.deleteTreesBelow();
+    } catch (IOException e) {
+      throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_DELETION_IO_EXCEPTION);
     }
-    root.syncTreeRecursively(symlinkTreeRoot);
   }
 
   /** Copies the input manifest to the output manifest. */
@@ -139,21 +152,23 @@ public void copyManifest() throws ExecException {
   public void createSymlinksUsingCommand(
       Path execRoot, BinTools binTools, Map<String, String> shellEnvironment, OutErr outErr)
       throws EnvironmentalExecException, InterruptedException {
-    Command command = createCommand(execRoot, binTools, shellEnvironment);
-    try {
-      if (outErr != null) {
-        command.execute(outErr.getOutputStream(), outErr.getErrorStream());
-      } else {
-        command.execute();
+    try (SilentCloseable c = Profiler.instance().profile("Create symlink tree out-of-process")) {
+      Command command = createCommand(execRoot, binTools, shellEnvironment);
+      try {
+        if (outErr != null) {
+          command.execute(outErr.getOutputStream(), outErr.getErrorStream());
+        } else {
+          command.execute();
+        }
+      } catch (CommandException e) {
+        throw new EnvironmentalExecException(
+            e,
+            FailureDetail.newBuilder()
+                .setMessage(CommandUtils.describeCommandFailure(true, e))
+                .setExecution(
+                    Execution.newBuilder().setCode(Code.SYMLINK_TREE_CREATION_COMMAND_EXCEPTION))
+                .build());
       }
-    } catch (CommandException e) {
-      throw new EnvironmentalExecException(
-          e,
-          FailureDetail.newBuilder()
-              .setMessage(CommandUtils.describeCommandFailure(true, e))
-              .setExecution(
-                  Execution.newBuilder().setCode(Code.SYMLINK_TREE_CREATION_COMMAND_EXCEPTION))
-              .build());
     }
   }
 

src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java

@@ -102,7 +102,12 @@ public void createSymlinks(
 
           createOutput(action, actionExecutionContext, inputManifest);
         } else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.SKIP) {
-          createSymlinkTreeHelper(action, actionExecutionContext).copyManifest();
+          // 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.copyManifest();
         } else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.INTERNAL
             && !action.isFilesetTree()) {
           try {

src/main/protobuf/failure_details.proto

@@ -458,6 +458,7 @@ message Execution {
     UNEXPECTED_EXCEPTION = 37 [(metadata) = { exit_code: 1 }];
     reserved 38;
     SOURCE_INPUT_IO_EXCEPTION = 39 [(metadata) = { exit_code: 1 }];
+    SYMLINK_TREE_DELETION_IO_EXCEPTION = 40 [(metadata) = { exit_code: 36 }];
   }
 
   Code code = 1;

src/test/shell/bazel/runfiles_test.sh

@@ -115,30 +115,57 @@ EOF
     || fail "bar not recreated"
 }
 
+function test_enable_runfiles_change() {
+  create_workspace_with_default_repos WORKSPACE foo
+
+  mkdir data && echo "hello" > data/hello && echo "world" > data/world
+
+  touch bin.sh
+  chmod 755 bin.sh
+
+  cat > BUILD <<'EOF'
+sh_binary(
+  name = "bin",
+  srcs = ["bin.sh"],
+  data = glob(["data/*"]),
+)
+EOF
+
+  bazel build --enable_runfiles //:bin || fail "Building //:bin failed"
+
+  [[ -f bazel-bin/bin.runfiles/foo/data/hello ]] || fail "expected runfile data/hello"
+  [[ -f bazel-bin/bin.runfiles/foo/data/world ]] || fail "expected runfile data/world"
+  [[ -f bazel-bin/bin.runfiles/MANIFEST ]] || fail "expected output manifest to exist"
+
+  bazel build --noenable_runfiles //:bin || fail "Building //:bin failed"
+
+  [[ ! -f bazel-bin/bin.runfiles/foo/data/hello ]] || fail "expected no runfile data/hello"
+  [[ ! -f bazel-bin/bin.runfiles/foo/data/world ]] || fail "expected no runfile data/world"
+  [[ -f bazel-bin/bin.runfiles/MANIFEST ]] || fail "expected output manifest to exist"
+}
+
 # Test that the local strategy creates a runfiles tree during test if no --nobuild_runfile_links
 # is specified.
 function test_nobuild_runfile_links() {
-  mkdir data && echo "hello" > data/hello && echo "world" > data/world
   create_workspace_with_default_repos WORKSPACE foo
 
-cat > test.sh <<'EOF'
+  mkdir data && echo "hello" > data/hello && echo "world" > data/world
+
+  cat > test.sh <<'EOF'
 #!/bin/bash
 set -e
 [[ -f ${RUNFILES_DIR}/foo/data/hello ]]
 [[ -f ${RUNFILES_DIR}/foo/data/world ]]
 exit 0
 EOF
+
   chmod 755 test.sh
-  cat > BUILD <<'EOF'
-filegroup(
-  name = "runfiles",
-  srcs = ["data/hello", "data/world"],
-)
 
+  cat > BUILD <<'EOF'
 sh_test(
   name = "test",
   srcs = ["test.sh"],
-  data = [":runfiles"],
+  data = glob(["data/*"]),
 )
 EOF
 
@@ -150,7 +177,7 @@ EOF
   [[ ! -f bazel-bin/test.runfiles/MANIFEST ]] || fail "expected output manifest to not exist"
 
   bazel test --spawn_strategy=local --nobuild_runfile_links //:test \
-    || fail "Testing //:foo failed"
+    || fail "Testing //:test failed"
 
   [[ -f bazel-bin/test.runfiles/foo/data/hello ]] || fail "expected runfile data/hello to exist"
   [[ -f bazel-bin/test.runfiles/foo/data/world ]] || fail "expected runfile data/world to exist"
@@ -161,22 +188,26 @@ EOF
 # attempt to create the runfiles directory both for the target to run and the
 # --run_under target.
 function test_nobuild_runfile_links_with_run_under() {
-  mkdir data && echo "hello" > data/hello && echo "world" > data/world
   create_workspace_with_default_repos WORKSPACE foo
 
-cat > hello.sh <<'EOF'
+  mkdir data && echo "hello" > data/hello && echo "world" > data/world
+
+  cat > hello.sh <<'EOF'
 #!/bin/bash
 set -ex
 [[ -f $0.runfiles/foo/data/hello ]]
 exec "$@"
 EOF
-cat > world.sh <<'EOF'
+
+  cat > world.sh <<'EOF'
 #!/bin/bash
 set -ex
 [[ -f $0.runfiles/foo/data/world ]]
 exit 0
 EOF
+
   chmod 755 hello.sh world.sh
+
   cat > BUILD <<'EOF'
 sh_binary(
   name = "hello",
@@ -200,7 +231,7 @@ EOF
   [[ ! -f bazel-bin/world.runfiles/MANIFEST ]] || fail "expected output manifest world to not exist"
 
   bazel run --spawn_strategy=local --nobuild_runfile_links --run_under //:hello //:world \
-    || fail "Testing //:foo failed"
+    || fail "Running //:hello and //:world failed"
 
   [[ -f bazel-bin/hello.runfiles/foo/data/hello ]] || fail "expected runfile data/hello to exist"
   [[ -f bazel-bin/hello.runfiles/MANIFEST ]] || fail "expected output manifest hello to exist"