From 0d7a3cc4d0cd6fd371591dfff2f39017107a62bb Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 20 Sep 2024 12:19:57 +0200 Subject: [PATCH] New escaping scheme --- .../lib/analysis/SourceManifestAction.java | 19 ++-- src/main/tools/build-runfiles-windows.cc | 27 +++--- src/main/tools/build-runfiles.cc | 24 +++-- .../analysis/SourceManifestActionTest.java | 31 ++++++ src/test/shell/integration/runfiles_test.sh | 94 ++++++++++++++++++- tools/bash/runfiles/runfiles.bash | 20 ++-- tools/bash/runfiles/runfiles_test.bash | 7 +- tools/cpp/runfiles/runfiles_src.cc | 35 +++---- tools/cpp/runfiles/runfiles_test.cc | 6 +- tools/java/runfiles/Runfiles.java | 23 ++--- tools/java/runfiles/testing/RunfilesTest.java | 6 +- 11 files changed, 211 insertions(+), 81 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java index 4381ad48182a8c..0118195cfbb35a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java +++ b/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,10 @@ public final class SourceManifestAction extends AbstractFileWriteAction private static final String GUID = "07459553-a3d0-4d37-9d78-18ed942470f4"; private static final Comparator> 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('\\', "\\b").toEscaper(); + private final Artifact repoMappingManifest; /** * Interface for defining manifest formatting and reporting specifics. Implementations must be @@ -291,6 +296,9 @@ public enum ManifestType implements ManifestWriter { * *

[rootRelativePath] [resolvingSymlink] * + *

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. + * *

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}. */ @@ -303,14 +311,13 @@ public void writeEntry( throws IOException { 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). + // are expected to split on the first space. if (rootRelativePathString.indexOf(' ') != -1) { manifestWriter.append(' '); - manifestWriter.append(String.valueOf(rootRelativePathString.length())); - manifestWriter.append(' '); + manifestWriter.append(ROOT_RELATIVE_PATH_ESCAPER.escape(rootRelativePathString)); + } else { + manifestWriter.append(rootRelativePathString); } - manifestWriter.append(rootRelativePathString); // This trailing whitespace is REQUIRED to process the single entry line correctly. manifestWriter.append(' '); if (symlinkTarget != null) { diff --git a/src/main/tools/build-runfiles-windows.cc b/src/main/tools/build-runfiles-windows.cc index 2b15e922a8d313..5399f5e0d1ace5 100644 --- a/src/main/tools/build-runfiles-windows.cc +++ b/src/main/tools/build-runfiles-windows.cc @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -41,6 +42,9 @@ using std::wstring; namespace { +const std::regex kEscapedBackslash(R"(\\b)"); +const std::regex kEscapedSpace(R"(\\s)"); + const wchar_t* manifest_filename; const wchar_t* runfiles_base_dir; @@ -162,21 +166,18 @@ class RunfilesCreator { 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()); + // 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()); } - link = blaze_util::CstringToWstring(line.substr(after_length_field, link_length)); - target = blaze_util::CstringToWstring(line.substr(after_length_field + link_length + 1)); + std::string link_path = line.substr(1, idx - 1); + link_path = std::regex_replace(link_path, kEscapedSpace, " "); + link_path = std::regex_replace(link_path, kEscapedBackslash, "\\"); + link = blaze_util::CstringToWstring(link_path); + target = blaze_util::CstringToWstring(line.substr(idx + 1)); } else { - string::size_type idx = line.find_first_of(' '); + string::size_type idx = line.find(' '); if (idx == string::npos) { die(L"Missing separator in manifest line: %hs", line.c_str()); } diff --git a/src/main/tools/build-runfiles.cc b/src/main/tools/build-runfiles.cc index 0395f0140e15af..5f3c85bc32e3db 100644 --- a/src/main/tools/build-runfiles.cc +++ b/src/main/tools/build-runfiles.cc @@ -54,11 +54,15 @@ #include #include +#include #include // program_invocation_short_name is not portable. static const char *argv0; +static const std::regex kEscapedBackslash(R"(\\b)"); +static const std::regex kEscapedSpace(R"(\\s)"); + const char *input_filename; const char *output_base_dir; @@ -160,18 +164,18 @@ class RunfilesCreator { 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); + // 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 = std::string(after_length_field, link_length); + link = std::string(buf + 1, s); + link = std::regex_replace(link, kEscapedSpace, " "); + link = std::regex_replace(link, kEscapedBackslash, "\\"); + target = s + 1; } else { - // The line is of the form "foo /target/path", with only a single space. + // 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); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java index d51212772d5394..f8ee3e6f122fa6 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java @@ -387,6 +387,37 @@ 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 fileWithSpacePath = scratch.file("trivial/file with sp\\ace", "foo"); + Artifact fileWithSpaceAndBackslash = ActionsTestUtil.createArtifact(trivialRoot, fileWithSpacePath); + + 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("with sp\\ace"), buildFile) + .addSymlink(PathFragment.create("also/with sp\\ace"), fileWithSpaceAndBackslash) + .build()); + + assertThat(action.getFileContents(reporter)) + .isEqualTo( + """ + TESTING/also/no/sp\\ace /workspace/trivial/file with sp\\ace + TESTING/also/with\\ssp\\bace /workspace/trivial/file with sp\\ace + TESTING/no/sp\\ace /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); diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh index 53acfcfba1ebbc..1f226b0dfb320a 100755 --- a/src/test/shell/integration/runfiles_test.sh +++ b/src/test/shell/integration/runfiles_test.sh @@ -569,7 +569,7 @@ EOF assert_contains '/link_two$' *-bin/a/go } -function test_spaces_in_runfiles_source_paths() { +function setup_spaces_in_runfiles_source_paths() { mkdir -p pkg cat > pkg/defs.bzl <<'EOF' def _spaces_impl(ctx): @@ -598,11 +598,21 @@ if [[ "$(cat "pkg/ a b .txt")" != "my content" ]]; then fi EOF chmod +x pkg/foo.sh +} + +function test_spaces_in_runfiles_source_paths_out_of_process() { + setup_spaces_in_runfiles_source_paths + bazel test --noexperimental_inprocess_symlink_creation \ + //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" +} - bazel test //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" +function test_spaces_in_runfiles_source_paths_in_process() { + setup_spaces_in_runfiles_source_paths + bazel test --experimental_inprocess_symlink_creation \ + //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" } -function test_spaces_in_runfiles_source_and_target_paths() { +function setup_spaces_in_runfiles_source_and_target_paths() { dir=$(mktemp -d 'runfiles test.XXXXXX') cd "$dir" || fail "failed to cd to $dir" touch MODULE.bazel @@ -635,8 +645,84 @@ if [[ "$(cat "pkg/ a b .txt")" != "my content" ]]; then fi EOF chmod +x pkg/foo.sh +} + +function test_spaces_in_runfiles_source_and_target_paths_out_of_process() { + setup_spaces_in_runfiles_source_and_target_paths + bazel test --noexperimental_inprocess_symlink_creation \ + //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" +} + +function test_spaces_in_runfiles_source_and_target_paths_in_process() { + setup_spaces_in_runfiles_source_and_target_paths + bazel test --experimental_inprocess_symlink_creation \ + //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" +} + +# Verify that Bazel's runfiles manifest is compatible with v3 of the Bash +# runfiles library snippet, even if the workspace path contains a space. +function test_compatibility_with_bash_runfiles_library_snippet() { + # Create a workspace path with a space. + WORKSPACE="$(mktemp -d XXXXXXXX.jar_manifest)/my workspace" + trap "rm -fr '$WORKSPACE'" EXIT + mkdir -p "$WORKSPACE" + cd "$WORKSPACE" || fail "failed to cd to $WORKSPACE" + cat > MODULE.bazel <<'EOF' +module(name = "my_module") +EOF + + mkdir pkg + cat > pkg/BUILD <<'EOF' +sh_binary( + name = "tool", + srcs = ["tool.sh"], + deps = ["@bazel_tools//tools/bash/runfiles"], +) + +genrule( + name = "gen", + outs = ["out"], + tools = [":tool"], + cmd = "$(execpath :tool) $@", +) +EOF + cat > pkg/tool.sh <<'EOF' +#!/bin/bash +# --- begin runfiles.bash initialization v3 --- +# Copy-pasted from the Bazel Bash runfiles library v3. +set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash +# shellcheck disable=SC1090 +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 v3 --- + +if [[ ! -z "${RUNFILES_DIR+x}" ]]; then + echo "RUNFILES_DIR is set" + exit 1 +fi + +if [[ -z "${RUNFILES_MANIFEST_FILE+x}" ]]; then + echo "RUNFILES_MANIFEST_FILE is not set" + exit 1 +fi + +if [[ -z "$(rlocation "my_module/pkg/tool.sh")" ]]; then + echo "rlocation failed" + exit 1 +fi + +touch $1 +EOF + chmod +x pkg/tool.sh - bazel test //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" + bazel build --noenable_runfiles \ + --spawn_strategy=local \ + --action_env=RUNFILES_LIB_DEBUG=1 \ + //pkg:gen >&$TEST_log || fail "build failed" } run_suite "runfiles" diff --git a/tools/bash/runfiles/runfiles.bash b/tools/bash/runfiles/runfiles.bash index 746bba59f8e27a..27372d35c16a7c 100644 --- a/tools/bash/runfiles/runfiles.bash +++ b/tools/bash/runfiles/runfiles.bash @@ -357,10 +357,10 @@ 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 - # If the rlocation path contains a space, it needs to be prefixed with - # " ", where is the length of the path in bytes. + # If the rlocation path contains a space, it needs to be prefixed with a + # space and spaces and backslashes have to be escaped as \s and \b. if [[ "$1" == *" "* ]]; then - local search_prefix=" $(echo -n "$1" | wc -c | tr -d ' ') $1" + local search_prefix=" $(echo -n "$1" | sed 's/\\/\\b/g; s/ /\\s/g')" if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "INFO[runfiles.bash]: rlocation($1): using escaped search prefix ($search_prefix)" fi @@ -369,7 +369,9 @@ function runfiles_rlocation_checked() { 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}-) + # Escape the search prefix for use in the grep regex below *after* + # determining the trim length. + local -r result=$(__runfiles_maybe_grep -m1 "^$(echo -n "$search_prefix" | sed 's/[.[\*^$]/\\&/g') " "${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 @@ -382,14 +384,20 @@ function runfiles_rlocation_checked() { new_prefix="${prefix%/*}" [[ "$new_prefix" == "$prefix" ]] && break prefix="$new_prefix" + if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then + echo >&2 "INFO[runfiles.bash]: rlocation($1): looking for prefix ($prefix)" + fi if [[ "$prefix" == *" "* ]]; then - search_prefix=" $(echo -n "$prefix" | wc -c | tr -d ' ') $prefix" + search_prefix=" $(echo -n "$prefix" | sed 's/\\/\\b/g; s/ /\\s/g')" + if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then + echo >&2 "INFO[runfiles.bash]: rlocation($1): using escaped search prefix ($search_prefix)" + fi 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}-) + prefix_result=$(__runfiles_maybe_grep -m1 "$(echo -n "$search_prefix" | sed 's/[.[\*^$]/\\&/g') " "${RUNFILES_MANIFEST_FILE}" | cut -b ${trim_length}-) [[ -z "$prefix_result" ]] && continue local -r candidate="${prefix_result}${1#"${prefix}"}" if [[ -e "$candidate" ]]; then diff --git a/tools/bash/runfiles/runfiles_test.bash b/tools/bash/runfiles/runfiles_test.bash index b6b43006dff264..4d7845c1ce3814 100755 --- a/tools/bash/runfiles/runfiles_test.bash +++ b/tools/bash/runfiles/runfiles_test.bash @@ -141,8 +141,9 @@ 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 + h/\si $tmpdir/ j k + h/\s\bi $tmpdir/ j k b + dir\swith\sspaces $tmpdir/dir with spaces EOF mkdir "${tmpdir}/c" mkdir "${tmpdir}/y" @@ -154,6 +155,7 @@ EOF touch "${tmpdir}/dir/deeply/nested/file with spaces" ln -s /does/not/exist "${tmpdir}/unresolved" touch "${tmpdir}/ j k" + touch "${tmpdir}/ j k b" mkdir -p "${tmpdir}/dir with spaces/nested" touch "${tmpdir}/dir with spaces/nested/file" @@ -175,6 +177,7 @@ EOF [[ "$(rlocation "c/dir/deeply/nested/file with spaces" || echo failed)" == "$tmpdir/dir/deeply/nested/file with spaces" ]] || fail [[ -z "$(rlocation unresolved || echo failed)" ]] || fail [[ "$(rlocation "h/ i" || echo failed)" == "$tmpdir/ j k" ]] || fail + [[ "$(rlocation "h/ \i" || echo failed)" == "$tmpdir/ j k b" ]] || 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" diff --git a/tools/cpp/runfiles/runfiles_src.cc b/tools/cpp/runfiles/runfiles_src.cc index e3c872d3f1813b..04da1dbd1d007e 100644 --- a/tools/cpp/runfiles/runfiles_src.cc +++ b/tools/cpp/runfiles/runfiles_src.cc @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -49,6 +50,9 @@ using std::vector; namespace { +const std::regex kEscapedBackslash("\\\\b"); +const std::regex kEscapedSpace("\\\\s"); + bool starts_with(const string& s, const char* prefix) { if (!prefix || !*prefix) { return true; @@ -257,35 +261,24 @@ bool ParseManifest(const string& path, map* result, 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; - } - 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] != ' ') { + // The link path contains escape sequences for spaces and backslashes. + string::size_type idx = line.find(' ', 1); + if (idx == string::npos) { if (error) { std::ostringstream err; err << "ERROR: " << __FILE__ << "(" << __LINE__ - << "): invalid length field at line " << line_count << ": '" - << line << "'"; + << "): bad runfiles manifest entry in \"" << path << "\" 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); + source = line.substr(1, idx - 1); + source = std::regex_replace(source, kEscapedSpace, " "); + source = std::regex_replace(source, kEscapedBackslash, "\\"); + target = line.substr(idx + 1); } else { - string::size_type idx = line.find_first_of(' '); + string::size_type idx = line.find(' '); if (idx == string::npos) { if (error) { std::ostringstream err; diff --git a/tools/cpp/runfiles/runfiles_test.cc b/tools/cpp/runfiles/runfiles_test.cc index 139081e148ff3f..463e5a0c1f8646 100644 --- a/tools/cpp/runfiles/runfiles_test.cc +++ b/tools/cpp/runfiles/runfiles_test.cc @@ -230,8 +230,9 @@ TEST_F(RunfilesTest, ManifestBasedRunfilesRlocationAndEnvVars) { "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", + " h/\\si j k", + " dir\\\swith\\\sspaces l/m", + " h/\\s\\bi j k b", })); ASSERT_TRUE(mf != nullptr); @@ -265,6 +266,7 @@ TEST_F(RunfilesTest, ManifestBasedRunfilesRlocationAndEnvVars) { 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("h/ \\i"), "j k b"); EXPECT_EQ(r->Rlocation("dir with spaces"), "l/m"); EXPECT_EQ(r->Rlocation("dir with spaces/file"), "l/m/file"); } diff --git a/tools/java/runfiles/Runfiles.java b/tools/java/runfiles/Runfiles.java index 7445648e355b02..0263e19913f7c7 100644 --- a/tools/java/runfiles/Runfiles.java +++ b/tools/java/runfiles/Runfiles.java @@ -495,24 +495,17 @@ private static Map loadRunfiles(String path) throws IOException String runfile; String realPath; if (line.startsWith(" ")) { - // 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. - int endOfLengthField = line.indexOf(' ', 1); - if (endOfLengthField == -1) { - throw new IOException( - "Invalid runfiles manifest line, expected second space with leading space: " - + line); - } - int runfileLength; - try { - runfileLength = Integer.parseUnsignedInt(line.substring(1, endOfLengthField)); - } catch (NumberFormatException e) { + // In lines starting with a space, the runfile path contains spaces and backslashes + // escaped with a backslash. The real path is the rest of the line after the first + // unescaped space. + int firstSpace = line.indexOf(' ', 1); + if (firstSpace == -1) { throw new IOException( - "Invalid runfiles manifest line, expected unsigned integer length field: " + "Invalid runfiles manifest line, expected at least one space after the leading space: " + line); } - runfile = line.substring(endOfLengthField + 1, endOfLengthField + 1 + runfileLength); - realPath = line.substring(endOfLengthField + 1 + runfileLength + 1); + runfile = line.substring(1, firstSpace).replace("\\s", " ").replace("\\b", "\\"); + realPath = line.substring(firstSpace + 1); } else { int firstSpace = line.indexOf(' '); if (firstSpace == -1) { diff --git a/tools/java/runfiles/testing/RunfilesTest.java b/tools/java/runfiles/testing/RunfilesTest.java index 802a3de8c1d446..9bec0c68652cf8 100644 --- a/tools/java/runfiles/testing/RunfilesTest.java +++ b/tools/java/runfiles/testing/RunfilesTest.java @@ -248,8 +248,9 @@ public void testManifestBasedRlocation() throws Exception { "Foo/runfile1 C:/Actual Path\\runfile1", "Foo/Bar/runfile2 D:\\the path\\run file 2.txt", "Foo/Bar/Dir E:\\Actual Path\\Directory", - " 4 h/ i F:\\jk", - " 15 dir with spaces F:\\j k\\dir with spaces")); + " h/\\si F:\\jk", + " dir\\swith\\sspaces F:\\j k\\dir with spaces", + " h/\\s\\bi F:\\jkb")); Runfiles r = Runfiles.createManifestBasedForTesting(mf.toString()).withSourceRepository(""); assertThat(r.rlocation("Foo/runfile1")).isEqualTo("C:/Actual Path\\runfile1"); assertThat(r.rlocation("Foo/Bar/runfile2")).isEqualTo("D:\\the path\\run file 2.txt"); @@ -260,6 +261,7 @@ public void testManifestBasedRlocation() throws Exception { assertThat(r.rlocation("Foo/Bar/Dir/Deeply/Nested/File With Spaces")) .isEqualTo("E:\\Actual Path\\Directory/Deeply/Nested/File With Spaces"); assertThat(r.rlocation("h/ i")).isEqualTo("F:\\jk"); + assertThat(r.rlocation("h/ \\i")).isEqualTo("F:\\jkb"); assertThat(r.rlocation("dir with spaces")).isEqualTo("F:\\j k\\dir with spaces"); assertThat(r.rlocation("dir with spaces/file")).isEqualTo("F:\\j k\\dir with spaces/file"); assertThat(r.rlocation("unknown")).isNull();