Skip to content

Commit

Permalink
Allow all characters in runfile source and target paths
Browse files Browse the repository at this point in the history
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 bazelbuild#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 bazelbuild#23331.

PiperOrigin-RevId: 683362776
Change-Id: I1eb79217dcd53cef0089d62a7ba477b1d8f52c21

(cherry picked from commit 7407cef)
  • Loading branch information
fmeum committed Oct 9, 2024
1 parent caa7e00 commit c07866c
Show file tree
Hide file tree
Showing 16 changed files with 688 additions and 67 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -61,7 +63,16 @@ public final class SourceManifestAction extends AbstractFileWriteAction
private static final String GUID = "07459553-a3d0-4d37-9d78-18ed942470f4";

private static final Comparator<Map.Entry<PathFragment, Artifact>> 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
Expand Down Expand Up @@ -291,6 +302,9 @@ public enum ManifestType implements ManifestWriter {
*
* <p>[rootRelativePath] [resolvingSymlink]
*
* <p>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.
*
* <p>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}.
*/
Expand All @@ -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');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/main/protobuf/failure_details.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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 }];
}

Expand Down
68 changes: 51 additions & 17 deletions src/main/tools/build-runfiles-windows.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
60 changes: 52 additions & 8 deletions src/main/tools/build-runfiles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,39 @@ struct FileInfo {

typedef std::map<std::string, FileInfo> 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)
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/test/shell/bazel/bazel_determinism_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
2 changes: 1 addition & 1 deletion src/test/shell/bazel/workspace_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}

Expand Down
Loading

0 comments on commit c07866c

Please sign in to comment.