Allow all characters in runfile source and target paths

Adds support for spaces and newlines in source and target paths of runfiles symlinks to `build-runfiles` as well as to the Bash, C++, and Java runfiles libraries (the Python runfiles library has moved out of the repo).

If a symlink has spaces or newlines in its source path, it is prefixed with a space in the manifest and spaces, newlines, and backslashes in the source path are escaped with `\s`, `\n`, and `\b` respectively. This scheme has been chosen as it has the following properties:
1. There is no change to the format of manifest lines for entries whose source and target paths don't contain a space. This ensures compatibility with existing runfiles libraries.
2. There is even no change to the format of manifest lines for entries whose target path contains spaces but whose source path does not. These manifests previously failed in `build-runfiles`, but were usable on Windows and supported by the official runfiles libraries. This also ensures that the initialization snippet for the Bash runfiles library doesn't need to change, even when used on Unix with a space in the output base path.
3. The scheme is fully reversible and only depends on the source path, which gives runfiles libraries a choice between reversing the escaping when parsing the manifest (C++, Java) or applying the escaping when searching the manifest (Bash).

Fixes bazelbuild#4327

RELNOTES: Bazel now supports all characters in the rlocation and target paths of runfiles and can be run from workspaces with a space in their full path.

Closes bazelbuild#23331.

PiperOrigin-RevId: 683362776
Change-Id: I1eb79217dcd53cef0089d62a7ba477b1d8f52c21

(cherry picked from commit 7407cef)

src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java

@@ -19,6 +19,8 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
+import com.google.common.escape.CharEscaperBuilder;
+import com.google.common.escape.Escaper;
 import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
@@ -61,7 +63,16 @@ public final class SourceManifestAction extends AbstractFileWriteAction
   private static final String GUID = "07459553-a3d0-4d37-9d78-18ed942470f4";
 
   private static final Comparator<Map.Entry<PathFragment, Artifact>> ENTRY_COMPARATOR =
-      (path1, path2) -> path1.getKey().getPathString().compareTo(path2.getKey().getPathString());
+      Comparator.comparing(path -> path.getKey().getPathString());
+  private static final Escaper ROOT_RELATIVE_PATH_ESCAPER =
+      new CharEscaperBuilder()
+          .addEscape(' ', "\\s")
+          .addEscape('\n', "\\n")
+          .addEscape('\\', "\\b")
+          .toEscaper();
+  private static final Escaper TARGET_PATH_ESCAPER =
+      new CharEscaperBuilder().addEscape('\n', "\\n").addEscape('\\', "\\b").toEscaper();
+
   private final Artifact repoMappingManifest;
   /**
    * Interface for defining manifest formatting and reporting specifics. Implementations must be
@@ -291,6 +302,9 @@ public enum ManifestType implements ManifestWriter {
      *
      * <p>[rootRelativePath] [resolvingSymlink]
      *
+     * <p>If rootRelativePath contains spaces, then each backslash is replaced with '\b', each space
+     * is replaced with '\s' and the line is prefixed with a space.
+     *
      * <p>This strategy is suitable for creating an input manifest to a source view tree. Its output
      * is a valid input to {@link com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction}.
      */
@@ -301,11 +315,32 @@ public void writeEntry(
           PathFragment rootRelativePath,
           @Nullable PathFragment symlinkTarget)
           throws IOException {
-        manifestWriter.append(rootRelativePath.getPathString());
+        String rootRelativePathString = rootRelativePath.getPathString();
+        // Source paths with spaces require escaping. Target paths with spaces don't as consumers
+        // are expected to split on the first space. Newlines always need to be escaped.
+        // Note that if any of these characters are present, then we also need to escape the escape
+        // character (backslash) in both paths. We avoid doing so if none of the problematic
+        // characters are present for backwards compatibility with existing runfiles libraries. In
+        // particular, entries with a source path that contains neither spaces nor newlines and
+        // target paths that contain both spaces and backslashes require no escaping.
+        boolean needsEscaping =
+            rootRelativePathString.indexOf(' ') != -1
+                || rootRelativePathString.indexOf('\n') != -1
+                || (symlinkTarget != null && symlinkTarget.getPathString().indexOf('\n') != -1);
+        if (needsEscaping) {
+          manifestWriter.append(' ');
+          manifestWriter.append(ROOT_RELATIVE_PATH_ESCAPER.escape(rootRelativePathString));
+        } else {
+          manifestWriter.append(rootRelativePathString);
+        }
         // This trailing whitespace is REQUIRED to process the single entry line correctly.
         manifestWriter.append(' ');
         if (symlinkTarget != null) {
-          manifestWriter.append(symlinkTarget.getPathString());
+          if (needsEscaping) {
+            manifestWriter.append(TARGET_PATH_ESCAPER.escape(symlinkTarget.getPathString()));
+          } else {
+            manifestWriter.append(symlinkTarget.getPathString());
+          }
         }
         manifestWriter.append('\n');
       }

