Allow all characters in runfile source and target paths

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;
@@ -62,7 +64,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

@@ -166,18 +166,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

@@ -596,7 +596,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

@@ -309,6 +309,7 @@ java_test(
         "//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;
@@ -387,6 +388,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"
   touch WORKSPACE
 
-  bazel info &> $TEST_log && fail "Info succeeeded"
+  bazel info &> $TEST_log || fail "Info failed"
   bazel help &> $TEST_log || fail "Help failed"
 }