Allow spaces in runfile source and target paths

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

@@ -301,7 +301,16 @@ 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 (without escaping) or after the part indicated
+        // by the length prefix (with escaping).
+        if (rootRelativePathString.indexOf(' ') != -1) {
+          manifestWriter.append(' ');
+          manifestWriter.append(String.valueOf(rootRelativePathString.length()));
+          manifestWriter.append(' ');
+        }
+        manifestWriter.append(rootRelativePathString);
         // This trailing whitespace is REQUIRED to process the single entry line correctly.
         manifestWriter.append(' ');
         if (symlinkTarget != null) {

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

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

@@ -164,21 +164,31 @@ 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] == ' ') {
+        // Lines starting with a space are of the form " 7 foo bar /tar get/path", with
+        // the first field indicating the length of the runfiles path.
+        std::size_t length_field_end = line.find_first_of(' ', 1);
+        if (length_field_end == string::npos) {
+          die(L"Invalid length field: %hs", line.c_str());
+        }
+        std::size_t link_length = std::stoul(line.substr(1, length_field_end - 1));
+        std::size_t after_length_field = length_field_end + 1;
+        if (line.size() < after_length_field + link_length || line[after_length_field + link_length] != ' ') {
+          die(L"Invalid length field: %hs", line.c_str());
+        }
+        link = blaze_util::CstringToWstring(line.substr(after_length_field, link_length));
+        target = blaze_util::CstringToWstring(line.substr(after_length_field + link_length + 1));
       } else {
-        link = wline.substr(0, space_pos);
-        target = wline.substr(space_pos + 1);
+        string::size_type idx = line.find_first_of(' ');
+        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

@@ -157,15 +157,28 @@ 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;
+      const char *target;
+      if (buf[0] == ' ') {
+        // The line is of the form " 7 foo bar /tar get/path", with the first
+        // field indicating the length of the source path.
+        std::size_t length_field_length;
+        std::size_t link_length = std::stoul(buf+1, &length_field_length);
+        const char *after_length_field = buf + 1 + length_field_length + 1;
+        target = after_length_field + link_length + 1;
+        if (target >= buf + n || *(target - 1) != ' ') {
+          DIE("invalid length field at line %d: '%s'\n", lineno, buf);
+        }
+        link = std::string(after_length_field, link_length);
+      } else {
+        // The line is of the form "foo /target/path", with only a single space.
+        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/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"
 }
 

src/test/shell/integration/runfiles_test.sh

@@ -569,4 +569,74 @@ EOF
   assert_contains '/link_two$' *-bin/a/go
 }
 
+function test_spaces_in_runfiles_source_paths() {
+  mkdir -p pkg
+  cat > pkg/defs.bzl <<'EOF'
+def _spaces_impl(ctx):
+    out = ctx.actions.declare_file(" a b .txt")
+    ctx.actions.write(out, "my content")
+    return [DefaultInfo(files = depset([out]))]
+
+spaces = rule(
+    implementation = _spaces_impl,
+)
+EOF
+  cat > pkg/BUILD <<'EOF'
+load(":defs.bzl", "spaces")
+spaces(name = "spaces")
+sh_test(
+    name = "foo",
+    srcs = ["foo.sh"],
+    data = [":spaces"],
+)
+EOF
+  cat > pkg/foo.sh <<'EOF'
+#!/bin/bash
+if [[ "$(cat "pkg/ a b .txt")" != "my content" ]]; then
+  echo "unexpected content or not found"
+  exit 1
+fi
+EOF
+  chmod +x pkg/foo.sh
+
+  bazel test //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed"
+}
+
+function test_spaces_in_runfiles_source_and_target_paths() {
+  dir=$(mktemp -d 'runfiles test.XXXXXX')
+  cd "$dir" || fail "failed to cd to $dir"
+  touch MODULE.bazel
+
+  mkdir -p pkg
+  cat > pkg/defs.bzl <<'EOF'
+def _spaces_impl(ctx):
+    out = ctx.actions.declare_file(" a b .txt")
+    ctx.actions.write(out, "my content")
+    return [DefaultInfo(files = depset([out]))]
+
+spaces = rule(
+    implementation = _spaces_impl,
+)
+EOF
+  cat > pkg/BUILD <<'EOF'
+load(":defs.bzl", "spaces")
+spaces(name = "spaces")
+sh_test(
+    name = "foo",
+    srcs = ["foo.sh"],
+    data = [":spaces"],
+)
+EOF
+  cat > pkg/foo.sh <<'EOF'
+#!/bin/bash
+if [[ "$(cat "pkg/ a b .txt")" != "my content" ]]; then
+  echo "unexpected content or not found"
+  exit 1
+fi
+EOF
+  chmod +x pkg/foo.sh
+
+  bazel test //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed"
+}
+
 run_suite "runfiles"

tools/bash/runfiles/runfiles.bash