src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java

@@ -164,18 +164,6 @@ private DetailedExitCode checkCwdInWorkspace(EventHandler eventHandler) {
     }
 
     Path workspacePath = workspace.getWorkspace();
-    // TODO(kchodorow): Remove this once spaces are supported.
-    if (workspacePath.getPathString().contains(" ")) {
-      String message =
-          runtime.getProductName()
-              + " does not currently work properly from paths "
-              + "containing spaces ("
-              + workspacePath
-              + ").";
-      eventHandler.handle(Event.error(message));
-      return createDetailedExitCode(message, Code.SPACES_IN_WORKSPACE_PATH);
-    }
-
     if (workspacePath.getParentDirectory() != null) {
       Path doNotBuild =
           workspacePath.getParentDirectory().getRelative(BlazeWorkspace.DO_NOT_BUILD_FILE_NAME);

src/main/protobuf/failure_details.proto

@@ -566,7 +566,7 @@ message Command {
     STARLARK_OPTIONS_PARSE_FAILURE = 10 [(metadata) = { exit_code: 2 }];
     ARGUMENTS_NOT_RECOGNIZED = 11 [(metadata) = { exit_code: 2 }];
     NOT_IN_WORKSPACE = 12 [(metadata) = { exit_code: 2 }];
-    SPACES_IN_WORKSPACE_PATH = 13 [(metadata) = { exit_code: 36 }];
+    reserved 13;
     IN_OUTPUT_DIRECTORY = 14 [(metadata) = { exit_code: 2 }];
   }
 

src/main/tools/build-runfiles-windows.cc

@@ -100,11 +100,6 @@ wstring GetParentDirFromPath(const wstring& path) {
   return path.substr(0, path.find_last_of(L"\\/"));
 }
 
-inline void Trim(wstring& str) {
-  str.erase(0, str.find_first_not_of(' '));
-  str.erase(str.find_last_not_of(' ') + 1);
-}
-
 bool ReadSymlink(const wstring& abs_path, wstring* target, wstring* error) {
   switch (bazel::windows::ReadSymlinkOrJunction(abs_path, target, error)) {
     case bazel::windows::ReadSymlinkOrJunctionResult::kSuccess:
@@ -129,6 +124,39 @@ bool ReadSymlink(const wstring& abs_path, wstring* target, wstring* error) {
   return false;
 }
 
+// Replaces \s, \n, and \b with their respective characters.
+std::string Unescape(const std::string& path) {
+  std::string result;
+  result.reserve(path.size());
+  for (size_t i = 0; i < path.size(); ++i) {
+    if (path[i] == '\\' && i + 1 < path.size()) {
+      switch (path[i + 1]) {
+        case 's': {
+          result.push_back(' ');
+          break;
+        }
+        case 'n': {
+          result.push_back('\n');
+          break;
+        }
+        case 'b': {
+          result.push_back('\\');
+          break;
+        }
+        default: {
+          result.push_back(path[i]);
+          result.push_back(path[i + 1]);
+          break;
+        }
+      }
+      ++i;
+    } else {
+      result.push_back(path[i]);
+    }
+  }
+  return result;
+}
+
 }  // namespace
 
 class RunfilesCreator {
@@ -164,21 +192,27 @@ class RunfilesCreator {
         continue;
       }
 
-      size_t space_pos = line.find_first_of(' ');
-      wstring wline = blaze_util::CstringToWstring(line);
-      wstring link, target;
-      if (space_pos == string::npos) {
-        link = wline;
-        target = wstring();
+      wstring link;
+      wstring target;
+      if (!line.empty() && line[0] == ' ') {
+        // The link path contains escape sequences for spaces and backslashes.
+        string::size_type idx = line.find(' ', 1);
+        if (idx == string::npos) {
+          die(L"Missing separator in manifest line: %hs", line.c_str());
+        }
+        std::string link_path = Unescape(line.substr(1, idx - 1));
+        link = blaze_util::CstringToWstring(link_path);
+        std::string target_path = Unescape(line.substr(idx + 1));
+        target = blaze_util::CstringToWstring(target_path);
       } else {
-        link = wline.substr(0, space_pos);
-        target = wline.substr(space_pos + 1);
+        string::size_type idx = line.find(' ');
+        if (idx == string::npos) {
+          die(L"Missing separator in manifest line: %hs", line.c_str());
+        }
+        link = blaze_util::CstringToWstring(line.substr(0, idx));
+        target = blaze_util::CstringToWstring(line.substr(idx + 1));
       }
 
-      // Removing leading and trailing spaces
-      Trim(link);
-      Trim(target);
-
       // We sometimes need to create empty files under the runfiles tree.
       // For example, for python binary, __init__.py is needed under every
       // directory. Storing an entry with an empty target indicates we need to

src/main/tools/build-runfiles.cc

@@ -103,6 +103,39 @@ struct FileInfo {
 
 typedef std::map<std::string, FileInfo> FileInfoMap;
 
+// Replaces \s, \n, and \b with their respective characters.
+std::string Unescape(const std::string &path) {
+  std::string result;
+  result.reserve(path.size());
+  for (size_t i = 0; i < path.size(); ++i) {
+    if (path[i] == '\\' && i + 1 < path.size()) {
+      switch (path[i + 1]) {
+        case 's': {
+          result.push_back(' ');
+          break;
+        }
+        case 'n': {
+          result.push_back('\n');
+          break;
+        }
+        case 'b': {
+          result.push_back('\\');
+          break;
+        }
+        default: {
+          result.push_back(path[i]);
+          result.push_back(path[i + 1]);
+          break;
+        }
+      }
+      ++i;
+    } else {
+      result.push_back(path[i]);
+    }
+  }
+  return result;
+}
+
 class RunfilesCreator {
  public:
   explicit RunfilesCreator(const std::string &output_base)
@@ -157,15 +190,26 @@ class RunfilesCreator {
       if (buf[0] ==  '/') {
         DIE("paths must not be absolute: line %d: '%s'\n", lineno, buf);
       }
-      const char *s = strchr(buf, ' ');
-      if (!s) {
-        DIE("missing field delimiter at line %d: '%s'\n", lineno, buf);
-      } else if (strchr(s+1, ' ')) {
-        DIE("link or target filename contains space on line %d: '%s'\n",
-            lineno, buf);
+      std::string link;
+      std::string target;
+      if (buf[0] == ' ') {
+        // The link path contains escape sequences for spaces and backslashes.
+        char *s = strchr(buf + 1, ' ');
+        if (!s) {
+          DIE("missing field delimiter at line %d: '%s'\n", lineno, buf);
+        }
+        link = Unescape(std::string(buf + 1, s));
+        target = Unescape(s + 1);
+      } else {
+        // The line is of the form "foo /target/path", with only a single space
+        // in the link path.
+        const char *s = strchr(buf, ' ');
+        if (!s) {
+          DIE("missing field delimiter at line %d: '%s'\n", lineno, buf);
+        }
+        link = std::string(buf, s - buf);
+        target = s + 1;
       }
-      std::string link(buf, s-buf);
-      const char *target = s+1;
       if (!allow_relative && target[0] != '\0' && target[0] != '/'
           && target[1] != ':') {  // Match Windows paths, e.g. C:\foo or C:/foo.
         DIE("expected absolute path at line %d: '%s'\n", lineno, buf);

src/test/java/com/google/devtools/build/lib/analysis/BUILD

@@ -340,10 +340,10 @@ java_test(
     srcs = ["SourceManifestActionTest.java"],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
-        "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/util",
+        "//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",
         "//src/test/java/com/google/devtools/build/lib/actions/util",

src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.util.Fingerprint;
+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;
@@ -388,6 +389,78 @@ public void testUnresolvedSymlink() throws Exception {
             """);
   }
 
+  @Test
+  public void testEscaping() throws Exception {
+    Artifact manifest = getBinArtifactWithNoOwner("manifest1");
+
+    ArtifactRoot trivialRoot =
+        ArtifactRoot.asSourceRoot(Root.fromPath(rootDirectory.getRelative("trivial")));
+    Path fileWithSpaceAndBackslashPath = scratch.file("trivial/file with sp\\ace", "foo");
+    Artifact fileWithSpaceAndBackslash =
+        ActionsTestUtil.createArtifact(trivialRoot, fileWithSpaceAndBackslashPath);
+    Path fileWithNewlineAndBackslashPath = scratch.file("trivial/file\nwith\\newline", "foo");
+    Artifact fileWithNewlineAndBackslash =
+        ActionsTestUtil.createArtifact(trivialRoot, fileWithNewlineAndBackslashPath);
+
+    SourceManifestAction action =
+        new SourceManifestAction(
+            ManifestType.SOURCE_SYMLINKS,
+            NULL_ACTION_OWNER,
+            manifest,
+            new Runfiles.Builder("TESTING", false)
+                .addSymlink(PathFragment.create("no/sp\\ace"), buildFile)
+                .addSymlink(PathFragment.create("also/no/sp\\ace"), fileWithSpaceAndBackslash)
+                .addSymlink(PathFragment.create("still/no/sp\\ace"), fileWithNewlineAndBackslash)
+                .addSymlink(PathFragment.create("with sp\\ace"), buildFile)
+                .addSymlink(PathFragment.create("also/with sp\\ace"), fileWithSpaceAndBackslash)
+                .addSymlink(PathFragment.create("more/with sp\\ace"), fileWithNewlineAndBackslash)
+                .addSymlink(PathFragment.create("with\nnew\\line"), buildFile)
+                .addSymlink(PathFragment.create("also/with\nnewline"), fileWithSpaceAndBackslash)
+                .addSymlink(PathFragment.create("more/with\nnewline"), fileWithNewlineAndBackslash)
+                .addSymlink(PathFragment.create("with\nnew\\line and space"), buildFile)
+                .addSymlink(
+                    PathFragment.create("also/with\nnewline and space"), fileWithSpaceAndBackslash)
+                .addSymlink(
+                    PathFragment.create("more/with\nnewline and space"),
+                    fileWithNewlineAndBackslash)
+                .build());
+    if (OS.getCurrent().equals(OS.WINDOWS)) {
+      assertThat(action.getFileContents(reporter))
+          .isEqualTo(
+              """
+              TESTING/also/no/sp/ace /workspace/trivial/file with sp/ace
+               TESTING/also/with\\nnewline /workspace/trivial/file with sp/ace
+               TESTING/also/with\\nnewline\\sand\\sspace /workspace/trivial/file with sp/ace
+               TESTING/also/with\\ssp/ace /workspace/trivial/file with sp/ace
+               TESTING/more/with\\nnewline /workspace/trivial/file\\nwith/newline
+               TESTING/more/with\\nnewline\\sand\\sspace /workspace/trivial/file\\nwith/newline
+               TESTING/more/with\\ssp/ace /workspace/trivial/file\\nwith/newline
+              TESTING/no/sp/ace /workspace/trivial/BUILD
+               TESTING/still/no/sp/ace /workspace/trivial/file\\nwith/newline
+               TESTING/with\\nnew/line /workspace/trivial/BUILD
+               TESTING/with\\nnew/line\\sand\\sspace /workspace/trivial/BUILD
+               TESTING/with\\ssp/ace /workspace/trivial/BUILD
+              """);
+    } else {
+      assertThat(action.getFileContents(reporter))
+          .isEqualTo(
+              """
+              TESTING/also/no/sp\\ace /workspace/trivial/file with sp\\ace
+               TESTING/also/with\\nnewline /workspace/trivial/file with sp\\bace
+               TESTING/also/with\\nnewline\\sand\\sspace /workspace/trivial/file with sp\\bace
+               TESTING/also/with\\ssp\\bace /workspace/trivial/file with sp\\bace
+               TESTING/more/with\\nnewline /workspace/trivial/file\\nwith\\bnewline
+               TESTING/more/with\\nnewline\\sand\\sspace /workspace/trivial/file\\nwith\\bnewline
+               TESTING/more/with\\ssp\\bace /workspace/trivial/file\\nwith\\bnewline
+              TESTING/no/sp\\ace /workspace/trivial/BUILD
+               TESTING/still/no/sp\\bace /workspace/trivial/file\\nwith\\bnewline
+               TESTING/with\\nnew\\bline /workspace/trivial/BUILD
+               TESTING/with\\nnew\\bline\\sand\\sspace /workspace/trivial/BUILD
+               TESTING/with\\ssp\\bace /workspace/trivial/BUILD
+              """);
+    }
+  }
+
   private String computeKey(SourceManifestAction action) {
     Fingerprint fp = new Fingerprint();
     action.computeKey(actionKeyContext, /* artifactExpander= */ null, fp);

src/test/shell/bazel/bazel_determinism_test.sh

@@ -61,7 +61,8 @@ function hash_outputs() {
 }
 
 function test_determinism()  {
-    local workdir="${TEST_TMPDIR}/workdir"
+    # Verify that Bazel can build itself under a path with spaces.
+    local workdir="${TEST_TMPDIR}/work dir"
     mkdir "${workdir}" || fail "Could not create work directory"
     cd "${workdir}" || fail "Could not change to work directory"
     unzip -q "${DISTFILE}"

src/test/shell/bazel/workspace_test.sh

@@ -73,7 +73,7 @@ function test_path_with_spaces() {
   cd "$ws"
   create_workspace_with_default_repos WORKSPACE
 
-  bazel info &> $TEST_log && fail "Info succeeeded"
+  bazel info &> $TEST_log || fail "Info failed"
   bazel help &> $TEST_log || fail "Help failed"
 }