Runfiles: support space in file paths

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

@@ -224,8 +224,12 @@ public enum ManifestType implements ManifestWriter {
       @Override
       public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, Artifact symlink)
           throws IOException {
-        manifestWriter.append(rootRelativePath.getPathString());
-        // This trailing whitespace is REQUIRED to process the single entry line correctly.
+        // Replace spaces in the link name by a special character (U+0001).
+        // This character is very unlikely to be used in a path, and has a single-byte UTF-8
+        // encoding, so it's easily replaceable by 'sed' and other shell tools.
+        String p = rootRelativePath.getPathString().replace(' ', '\u0001');
+        manifestWriter.append(p);
+        // This trailing delimiter is REQUIRED to process the single entry line correctly.
         manifestWriter.append(' ');
         if (symlink != null) {
           manifestWriter.append(symlink.getPath().getPathString());

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

@@ -19,6 +19,7 @@
 
 #include <string.h>
 
+#include <algorithm>
 #include <fstream>
 #include <iostream>
 #include <sstream>
@@ -179,6 +180,8 @@ class RunfilesCreator {
         link = wline.substr(0, space_pos);
         target = wline.substr(space_pos + 1);
       }
+      // Spaces in the link name are replaced by a special character (\x01).
+      std::replace(link.begin(), link.end(), '\x01', ' ');
 
       // Removing leading and trailing spaces
       Trim(link);

src/main/tools/build-runfiles.cc

@@ -53,6 +53,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include <algorithm>
 #include <map>
 #include <string>
 
@@ -154,11 +155,10 @@ class RunfilesCreator {
       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(buf, s-buf);
+      // Spaces in the link name are replaced by a special character (\x01).
+      std::replace(link.begin(), link.end(), '\x01', ' ');
       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.

src/test/shell/bazel/BUILD

@@ -788,7 +788,7 @@ sh_test(
     size = "medium",
     srcs = ["runfiles_test.sh"],
     data = [":test-deps"],
-    tags = ["no_windows"],
+    deps = ["@bazel_tools//tools/bash/runfiles"],
 )
 
 sh_test(

src/test/shell/bazel/runfiles_test.sh

@@ -17,11 +17,37 @@
 # Test runfiles creation
 #
 
-# Load the test setup defined in the parent directory
-CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-source "${CURRENT_DIR}/../integration_test_setup.sh" \
+# --- begin runfiles.bash initialization v2 ---
+# Copy-pasted from the Bazel Bash runfiles library v2.
+set -uo pipefail; f=bazel_tools/tools/bash/runfiles/runfiles.bash
+source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$0.runfiles/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
+# --- end runfiles.bash initialization v2 ---
+
+source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \
   || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
 
+case "$(uname -s | tr [:upper:] [:lower:])" in
+msys*|mingw*|cygwin*)
+  declare -r is_windows=true
+  ;;
+*)
+  declare -r is_windows=false
+  ;;
+esac
+
+if "$is_windows"; then
+  export MSYS_NO_PATHCONV=1
+  export MSYS2_ARG_CONV_EXCL="*"
+  export EXT=".exe"
+else
+  export EXT=""
+fi
+
 # Make sure runfiles are created under a custom-named subdirectory when
 # workspace() is specified in the WORKSPACE file.
 function test_runfiles() {
@@ -48,10 +74,10 @@ public class Noise {
 }
 EOF
 
-  bazel build //foo:foo >& $TEST_log || fail "Build failed"
-  [[ -d bazel-bin/foo/foo.runfiles/$name ]] || fail "$name runfiles directory not created"
-  [[ -d bazel-bin/foo/foo.runfiles/$name/foo ]] || fail "No foo subdirectory under $name"
-  [[ -x bazel-bin/foo/foo.runfiles/$name/foo/foo ]] || fail "No foo executable under $name"
+  bazel build --enable_runfiles=yes //foo:foo >& $TEST_log || fail "Build failed"
+  [[ -d "bazel-bin/foo/foo${EXT}.runfiles/$name" ]] || fail "$name runfiles directory not created"
+  [[ -d "bazel-bin/foo/foo${EXT}.runfiles/$name/foo" ]] || fail "No foo subdirectory under $name"
+  [[ -x "bazel-bin/foo/foo${EXT}.runfiles/$name/foo/foo" ]] || fail "No foo executable under $name"
 }
 
 function test_legacy_runfiles_change() {
@@ -77,19 +103,19 @@ EOF
   cat > thing.cc <<EOF
 int main() { return 0; }
 EOF
-  bazel build --legacy_external_runfiles //:thing &> $TEST_log \
+  bazel build --enable_runfiles=yes --legacy_external_runfiles //:thing &> $TEST_log \
     || fail "Build failed"
-  [[ -d bazel-bin/thing.runfiles/foo/external/bar ]] \
+  [[ -d bazel-bin/thing${EXT}.runfiles/foo/external/bar ]] \
     || fail "bar not found"
 
-  bazel build --nolegacy_external_runfiles //:thing &> $TEST_log \
+  bazel build --enable_runfiles=yes --nolegacy_external_runfiles //:thing &> $TEST_log \
     || fail "Build failed"
-  [[ ! -d bazel-bin/thing.runfiles/foo/external/bar ]] \
+  [[ ! -d bazel-bin/thing${EXT}.runfiles/foo/external/bar ]] \
     || fail "Old bar still found"
 
-  bazel build --legacy_external_runfiles //:thing &> $TEST_log \
+  bazel build --enable_runfiles=yes --legacy_external_runfiles //:thing &> $TEST_log \
     || fail "Build failed"
-  [[ -d bazel-bin/thing.runfiles/foo/external/bar ]] \
+  [[ -d bazel-bin/thing${EXT}.runfiles/foo/external/bar ]] \
     || fail "bar not recreated"
 }
 
@@ -122,19 +148,85 @@ sh_test(
 )
 EOF
 
-  bazel build --spawn_strategy=local --nobuild_runfile_links //:test \
+  bazel build --spawn_strategy=local --enable_runfiles=yes --nobuild_runfile_links //:test \
     || fail "Building //:test failed"
 
-  [[ ! -f bazel-bin/test.runfiles/foo/data/hello ]] || fail "expected no runfile data/hello"
-  [[ ! -f bazel-bin/test.runfiles/foo/data/world ]] || fail "expected no runfile data/world"
-  [[ ! -f bazel-bin/test.runfiles/MANIFEST ]] || fail "expected output manifest to not exist"
+  [[ ! -f "bazel-bin/test${EXT}.runfiles/foo/data/hello" ]] || fail "expected no runfile data/hello"
+  [[ ! -f "bazel-bin/test${EXT}.runfiles/foo/data/world" ]] || fail "expected no runfile data/world"
+  [[ ! -f "bazel-bin/test${EXT}.runfiles/MANIFEST" ]] || fail "expected output manifest to not exist"
 
-  bazel test --spawn_strategy=local --nobuild_runfile_links //:test \
+  bazel test --spawn_strategy=local --enable_runfiles=yes --nobuild_runfile_links --test_output=errors //:test \
     || fail "Testing //:foo 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"
-  [[ -f bazel-bin/test.runfiles/MANIFEST ]] || fail "expected output manifest to exist"
+  [[ -f "bazel-bin/test${EXT}.runfiles/foo/data/hello" ]] || fail "expected runfile data/hello to exist"
+  [[ -f "bazel-bin/test${EXT}.runfiles/foo/data/world" ]] || fail "expected runfile data/world to exist"
+  [[ -f "bazel-bin/test${EXT}.runfiles/MANIFEST" ]] || fail "expected output manifest to exist"
+}
+
+function test_space_in_runfile_path() {
+  mkdir -p 'foo/a a'         # one space
+  mkdir -p 'repo/b b/c   c'  # more than one space
+  touch 'foo/x.txt'          # no space
+  touch 'foo/a a/y.txt'      # space in runfile link and target path
+  touch 'repo/b b/z.txt'     # space in target path only
+  touch 'repo/b b/c   c/w.txt' # space in runfile link and target path in ext.repo
+  touch 'repo/b b/WORKSPACE'
+  cat >'repo/b b/BUILD' <<'eof'
+filegroup(
+    name = "files",
+    srcs = glob(["**"]),
+    visibility = ["//visibility:public"],
+)
+eof
+  cat >WORKSPACE <<'eof'
+workspace(name = "foo_ws")
+local_repository(
+    name = "space_ws",
+    path = "repo/b b",
+)
+eof
+  cat >BUILD <<'eof'
+sh_binary(
+    name = "x",
+    srcs = ["x.sh"],
+    data = ["@space_ws//:files"] + glob(["foo/**"]),
+    deps = ["@bazel_tools//tools/bash/runfiles"],
+)
+eof
+  cat >x.sh <<'eof'
+#!/bin/bash
+# --- begin runfiles.bash initialization v2 ---
+# Copy-pasted from the Bazel Bash runfiles library v2.
+set -uo pipefail; f=bazel_tools/tools/bash/runfiles/runfiles.bash
+source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$0.runfiles/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
+# --- end runfiles.bash initialization v2 ---
+echo "Hello from x.sh"
+echo "x=($(rlocation "foo_ws/foo/x.txt"))"
+echo "y=($(rlocation "foo_ws/foo/a a/y.txt"))"
+echo "z=($(rlocation "space_ws/z.txt"))"
+echo "w=($(rlocation "space_ws/c   c/w.txt"))"
+eof
+  chmod +x x.sh
+
+  # Look up runfiles using the runfiles manifest
+  bazel run --enable_runfiles=yes --nobuild_runfile_links //:x >&"$TEST_log"
+  expect_log "Hello from x.sh"
+  expect_log "^x=(.*foo/x.txt)"
+  expect_log "^y=(.*foo/a a/y.txt)"
+  expect_log "^z=(.*space_ws/z.txt)"
+  expect_log "^w=(.*space_ws/c   c/w.txt)"
+
+  # See if runfiles links are generated
+  bazel build --enable_runfiles=yes --build_runfile_links //:x >&"$TEST_log"
+  [[ -e "bazel-bin/x${EXT}.runfiles/foo_ws/foo/x.txt" ]] || fail "Cannot find x.txt"
+  [[ -e "bazel-bin/x${EXT}.runfiles/foo_ws/foo/a a/y.txt" ]] || fail "Cannot find y.txt"
+  [[ -e "bazel-bin/x${EXT}.runfiles/space_ws/z.txt" ]] || fail "Cannot find z.txt"
+  [[ -e "bazel-bin/x${EXT}.runfiles/space_ws/c   c/w.txt" ]] || fail "Cannot find w.txt"
 }
 
 run_suite "runfiles tests"

tools/bash/runfiles/runfiles.bash

@@ -68,7 +68,7 @@
 #       cat "$(rlocation my_workspace/path/to/my/data.txt)"
 #
 
-if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+if [[ ! -d "${RUNFILES_DIR:-/dev/null/x}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null/x}" ]]; then
   if [[ -f "$0.runfiles_manifest" ]]; then
     export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest"
   elif [[ -f "$0.runfiles/MANIFEST" ]]; then
@@ -112,17 +112,18 @@ function rlocation() {
     fi
     return 1
   else
-    if [[ -e "${RUNFILES_DIR:-/dev/null}/$1" ]]; then
+    if [[ -e "${RUNFILES_DIR:-/dev/null/x}/$1" ]]; then
       if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
         echo >&2 "INFO[runfiles.bash]: rlocation($1): found under RUNFILES_DIR ($RUNFILES_DIR), return"
       fi
       echo "${RUNFILES_DIR}/$1"
-    elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+    elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null/x}" ]]; then
       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=$(grep -m1 "^$1 " "${RUNFILES_MANIFEST_FILE}" | cut -d ' ' -f 2-)
-      if [[ -e "${result:-/dev/null}" ]]; then
+      # Spaces in the link name are replaced by a special character (U+0001).
+      local -r result=$(grep -m1 "^$(echo "$1" | tr ' ' '\001') " "${RUNFILES_MANIFEST_FILE}" | cut -d ' ' -f 2-)
+      if [[ -e "${result:-/dev/null/x}" ]]; then
         if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
           echo >&2 "INFO[runfiles.bash]: rlocation($1): found in manifest as ($result)"
         fi
@@ -151,20 +152,20 @@ export -f rlocation
 # libraries under @bazel_tools//tools/<lang>/runfiles, then that binary needs
 # these envvars in order to initialize its own runfiles library.
 function runfiles_export_envvars() {
-  if [[ ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" \
-        && ! -d "${RUNFILES_DIR:-/dev/null}" ]]; then
+  if [[ ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null/x}" \
+        && ! -d "${RUNFILES_DIR:-/dev/null/x}" ]]; then
     return 1
   fi
 
-  if [[ ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+  if [[ ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null/x}" ]]; then
     if [[ -f "$RUNFILES_DIR/MANIFEST" ]]; then
       export RUNFILES_MANIFEST_FILE="$RUNFILES_DIR/MANIFEST"
     elif [[ -f "${RUNFILES_DIR}_manifest" ]]; then
       export RUNFILES_MANIFEST_FILE="${RUNFILES_DIR}_manifest"
     else
       export RUNFILES_MANIFEST_FILE=
     fi
-  elif [[ ! -d "${RUNFILES_DIR:-/dev/null}" ]]; then
+  elif [[ ! -d "${RUNFILES_DIR:-/dev/null/x}" ]]; then
     if [[ "$RUNFILES_MANIFEST_FILE" == */MANIFEST \
           && -d "${RUNFILES_MANIFEST_FILE%/MANIFEST}" ]]; then
       export RUNFILES_DIR="${RUNFILES_MANIFEST_FILE%/MANIFEST}"