@@ -357,7 +357,19 @@ function runfiles_rlocation_checked() {
     if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
       echo >&2 "INFO[runfiles.bash]: rlocation($1): looking in RUNFILES_MANIFEST_FILE ($RUNFILES_MANIFEST_FILE)"
     fi
-    local -r result=$(__runfiles_maybe_grep -m1 "^$1 " "${RUNFILES_MANIFEST_FILE}" | cut -d ' ' -f 2-)
+    # If the rlocation path contains a space, it needs to be prefixed with
+    # " <length> ", where <length> is the length of the path in bytes.
+    if [[ "$1" == *" "* ]]; then
+      local search_prefix=" $(echo -n "$1" | wc -c | tr -d ' ') $1"
+      if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
+        echo >&2 "INFO[runfiles.bash]: rlocation($1): using escaped search prefix ($search_prefix)"
+      fi
+    else
+      local search_prefix="$1"
+    fi
+    # The extra space below is added because cut counts from 1.
+    local trim_length=$(echo -n "$search_prefix  " | wc -c)
+    local -r result=$(__runfiles_maybe_grep -m1 "^$search_prefix " "${RUNFILES_MANIFEST_FILE}" | cut -b ${trim_length}-)
     if [[ -z "$result" ]]; then
       # If path references a runfile that lies under a directory that itself
       # is a runfile, then only the directory is listed in the manifest. Look
@@ -370,7 +382,14 @@ function runfiles_rlocation_checked() {
         new_prefix="${prefix%/*}"
         [[ "$new_prefix" == "$prefix" ]] && break
         prefix="$new_prefix"
-        prefix_result=$(__runfiles_maybe_grep -m1 "^$prefix " "${RUNFILES_MANIFEST_FILE}" | cut -d ' ' -f 2-)
+        if [[ "$prefix" == *" "* ]]; then
+          search_prefix=" $(echo -n "$prefix" | wc -c | tr -d ' ') $prefix"
+        else
+          search_prefix="$prefix"
+        fi
+        # The extra space below is added because cut counts from 1.
+        trim_length=$(echo -n "$search_prefix  " | wc -c)
+        prefix_result=$(__runfiles_maybe_grep -m1 "^$search_prefix " "${RUNFILES_MANIFEST_FILE}" | cut -b ${trim_length}-)
         [[ -z "$prefix_result" ]] && continue
         local -r candidate="${prefix_result}${1#"${prefix}"}"
         if [[ -e "$candidate" ]]; then

tools/bash/runfiles/runfiles_test.bash

@@ -141,6 +141,8 @@ e/f $tmpdir/g h
 y $tmpdir/y
 c/dir $tmpdir/dir
 unresolved $tmpdir/unresolved
+ 4 h/ i $tmpdir/ j k
+ 15 dir with spaces $tmpdir/dir with spaces
 EOF
   mkdir "${tmpdir}/c"
   mkdir "${tmpdir}/y"
@@ -149,7 +151,11 @@ EOF
   touch "${tmpdir}/dir/file"
   ln -s /does/not/exist "${tmpdir}/dir/unresolved"
   touch "${tmpdir}/dir/deeply/nested/file"
+  touch "${tmpdir}/dir/deeply/nested/file with spaces"
   ln -s /does/not/exist "${tmpdir}/unresolved"
+  touch "${tmpdir}/ j k"
+  mkdir -p "${tmpdir}/dir with spaces/nested"
+  touch "${tmpdir}/dir with spaces/nested/file"
 
   export RUNFILES_DIR=
   export RUNFILES_MANIFEST_FILE=$tmpdir/foo.runfiles_manifest
@@ -166,14 +172,21 @@ EOF
   [[ "$(rlocation c/dir/file || echo failed)" == "$tmpdir/dir/file" ]] || fail
   [[ -z "$(rlocation c/dir/unresolved || echo failed)" ]] || fail
   [[ "$(rlocation c/dir/deeply/nested/file || echo failed)" == "$tmpdir/dir/deeply/nested/file" ]] || fail
+  [[ "$(rlocation "c/dir/deeply/nested/file with spaces" || echo failed)" == "$tmpdir/dir/deeply/nested/file with spaces" ]] || fail
   [[ -z "$(rlocation unresolved || echo failed)" ]] || fail
-  rm -r "$tmpdir/c/d" "$tmpdir/g h" "$tmpdir/y" "$tmpdir/dir" "$tmpdir/unresolved"
+  [[ "$(rlocation "h/ i" || echo failed)" == "$tmpdir/ j k" ]] || fail
+  [[ "$(rlocation "dir with spaces" || echo failed)" == "$tmpdir/dir with spaces" ]] || fail
+  [[ "$(rlocation "dir with spaces/nested/file" || echo failed)" == "$tmpdir/dir with spaces/nested/file" ]] || fail
+  rm -r "$tmpdir/c/d" "$tmpdir/g h" "$tmpdir/y" "$tmpdir/dir" "$tmpdir/unresolved" "$tmpdir/ j k" "$tmpdir/dir with spaces"
   [[ -z "$(rlocation a/b || echo failed)" ]] || fail
   [[ -z "$(rlocation e/f || echo failed)" ]] || fail
   [[ -z "$(rlocation y || echo failed)" ]] || fail
   [[ -z "$(rlocation c/dir || echo failed)" ]] || fail
   [[ -z "$(rlocation c/dir/file || echo failed)" ]] || fail
   [[ -z "$(rlocation c/dir/deeply/nested/file || echo failed)" ]] || fail
+  [[ -z "$(rlocation "h/ i" || echo failed)" ]] || fail
+  [[ -z "$(rlocation "dir with spaces" || echo failed)" ]] || fail
+  [[ -z "$(rlocation "dir with spaces/nested/file" || echo failed)" ]] || fail
 }
 
 function test_manifest_based_envvars() {

tools/cpp/runfiles/runfiles_src.cc

@@ -254,18 +254,52 @@ bool ParseManifest(const string& path, map<string, string>* result,
   std::getline(stm, line);
   size_t line_count = 1;
   while (!line.empty()) {
-    string::size_type idx = line.find_first_of(' ');
-    if (idx == string::npos) {
-      if (error) {
-        std::ostringstream err;
-        err << "ERROR: " << __FILE__ << "(" << __LINE__
-            << "): bad runfiles manifest entry in \"" << path << "\" line #"
-            << line_count << ": \"" << line << "\"";
-        *error = err.str();
+    std::string source;
+    std::string target;
+    if (line[0] == ' ') {
+      // Lines starting with a space are of the form " 7 foo bar /tar get/path", with
+      // the first field indicating the length of the runfiles path.
+      std::size_t length_field_end = line.find_first_of(' ', 1);
+      if (length_field_end == string::npos) {
+        if (error) {
+          std::ostringstream err;
+          err << "ERROR: " << __FILE__ << "(" << __LINE__
+              << "): invalid length field at line " << line_count << ": '"
+              << line << "'";
+          *error = err.str();
+        }
+        return false;
       }
-      return false;
+      std::size_t link_length = std::stoul(line.substr(1, length_field_end - 1));
+      std::size_t after_length_field = length_field_end + 1;
+      if (line.size() < after_length_field + link_length || line[after_length_field + link_length] != ' ') {
+        if (error) {
+          std::ostringstream err;
+          err << "ERROR: " << __FILE__ << "(" << __LINE__
+              << "): invalid length field at line " << line_count << ": '"
+              << line << "'";
+          *error = err.str();
+        }
+        return false;
+      }
+      source = line.substr(after_length_field, link_length);
+      target = line.substr(after_length_field + link_length + 1);
+    } else {
+      string::size_type idx = line.find_first_of(' ');
+      if (idx == string::npos) {
+        if (error) {
+          std::ostringstream err;
+          err << "ERROR: " << __FILE__ << "(" << __LINE__
+              << "): bad runfiles manifest entry in \"" << path << "\" line #"
+              << line_count << ": \"" << line << "\"";
+          *error = err.str();
+        }
+        return false;
+      }
+      source = line.substr(0, idx);
+      target = line.substr(idx + 1);
     }
-    (*result)[line.substr(0, idx)] = line.substr(idx + 1);
+    (*result)[source] = target;
     std::getline(stm, line);
     ++line_count;
   }

tools/cpp/runfiles/runfiles_test.cc

@@ -227,7 +227,12 @@ TEST_F(RunfilesTest, CannotCreateManifestBasedRunfilesDueToBadManifest) {
 
 TEST_F(RunfilesTest, ManifestBasedRunfilesRlocationAndEnvVars) {
   unique_ptr<MockFile> mf(MockFile::Create(
-      "foo" LINE_AS_STRING() ".runfiles_manifest", {"a/b c/d"}));
+      "foo" LINE_AS_STRING() ".runfiles_manifest", {
+          "a/b c/d",
+          "e/f target path with spaces",
+          " 4 h/ i j k",
+          " 15 dir with spaces l/m",
+      }));
   ASSERT_TRUE(mf != nullptr);
 
   string error;
@@ -256,6 +261,12 @@ TEST_F(RunfilesTest, ManifestBasedRunfilesRlocationAndEnvVars) {
   EXPECT_EQ(r->Rlocation("c:\\Foo"), "c:\\Foo");
   EXPECT_EQ(r->Rlocation("a/b/file"), "c/d/file");
   EXPECT_EQ(r->Rlocation("a/b/deeply/nested/file"), "c/d/deeply/nested/file");
+  EXPECT_EQ(r->Rlocation("a/b/deeply/nested/file with spaces"), "c/d/deeply/nested/file with spaces");
+  EXPECT_EQ(r->Rlocation("e/f"), "target path with spaces");
+  EXPECT_EQ(r->Rlocation("e/f/file"), "target path with spaces/file");
+  EXPECT_EQ(r->Rlocation("h/ i"), "j k");
+  EXPECT_EQ(r->Rlocation("dir with spaces"), "l/m");
+  EXPECT_EQ(r->Rlocation("dir with spaces/file"), "l/m/file");
 }
 
 TEST_F(RunfilesTest, DirectoryBasedRunfilesRlocationAndEnvVars) {