[7.4.0] Allow all characters in runfile source and target paths (#23912)

(contains the refactor in f311efc to
reduce merge conflicts)

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 #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 #23331.

PiperOrigin-RevId: 683362776
Change-Id: I1eb79217dcd53cef0089d62a7ba477b1d8f52c21

(cherry picked from commits
7407cef
and f311efc)

Closes #23715

---------

Co-authored-by: Googler <[email protected]>

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
@@ -75,10 +86,11 @@ interface ManifestWriter {
      *
      * @param manifestWriter the output stream
      * @param rootRelativePath path of an entry relative to the manifest's root
-     * @param symlink (optional) symlink that resolves the above path
+     * @param symlinkTarget target of the entry at {@code rootRelativePath} if it is a symlink,
+     *     otherwise {@code null}
      */
     void writeEntry(
-        Writer manifestWriter, PathFragment rootRelativePath, @Nullable Artifact symlink)
+        Writer manifestWriter, PathFragment rootRelativePath, @Nullable PathFragment symlinkTarget)
         throws IOException;
 
     /** Fulfills {@link com.google.devtools.build.lib.actions.AbstractAction#getMnemonic()} */
@@ -234,7 +246,16 @@ private void writeFile(OutputStream out, Map<PathFragment, Artifact> output) thr
     List<Map.Entry<PathFragment, Artifact>> sortedManifest = new ArrayList<>(output.entrySet());
     sortedManifest.sort(ENTRY_COMPARATOR);
     for (Map.Entry<PathFragment, Artifact> line : sortedManifest) {
-      manifestWriter.writeEntry(manifestFile, line.getKey(), line.getValue());
+      Artifact artifact = line.getValue();
+      PathFragment symlinkTarget;
+      if (artifact == null) {
+        symlinkTarget = null;
+      } else if (artifact.isSymlink()) {
+        symlinkTarget = artifact.getPath().readSymbolicLink();
+      } else {
+        symlinkTarget = artifact.getPath().asFragment();
+      }
+      manifestWriter.writeEntry(manifestFile, line.getKey(), symlinkTarget);
     }
 
     manifestFile.flush();
@@ -281,21 +302,44 @@ 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}.
      */
     SOURCE_SYMLINKS {
       @Override
-      public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, Artifact symlink)
+      public void writeEntry(
+          Writer manifestWriter,
+          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 (symlink != null) {
-          if (symlink.isSymlink()) {
-            manifestWriter.append(symlink.getPath().readSymbolicLink().getPathString());
+        if (symlinkTarget != null) {
+          if (needsEscaping) {
+            manifestWriter.append(TARGET_PATH_ESCAPER.escape(symlinkTarget.getPathString()));
           } else {
-            manifestWriter.append(symlink.getPath().getPathString());
+            manifestWriter.append(symlinkTarget.getPathString());
           }
         }
         manifestWriter.append('\n');
@@ -334,7 +378,10 @@ public boolean emitsAbsolutePaths() {
      */
     SOURCES_ONLY {
       @Override
-      public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, Artifact symlink)
+      public void writeEntry(
+          Writer manifestWriter,
+          PathFragment rootRelativePath,
+          @Nullable PathFragment symlinkTarget)
           throws IOException {
         manifestWriter.append(rootRelativePath.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",