From 6e4072eeee16582531918d4ab1988f5df9fd550f Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 7 Oct 2024 16:10:37 -0700 Subject: [PATCH] Allow all characters in runfile source and target paths Adds support for spaces and newlines in source and target paths of runfiles symlinks to `build-runfiles` as well as to the Bash, C++, and Java runfiles libraries (the Python runfiles library has moved out of the repo). If a symlink has spaces or newlines in its source path, it is prefixed with a space in the manifest and spaces, newlines, and backslashes in the source path are escaped with `\s`, `\n`, and `\b` respectively. This scheme has been chosen as it has the following properties: 1. There is no change to the format of manifest lines for entries whose source and target paths don't contain a space. This ensures compatibility with existing runfiles libraries. 2. There is even no change to the format of manifest lines for entries whose target path contains spaces but whose source path does not. These manifests previously failed in `build-runfiles`, but were usable on Windows and supported by the official runfiles libraries. This also ensures that the initialization snippet for the Bash runfiles library doesn't need to change, even when used on Unix with a space in the output base path. 3. The scheme is fully reversible and only depends on the source path, which gives runfiles libraries a choice between reversing the escaping when parsing the manifest (C++, Java) or applying the escaping when searching the manifest (Bash). Fixes #4327 RELNOTES: Bazel now supports all characters in the rlocation and target paths of runfiles and can be run from workspaces with a space in their full path. Closes #23331. PiperOrigin-RevId: 683362776 Change-Id: I1eb79217dcd53cef0089d62a7ba477b1d8f52c21 (cherry picked from commit 7407cef3837b4fe14e48e1a925f9b886c0cc945d) --- .../lib/analysis/SourceManifestAction.java | 41 ++- .../build/lib/runtime/BlazeOptionHandler.java | 12 - src/main/protobuf/failure_details.proto | 2 +- src/main/tools/build-runfiles-windows.cc | 68 +++-- src/main/tools/build-runfiles.cc | 60 +++- .../google/devtools/build/lib/analysis/BUILD | 2 +- .../analysis/SourceManifestActionTest.java | 73 +++++ .../shell/bazel/bazel_determinism_test.sh | 3 +- src/test/shell/bazel/workspace_test.sh | 2 +- src/test/shell/integration/runfiles_test.sh | 268 ++++++++++++++++++ tools/bash/runfiles/runfiles.bash | 46 ++- tools/bash/runfiles/runfiles_test.bash | 35 ++- tools/cpp/runfiles/runfiles_src.cc | 74 ++++- tools/cpp/runfiles/runfiles_test.cc | 21 +- tools/java/runfiles/Runfiles.java | 33 ++- tools/java/runfiles/testing/RunfilesTest.java | 17 +- 16 files changed, 690 insertions(+), 67 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 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..be3d06d411f8b6 100755 --- a/src/test/shell/integration/runfiles_test.sh +++ b/src/test/shell/integration/runfiles_test.sh @@ -511,4 +511,272 @@ 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 \ + --enable_bzlmod \ + --noenable_workspace \ + --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(); }