Skip to content

Commit 34d1f84

Browse files
fmeumfmeum
committed
New escaping scheme
1 parent 807c52c commit 34d1f84

File tree

11 files changed

+211
-81
lines changed

11 files changed

+211
-81
lines changed

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

+13-6
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919

2020
import com.google.common.annotations.VisibleForTesting;
2121
import com.google.common.collect.ImmutableList;
22+
import com.google.common.escape.CharEscaperBuilder;
23+
import com.google.common.escape.Escaper;
2224
import com.google.devtools.build.lib.actions.AbstractAction;
2325
import com.google.devtools.build.lib.actions.ActionExecutionContext;
2426
import com.google.devtools.build.lib.actions.ActionKeyContext;
@@ -62,7 +64,10 @@ public final class SourceManifestAction extends AbstractFileWriteAction
6264
private static final String GUID = "07459553-a3d0-4d37-9d78-18ed942470f4";
6365

6466
private static final Comparator<Map.Entry<PathFragment, Artifact>> ENTRY_COMPARATOR =
65-
(path1, path2) -> path1.getKey().getPathString().compareTo(path2.getKey().getPathString());
67+
Comparator.comparing(path -> path.getKey().getPathString());
68+
private static final Escaper ROOT_RELATIVE_PATH_ESCAPER =
69+
new CharEscaperBuilder().addEscape(' ', "\\s").addEscape('\\', "\\b").toEscaper();
70+
6671
private final Artifact repoMappingManifest;
6772
/**
6873
* Interface for defining manifest formatting and reporting specifics. Implementations must be
@@ -291,6 +296,9 @@ public enum ManifestType implements ManifestWriter {
291296
*
292297
* <p>[rootRelativePath] [resolvingSymlink]
293298
*
299+
* <p>If rootRelativePath contains spaces, then each backslash is replaced with '\b', each space
300+
* is replaced with '\s' and the line is prefixed with a space.
301+
*
294302
* <p>This strategy is suitable for creating an input manifest to a source view tree. Its output
295303
* is a valid input to {@link com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction}.
296304
*/
@@ -303,14 +311,13 @@ public void writeEntry(
303311
throws IOException {
304312
String rootRelativePathString = rootRelativePath.getPathString();
305313
// Source paths with spaces require escaping. Target paths with spaces don't as consumers
306-
// are expected to split on the first space (without escaping) or after the part indicated
307-
// by the length prefix (with escaping).
314+
// are expected to split on the first space.
308315
if (rootRelativePathString.indexOf(' ') != -1) {
309316
manifestWriter.append(' ');
310-
manifestWriter.append(String.valueOf(rootRelativePathString.length()));
311-
manifestWriter.append(' ');
317+
manifestWriter.append(ROOT_RELATIVE_PATH_ESCAPER.escape(rootRelativePathString));
318+
} else {
319+
manifestWriter.append(rootRelativePathString);
312320
}
313-
manifestWriter.append(rootRelativePathString);
314321
// This trailing whitespace is REQUIRED to process the single entry line correctly.
315322
manifestWriter.append(' ');
316323
if (symlinkTarget != null) {

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

+14-13
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
#include <fstream>
2222
#include <iostream>
23+
#include <regex>
2324
#include <sstream>
2425
#include <string>
2526
#include <unordered_map>
@@ -41,6 +42,9 @@ using std::wstring;
4142

4243
namespace {
4344

45+
const std::regex kEscapedBackslash(R"(\\b)");
46+
const std::regex kEscapedSpace(R"(\\s)");
47+
4448
const wchar_t* manifest_filename;
4549
const wchar_t* runfiles_base_dir;
4650

@@ -162,21 +166,18 @@ class RunfilesCreator {
162166
wstring link;
163167
wstring target;
164168
if (!line.empty() && line[0] == ' ') {
165-
// Lines starting with a space are of the form " 7 foo bar /tar get/path", with
166-
// the first field indicating the length of the runfiles path.
167-
std::size_t length_field_end = line.find_first_of(' ', 1);
168-
if (length_field_end == string::npos) {
169-
die(L"Invalid length field: %hs", line.c_str());
170-
}
171-
std::size_t link_length = std::stoul(line.substr(1, length_field_end - 1));
172-
std::size_t after_length_field = length_field_end + 1;
173-
if (line.size() < after_length_field + link_length || line[after_length_field + link_length] != ' ') {
174-
die(L"Invalid length field: %hs", line.c_str());
169+
// The link path contains escape sequences for spaces and backslashes.
170+
string::size_type idx = line.find(' ', 1);
171+
if (idx == string::npos) {
172+
die(L"Missing separator in manifest line: %hs", line.c_str());
175173
}
176-
link = blaze_util::CstringToWstring(line.substr(after_length_field, link_length));
177-
target = blaze_util::CstringToWstring(line.substr(after_length_field + link_length + 1));
174+
std::string link_path = line.substr(1, idx - 1);
175+
link_path = std::regex_replace(link_path, kEscapedSpace, " ");
176+
link_path = std::regex_replace(link_path, kEscapedBackslash, "\\");
177+
link = blaze_util::CstringToWstring(link_path);
178+
target = blaze_util::CstringToWstring(line.substr(idx + 1));
178179
} else {
179-
string::size_type idx = line.find_first_of(' ');
180+
string::size_type idx = line.find(' ');
180181
if (idx == string::npos) {
181182
die(L"Missing separator in manifest line: %hs", line.c_str());
182183
}

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

+14-10
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,15 @@
5454
#include <unistd.h>
5555

5656
#include <map>
57+
#include <regex>
5758
#include <string>
5859

5960
// program_invocation_short_name is not portable.
6061
static const char *argv0;
6162

63+
static const std::regex kEscapedBackslash(R"(\\b)");
64+
static const std::regex kEscapedSpace(R"(\\s)");
65+
6266
const char *input_filename;
6367
const char *output_base_dir;
6468

@@ -160,18 +164,18 @@ class RunfilesCreator {
160164
std::string link;
161165
const char *target;
162166
if (buf[0] == ' ') {
163-
// The line is of the form " 7 foo bar /tar get/path", with the first
164-
// field indicating the length of the source path.
165-
std::size_t length_field_length;
166-
std::size_t link_length = std::stoul(buf+1, &length_field_length);
167-
const char *after_length_field = buf + 1 + length_field_length + 1;
168-
target = after_length_field + link_length + 1;
169-
if (target >= buf + n || *(target - 1) != ' ') {
170-
DIE("invalid length field at line %d: '%s'\n", lineno, buf);
167+
// The link path contains escape sequences for spaces and backslashes.
168+
char *s = strchr(buf + 1, ' ');
169+
if (!s) {
170+
DIE("missing field delimiter at line %d: '%s'\n", lineno, buf);
171171
}
172-
link = std::string(after_length_field, link_length);
172+
link = std::string(buf + 1, s);
173+
link = std::regex_replace(link, kEscapedSpace, " ");
174+
link = std::regex_replace(link, kEscapedBackslash, "\\");
175+
target = s + 1;
173176
} else {
174-
// The line is of the form "foo /target/path", with only a single space.
177+
// The line is of the form "foo /target/path", with only a single space
178+
// in the link path.
175179
const char *s = strchr(buf, ' ');
176180
if (!s) {
177181
DIE("missing field delimiter at line %d: '%s'\n", lineno, buf);

‎src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java

+31
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,37 @@ public void testUnresolvedSymlink() throws Exception {
387387
""");
388388
}
389389

390+
@Test
391+
public void testEscaping() throws Exception {
392+
Artifact manifest = getBinArtifactWithNoOwner("manifest1");
393+
394+
ArtifactRoot trivialRoot =
395+
ArtifactRoot.asSourceRoot(Root.fromPath(rootDirectory.getRelative("trivial")));
396+
Path fileWithSpacePath = scratch.file("trivial/file with sp\\ace", "foo");
397+
Artifact fileWithSpaceAndBackslash = ActionsTestUtil.createArtifact(trivialRoot, fileWithSpacePath);
398+
399+
SourceManifestAction action =
400+
new SourceManifestAction(
401+
ManifestType.SOURCE_SYMLINKS,
402+
NULL_ACTION_OWNER,
403+
manifest,
404+
new Runfiles.Builder("TESTING", false)
405+
.addSymlink(PathFragment.create("no/sp\\ace"), buildFile)
406+
.addSymlink(PathFragment.create("also/no/sp\\ace"), fileWithSpaceAndBackslash)
407+
.addSymlink(PathFragment.create("with sp\\ace"), buildFile)
408+
.addSymlink(PathFragment.create("also/with sp\\ace"), fileWithSpaceAndBackslash)
409+
.build());
410+
411+
assertThat(action.getFileContents(reporter))
412+
.isEqualTo(
413+
"""
414+
TESTING/also/no/sp\\ace /workspace/trivial/file with sp\\ace
415+
TESTING/also/with\\ssp\\bace /workspace/trivial/file with sp\\ace
416+
TESTING/no/sp\\ace /workspace/trivial/BUILD
417+
TESTING/with\\ssp\\bace /workspace/trivial/BUILD
418+
""");
419+
}
420+
390421
private String computeKey(SourceManifestAction action) {
391422
Fingerprint fp = new Fingerprint();
392423
action.computeKey(actionKeyContext, /* artifactExpander= */ null, fp);

‎src/test/shell/integration/runfiles_test.sh

+90-4
Original file line numberDiff line numberDiff line change
@@ -569,7 +569,7 @@ EOF
569569
assert_contains '/link_two$' *-bin/a/go
570570
}
571571

572-
function test_spaces_in_runfiles_source_paths() {
572+
function setup_spaces_in_runfiles_source_paths() {
573573
mkdir -p pkg
574574
cat > pkg/defs.bzl <<'EOF'
575575
def _spaces_impl(ctx):
@@ -598,11 +598,21 @@ if [[ "$(cat "pkg/ a b .txt")" != "my content" ]]; then
598598
fi
599599
EOF
600600
chmod +x pkg/foo.sh
601+
}
602+
603+
function test_spaces_in_runfiles_source_paths_out_of_process() {
604+
setup_spaces_in_runfiles_source_paths
605+
bazel test --noexperimental_inprocess_symlink_creation \
606+
//pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed"
607+
}
601608

602-
bazel test //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed"
609+
function test_spaces_in_runfiles_source_paths_in_process() {
610+
setup_spaces_in_runfiles_source_paths
611+
bazel test --experimental_inprocess_symlink_creation \
612+
//pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed"
603613
}
604614

605-
function test_spaces_in_runfiles_source_and_target_paths() {
615+
function setup_spaces_in_runfiles_source_and_target_paths() {
606616
dir=$(mktemp -d 'runfiles test.XXXXXX')
607617
cd "$dir" || fail "failed to cd to $dir"
608618
touch MODULE.bazel
@@ -635,8 +645,84 @@ if [[ "$(cat "pkg/ a b .txt")" != "my content" ]]; then
635645
fi
636646
EOF
637647
chmod +x pkg/foo.sh
648+
}
649+
650+
function test_spaces_in_runfiles_source_and_target_paths_out_of_process() {
651+
setup_spaces_in_runfiles_source_and_target_paths
652+
bazel test --noexperimental_inprocess_symlink_creation \
653+
//pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed"
654+
}
655+
656+
function test_spaces_in_runfiles_source_and_target_paths_in_process() {
657+
setup_spaces_in_runfiles_source_and_target_paths
658+
bazel test --experimental_inprocess_symlink_creation \
659+
//pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed"
660+
}
661+
662+
# Verify that Bazel's runfiles manifest is compatible with v3 of the Bash
663+
# runfiles library snippet, even if the workspace path contains a space.
664+
function test_compatibility_with_bash_runfiles_library_snippet() {
665+
# Create a workspace path with a space.
666+
WORKSPACE_NAME="my workspace"
667+
trap "rm -rf \"$PWD/$WORKSPACE_NAME\"" EXIT
668+
mkdir -p "$WORKSPACE_NAME"
669+
cd "$WORKSPACE_NAME" || fail "failed to cd to $WORKSPACE_NAME"
670+
cat > MODULE.bazel <<'EOF'
671+
module(name = "my_module")
672+
EOF
673+
674+
mkdir pkg
675+
cat > pkg/BUILD <<'EOF'
676+
sh_binary(
677+
name = "tool",
678+
srcs = ["tool.sh"],
679+
deps = ["@bazel_tools//tools/bash/runfiles"],
680+
)
681+
682+
genrule(
683+
name = "gen",
684+
outs = ["out"],
685+
tools = [":tool"],
686+
cmd = "$(execpath :tool) $@",
687+
)
688+
EOF
689+
cat > pkg/tool.sh <<'EOF'
690+
#!/bin/bash
691+
# --- begin runfiles.bash initialization v3 ---
692+
# Copy-pasted from the Bazel Bash runfiles library v3.
693+
set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash
694+
# shellcheck disable=SC1090
695+
source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
696+
source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
697+
source "$0.runfiles/$f" 2>/dev/null || \
698+
source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
699+
source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
700+
{ echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
701+
# --- end runfiles.bash initialization v3 ---
702+
703+
if [[ ! -z "${RUNFILES_DIR+x}" ]]; then
704+
echo "RUNFILES_DIR is set"
705+
exit 1
706+
fi
707+
708+
if [[ -z "${RUNFILES_MANIFEST_FILE+x}" ]]; then
709+
echo "RUNFILES_MANIFEST_FILE is not set"
710+
exit 1
711+
fi
712+
713+
if [[ -z "$(rlocation "my_module/pkg/tool.sh")" ]]; then
714+
echo "rlocation failed"
715+
exit 1
716+
fi
717+
718+
touch $1
719+
EOF
720+
chmod +x pkg/tool.sh
638721

639-
bazel test //pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "test failed"
722+
bazel build --noenable_runfiles \
723+
--spawn_strategy=local \
724+
--action_env=RUNFILES_LIB_DEBUG=1 \
725+
//pkg:gen >&$TEST_log || fail "build failed"
640726
}
641727

642728
run_suite "runfiles"

‎tools/bash/runfiles/runfiles.bash

+14-6
Original file line numberDiff line numberDiff line change
@@ -357,10 +357,10 @@ function runfiles_rlocation_checked() {
357357
if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
358358
echo >&2 "INFO[runfiles.bash]: rlocation($1): looking in RUNFILES_MANIFEST_FILE ($RUNFILES_MANIFEST_FILE)"
359359
fi
360-
# If the rlocation path contains a space, it needs to be prefixed with
361-
# " <length> ", where <length> is the length of the path in bytes.
360+
# If the rlocation path contains a space, it needs to be prefixed with a
361+
# space and spaces and backslashes have to be escaped as \s and \b.
362362
if [[ "$1" == *" "* ]]; then
363-
local search_prefix=" $(echo -n "$1" | wc -c | tr -d ' ') $1"
363+
local search_prefix=" $(echo -n "$1" | sed 's/\\/\\b/g; s/ /\\s/g')"
364364
if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
365365
echo >&2 "INFO[runfiles.bash]: rlocation($1): using escaped search prefix ($search_prefix)"
366366
fi
@@ -369,7 +369,9 @@ function runfiles_rlocation_checked() {
369369
fi
370370
# The extra space below is added because cut counts from 1.
371371
local trim_length=$(echo -n "$search_prefix " | wc -c)
372-
local -r result=$(__runfiles_maybe_grep -m1 "^$search_prefix " "${RUNFILES_MANIFEST_FILE}" | cut -b ${trim_length}-)
372+
# Escape the search prefix for use in the grep regex below *after*
373+
# determining the trim length.
374+
local -r result=$(__runfiles_maybe_grep -m1 "^$(echo -n "$search_prefix" | sed 's/[.[\*^$]/\\&/g') " "${RUNFILES_MANIFEST_FILE}" | cut -b ${trim_length}-)
373375
if [[ -z "$result" ]]; then
374376
# If path references a runfile that lies under a directory that itself
375377
# is a runfile, then only the directory is listed in the manifest. Look
@@ -382,14 +384,20 @@ function runfiles_rlocation_checked() {
382384
new_prefix="${prefix%/*}"
383385
[[ "$new_prefix" == "$prefix" ]] && break
384386
prefix="$new_prefix"
387+
if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
388+
echo >&2 "INFO[runfiles.bash]: rlocation($1): looking for prefix ($prefix)"
389+
fi
385390
if [[ "$prefix" == *" "* ]]; then
386-
search_prefix=" $(echo -n "$prefix" | wc -c | tr -d ' ') $prefix"
391+
search_prefix=" $(echo -n "$prefix" | sed 's/\\/\\b/g; s/ /\\s/g')"
392+
if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
393+
echo >&2 "INFO[runfiles.bash]: rlocation($1): using escaped search prefix ($search_prefix)"
394+
fi
387395
else
388396
search_prefix="$prefix"
389397
fi
390398
# The extra space below is added because cut counts from 1.
391399
trim_length=$(echo -n "$search_prefix " | wc -c)
392-
prefix_result=$(__runfiles_maybe_grep -m1 "^$search_prefix " "${RUNFILES_MANIFEST_FILE}" | cut -b ${trim_length}-)
400+
prefix_result=$(__runfiles_maybe_grep -m1 "$(echo -n "$search_prefix" | sed 's/[.[\*^$]/\\&/g') " "${RUNFILES_MANIFEST_FILE}" | cut -b ${trim_length}-)
393401
[[ -z "$prefix_result" ]] && continue
394402
local -r candidate="${prefix_result}${1#"${prefix}"}"
395403
if [[ -e "$candidate" ]]; then

‎tools/bash/runfiles/runfiles_test.bash

+5-2
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,9 @@ e/f $tmpdir/g h
141141
y $tmpdir/y
142142
c/dir $tmpdir/dir
143143
unresolved $tmpdir/unresolved
144-
4 h/ i $tmpdir/ j k
145-
15 dir with spaces $tmpdir/dir with spaces
144+
h/\si $tmpdir/ j k
145+
h/\s\bi $tmpdir/ j k b
146+
dir\swith\sspaces $tmpdir/dir with spaces
146147
EOF
147148
mkdir "${tmpdir}/c"
148149
mkdir "${tmpdir}/y"
@@ -154,6 +155,7 @@ EOF
154155
touch "${tmpdir}/dir/deeply/nested/file with spaces"
155156
ln -s /does/not/exist "${tmpdir}/unresolved"
156157
touch "${tmpdir}/ j k"
158+
touch "${tmpdir}/ j k b"
157159
mkdir -p "${tmpdir}/dir with spaces/nested"
158160
touch "${tmpdir}/dir with spaces/nested/file"
159161

@@ -175,6 +177,7 @@ EOF
175177
[[ "$(rlocation "c/dir/deeply/nested/file with spaces" || echo failed)" == "$tmpdir/dir/deeply/nested/file with spaces" ]] || fail
176178
[[ -z "$(rlocation unresolved || echo failed)" ]] || fail
177179
[[ "$(rlocation "h/ i" || echo failed)" == "$tmpdir/ j k" ]] || fail
180+
[[ "$(rlocation "h/ \i" || echo failed)" == "$tmpdir/ j k b" ]] || fail
178181
[[ "$(rlocation "dir with spaces" || echo failed)" == "$tmpdir/dir with spaces" ]] || fail
179182
[[ "$(rlocation "dir with spaces/nested/file" || echo failed)" == "$tmpdir/dir with spaces/nested/file" ]] || fail
180183
rm -r "$tmpdir/c/d" "$tmpdir/g h" "$tmpdir/y" "$tmpdir/dir" "$tmpdir/unresolved" "$tmpdir/ j k" "$tmpdir/dir with spaces"

0 commit comments

Comments
 (0)