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 cc4fb298fb6ac9..953cddda072c1e 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; @@ -61,7 +63,16 @@ 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('\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 { * *

[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}. */ @@ -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'); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java index cb8e2556bdb358..3be2a9423509cc 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java +++ b/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); diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 3886f627521f90..e392b43eb9540d 100644 --- a/src/main/protobuf/failure_details.proto +++ b/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 }]; } diff --git a/src/main/tools/build-runfiles-windows.cc b/src/main/tools/build-runfiles-windows.cc index 0a0b0ab6f5d706..05c3f265c9463c 100644 --- a/src/main/tools/build-runfiles-windows.cc +++ b/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 diff --git a/src/main/tools/build-runfiles.cc b/src/main/tools/build-runfiles.cc index 21a6b5a7efdb32..dd559b72fd05a7 100644 --- a/src/main/tools/build-runfiles.cc +++ b/src/main/tools/build-runfiles.cc @@ -103,6 +103,39 @@ struct FileInfo { typedef std::map 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); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD index b98c10f4ef66d5..4af452dd2d35c1 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD +++ b/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", 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 b0dc9b3ae659bd..afec836948201b 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 @@ -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; @@ -388,6 +389,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); diff --git a/src/test/shell/bazel/bazel_determinism_test.sh b/src/test/shell/bazel/bazel_determinism_test.sh index a122cd238106c2..4ba42fb3edc174 100755 --- a/src/test/shell/bazel/bazel_determinism_test.sh +++ b/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}" diff --git a/src/test/shell/bazel/workspace_test.sh b/src/test/shell/bazel/workspace_test.sh index 4e6f276f9f0b67..7e078d1e5762c2 100755 --- a/src/test/shell/bazel/workspace_test.sh +++ b/src/test/shell/bazel/workspace_test.sh @@ -73,7 +73,7 @@ function test_path_with_spaces() { cd "$ws" create_workspace_with_default_repos WORKSPACE - bazel info &> $TEST_log && fail "Info succeeeded" + bazel info &> $TEST_log || fail "Info failed" bazel help &> $TEST_log || fail "Help failed" } diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh index d7e0dc0c2ab7a9..f1aa03fcfca30a 100755 --- a/src/test/shell/integration/runfiles_test.sh +++ b/src/test/shell/integration/runfiles_test.sh @@ -511,4 +511,270 @@ EOF || fail "MANIFEST file not recreated" } +function setup_special_chars_in_runfiles_source_paths() { + mkdir -p pkg + if "$is_windows"; then + cat > pkg/constants.bzl <<'EOF' +NAME = "pkg/a b .txt" +EOF + else + cat > pkg/constants.bzl <<'EOF' +NAME = "pkg/a \n \\ b .txt" +EOF + fi + cat > pkg/defs.bzl <<'EOF' +load(":constants.bzl", "NAME") +def _special_chars_impl(ctx): + out = ctx.actions.declare_file("data.txt") + ctx.actions.write(out, "my content") + runfiles = ctx.runfiles( + symlinks = { + NAME: out, + }, + ) + return [DefaultInfo(files = depset([out]), runfiles = runfiles)] + +spaces = rule( + implementation = _special_chars_impl, +) +EOF + cat > pkg/BUILD <<'EOF' +load(":defs.bzl", "spaces") +spaces(name = "spaces") +sh_test( + name = "foo", + srcs = ["foo.sh"], + data = [":spaces"], +) +EOF + if "$is_windows"; then + 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 + else + cat > pkg/foo.sh <<'EOF' +#!/bin/bash +if [[ "$(cat $'pkg/a \n \\ b .txt')" != "my content" ]]; then + echo "unexpected content or not found" + exit 1 +fi +EOF + fi + chmod +x pkg/foo.sh +} + +function test_special_chars_in_runfiles_source_paths_out_of_process() { + setup_special_chars_in_runfiles_source_paths + bazel test --noexperimental_inprocess_symlink_creation \ + --test_output=errors \ + //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" +} + +function test_special_chars_in_runfiles_source_paths_in_process() { + setup_special_chars_in_runfiles_source_paths + bazel test --experimental_inprocess_symlink_creation \ + --test_output=errors \ + //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" +} + +function setup_special_chars_in_runfiles_target_paths() { + mkdir -p pkg + if "$is_windows"; then + cat > pkg/constants.bzl <<'EOF' +NAME = "pkg/a b .txt" +EOF + else + cat > pkg/constants.bzl <<'EOF' +NAME = "pkg/a \n \\ b .txt" +EOF + fi + cat > pkg/defs.bzl <<'EOF' +load(":constants.bzl", "NAME") +def _special_chars_impl(ctx): + out = ctx.actions.declare_file(NAME) + ctx.actions.write(out, "my content") + runfiles = ctx.runfiles( + symlinks = { + "pkg/data.txt": out, + }, + ) + return [DefaultInfo(files = depset([out]), runfiles = runfiles)] + +spaces = rule( + implementation = _special_chars_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/data.txt)" != "my content" ]]; then + echo "unexpected content or not found" + exit 1 +fi +EOF + chmod +x pkg/foo.sh +} + +function test_special_chars_in_runfiles_target_paths_out_of_process() { + setup_special_chars_in_runfiles_target_paths + bazel test --noexperimental_inprocess_symlink_creation \ + --test_output=errors \ + //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" +} + +function test_special_chars_in_runfiles_target_paths_in_process() { + setup_special_chars_in_runfiles_target_paths + bazel test --experimental_inprocess_symlink_creation \ + --test_output=errors \ + //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" +} + +function setup_special_chars_in_runfiles_source_and_target_paths() { + mkdir -p pkg + if "$is_windows"; then + cat > pkg/constants.bzl <<'EOF' +NAME = "a b .txt" +EOF + else + cat > pkg/constants.bzl <<'EOF' +NAME = "a \n \\ b .txt" +EOF + fi + cat > pkg/defs.bzl <<'EOF' +load(":constants.bzl", "NAME") +def _special_chars_impl(ctx): + out = ctx.actions.declare_file(NAME) + ctx.actions.write(out, "my content") + return [DefaultInfo(files = depset([out]))] + +spaces = rule( + implementation = _special_chars_impl, +) +EOF + cat > pkg/BUILD <<'EOF' +load(":defs.bzl", "spaces") +spaces(name = "spaces") +sh_test( + name = "foo", + srcs = ["foo.sh"], + data = [":spaces"], +) +EOF + if "$is_windows"; then + 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 + else + cat > pkg/foo.sh <<'EOF' +#!/bin/bash +if [[ "$(cat $'pkg/a \n \\ b .txt')" != "my content" ]]; then + echo "unexpected content or not found" + exit 1 +fi +EOF + fi + chmod +x pkg/foo.sh +} + +function test_special_chars_in_runfiles_source_and_target_paths_out_of_process() { + setup_special_chars_in_runfiles_source_and_target_paths + bazel test --noexperimental_inprocess_symlink_creation \ + --test_output=errors \ + //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed" +} + +function test_special_chars_in_runfiles_source_and_target_paths_in_process() { + setup_special_chars_in_runfiles_source_and_target_paths + bazel test --experimental_inprocess_symlink_creation \ + --test_output=errors \ + //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 and +# a backslash. +function test_compatibility_with_bash_runfiles_library_snippet() { + if [[ "${PRODUCT_NAME}" != "bazel" ]]; then + # This test is only relevant for Bazel. + return + fi + # Create a workspace path with a space. + WORKSPACE="$(mktemp -d jar_manifest.XXXXXXXX)/my w\orkspace" + 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 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 b40c88a392e8b2..a574f4c750d76b 100644 --- a/tools/bash/runfiles/runfiles.bash +++ b/tools/bash/runfiles/runfiles.bash @@ -357,7 +357,25 @@ 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 or newline, it needs to be prefixed + # with a space and spaces, newlines, and backslashes have to be escaped as + # \s, \n, and \b. + if [[ "$1" == *" "* || "$1" == *$'\n'* ]]; then + local search_prefix=" $(echo -n "$1" | sed 's/\\/\\b/g; s/ /\\s/g')" + search_prefix="${search_prefix//$'\n'/\\n}" + local escaped=true + 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" + local escaped=false + fi + # The extra space below is added because cut counts from 1. + local trim_length=$(echo -n "$search_prefix " | wc -c) + # Escape the search prefix for use in the grep regex below *after* + # determining the trim length. + local 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 @@ -370,7 +388,27 @@ 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 [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then + echo >&2 "INFO[runfiles.bash]: rlocation($1): looking for prefix ($prefix)" + fi + if [[ "$prefix" == *" "* || "$prefix" == *$'\n'* ]]; then + search_prefix=" $(echo -n "$prefix" | sed 's/\\/\\b/g; s/ /\\s/g')" + search_prefix="${search_prefix//$'\n'/\\n}" + escaped=true + if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then + echo >&2 "INFO[runfiles.bash]: rlocation($1): using escaped search prefix ($search_prefix)" + fi + else + search_prefix="$prefix" + escaped=false + 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 "$(echo -n "$search_prefix" | sed 's/[.[\*^$]/\\&/g') " "${RUNFILES_MANIFEST_FILE}" | cut -b ${trim_length}-) + if [[ "$escaped" = true ]]; then + prefix_result="${prefix_result//\\n/$'\n'}" + prefix_result="${prefix_result//\\b/\\}" + fi [[ -z "$prefix_result" ]] && continue local -r candidate="${prefix_result}${1#"${prefix}"}" if [[ -e "$candidate" ]]; then @@ -400,6 +438,10 @@ function runfiles_rlocation_checked() { fi echo "" else + if [[ "$escaped" = true ]]; then + result="${result//\\n/$'\n'}" + result="${result//\\b/\\}" + fi if [[ -e "$result" ]]; then if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "INFO[runfiles.bash]: rlocation($1): found in manifest as ($result)" diff --git a/tools/bash/runfiles/runfiles_test.bash b/tools/bash/runfiles/runfiles_test.bash index c4e21983bca42c..2b47e5fe315b12 100755 --- a/tools/bash/runfiles/runfiles_test.bash +++ b/tools/bash/runfiles/runfiles_test.bash @@ -141,6 +141,11 @@ e/f $tmpdir/g h y $tmpdir/y c/dir $tmpdir/dir unresolved $tmpdir/unresolved + h/\si $tmpdir/ j k + h/\s\bi $tmpdir/ j k b + h/\n\bi $tmpdir/ \bnj k \na + dir\swith\sspaces $tmpdir/dir with spaces + space\snewline\nbackslash\b_dir $tmpdir/space newline\nbackslash\ba EOF mkdir "${tmpdir}/c" mkdir "${tmpdir}/y" @@ -149,7 +154,17 @@ 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" + touch "${tmpdir}/ j k b" + mkdir -p "${tmpdir}/dir with spaces/nested" + touch "${tmpdir}/dir with spaces/nested/file" + if ! is_windows; then + touch "${tmpdir}/ \nj k "$'\n'a + mkdir -p "${tmpdir}/space newline"$'\n'"backslash\a" + touch "${tmpdir}/space newline"$'\n'"backslash\a/f i\le" + fi export RUNFILES_DIR= export RUNFILES_MANIFEST_FILE=$tmpdir/foo.runfiles_manifest @@ -166,14 +181,32 @@ 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 "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 + if ! is_windows; then + [[ "$(rlocation $'h/\n\\i' || echo failed)" == "$tmpdir/ \nj k "$'\n'a ]] || fail + [[ "$(rlocation "space newline"$'\n'"backslash\_dir/f i\le" || echo failed)" == "${tmpdir}/space newline"$'\n'"backslash\a/f i\le" ]] || fail + fi + + rm -r "$tmpdir/c/d" "$tmpdir/g h" "$tmpdir/y" "$tmpdir/dir" "$tmpdir/unresolved" "$tmpdir/ j k" "$tmpdir/dir with spaces" + if ! is_windows; then + rm -r "$tmpdir/ \nj k "$'\n'a "${tmpdir}/space newline"$'\n'"backslash\a" + [[ -z "$(rlocation $'h/\n\\i' || echo failed)" ]] || fail + [[ -z "$(rlocation "space newline"$'\n'"backslash\_dir/f i\le" || echo failed)" ]] || fail + fi [[ -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() { diff --git a/tools/cpp/runfiles/runfiles_src.cc b/tools/cpp/runfiles/runfiles_src.cc index d81edb643f9cec..9da569fda0b58d 100644 --- a/tools/cpp/runfiles/runfiles_src.cc +++ b/tools/cpp/runfiles/runfiles_src.cc @@ -179,6 +179,39 @@ string GetEnv(const string& key) { #endif } +// Replaces \s, \n, and \b with their respective characters. +string Unescape(const string& path) { + 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; +} + string Runfiles::Rlocation(const string& path) const { return Rlocation(path, source_repository_); } @@ -254,18 +287,39 @@ bool ParseManifest(const string& path, map* 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] == ' ') { + // 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__ + << "): bad runfiles manifest entry in \"" << path << "\" line #" + << line_count << ": \"" << line << "\""; + *error = err.str(); + } + return false; } - return false; + source = Unescape(line.substr(1, idx - 1)); + target = Unescape(line.substr(idx + 1)); + } else { + string::size_type idx = line.find(' '); + 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; } diff --git a/tools/cpp/runfiles/runfiles_test.cc b/tools/cpp/runfiles/runfiles_test.cc index 122b184cafe1d0..b6d9b3e2323712 100644 --- a/tools/cpp/runfiles/runfiles_test.cc +++ b/tools/cpp/runfiles/runfiles_test.cc @@ -226,8 +226,16 @@ TEST_F(RunfilesTest, CannotCreateManifestBasedRunfilesDueToBadManifest) { } TEST_F(RunfilesTest, ManifestBasedRunfilesRlocationAndEnvVars) { - unique_ptr mf(MockFile::Create( - "foo" LINE_AS_STRING() ".runfiles_manifest", {"a/b c/d"})); + unique_ptr mf( + MockFile::Create("foo" LINE_AS_STRING() ".runfiles_manifest", + { + "a/b c/d", + "e/f target path with spaces", + " h/\\si j k", + " dir\\swith\\sspaces l/m", + " h/\\n\\s\\bi j k \\n\\b", + "not_escaped with\\backslash and spaces", + })); ASSERT_TRUE(mf != nullptr); string error; @@ -256,6 +264,15 @@ 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("h/\n \\i"), "j k \n\\"); + EXPECT_EQ(r->Rlocation("dir with spaces"), "l/m"); + EXPECT_EQ(r->Rlocation("dir with spaces/file"), "l/m/file"); + EXPECT_EQ(r->Rlocation("not_escaped"), "with\\backslash and spaces"); } TEST_F(RunfilesTest, DirectoryBasedRunfilesRlocationAndEnvVars) { diff --git a/tools/java/runfiles/Runfiles.java b/tools/java/runfiles/Runfiles.java index 97fbfc5998b35e..cde61736a3fc75 100644 --- a/tools/java/runfiles/Runfiles.java +++ b/tools/java/runfiles/Runfiles.java @@ -490,11 +490,36 @@ private static Map loadRunfiles(String path) throws IOException try (BufferedReader r = new BufferedReader( new InputStreamReader(new FileInputStream(path), StandardCharsets.UTF_8))) { - String line = null; + String line; while ((line = r.readLine()) != null) { - int index = line.indexOf(' '); - String runfile = (index == -1) ? line : line.substring(0, index); - String realPath = (index == -1) ? line : line.substring(index + 1); + String runfile; + String realPath; + if (line.startsWith(" ")) { + // 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 at least one space after the leading" + + " space: " + + line); + } + runfile = + line.substring(1, firstSpace) + .replace("\\s", " ") + .replace("\\n", "\n") + .replace("\\b", "\\"); + realPath = line.substring(firstSpace + 1).replace("\\n", "\n").replace("\\b", "\\"); + } else { + int firstSpace = line.indexOf(' '); + if (firstSpace == -1) { + throw new IOException( + "Invalid runfiles manifest line, expected at least one space: " + line); + } + runfile = line.substring(0, firstSpace); + realPath = line.substring(firstSpace + 1); + } result.put(runfile, realPath); } } diff --git a/tools/java/runfiles/testing/RunfilesTest.java b/tools/java/runfiles/testing/RunfilesTest.java index 92d420f77ce7bf..d3654d88b26ec2 100644 --- a/tools/java/runfiles/testing/RunfilesTest.java +++ b/tools/java/runfiles/testing/RunfilesTest.java @@ -247,14 +247,23 @@ public void testManifestBasedRlocation() throws Exception { ImmutableList.of( "Foo/runfile1 C:/Actual Path\\runfile1", "Foo/Bar/runfile2 D:\\the path\\run file 2.txt", - "Foo/Bar/Dir E:\\Actual Path\\Directory")); + "Foo/Bar/Dir E:\\Actual Path\\bDirectory", + " h/\\si F:\\bjk", + " dir\\swith\\sspaces F:\\bj k\\bdir with spaces", + " h/\\s\\n\\bi F:\\bjk\\nb")); 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"); - assertThat(r.rlocation("Foo/Bar/Dir")).isEqualTo("E:\\Actual Path\\Directory"); - assertThat(r.rlocation("Foo/Bar/Dir/File")).isEqualTo("E:\\Actual Path\\Directory/File"); + assertThat(r.rlocation("Foo/Bar/Dir")).isEqualTo("E:\\Actual Path\\bDirectory"); + assertThat(r.rlocation("Foo/Bar/Dir/File")).isEqualTo("E:\\Actual Path\\bDirectory/File"); assertThat(r.rlocation("Foo/Bar/Dir/Deeply/Nested/File")) - .isEqualTo("E:\\Actual Path\\Directory/Deeply/Nested/File"); + .isEqualTo("E:\\Actual Path\\bDirectory/Deeply/Nested/File"); + assertThat(r.rlocation("Foo/Bar/Dir/Deeply/Nested/File With Spaces")) + .isEqualTo("E:\\Actual Path\\bDirectory/Deeply/Nested/File With Spaces"); + assertThat(r.rlocation("h/ i")).isEqualTo("F:\\jk"); + assertThat(r.rlocation("h/ \n\\i")).isEqualTo("F:\\jk\nb"); + 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(); }