Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Runfiles: support paths with spaces #4327

Closed
laszlocsomor opened this issue Dec 19, 2017 · 44 comments
Closed

Runfiles: support paths with spaces #4327

laszlocsomor opened this issue Dec 19, 2017 · 44 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: feature request

Comments

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Dec 19, 2017

Update: this bug no longer claims to be Windows-specific, see #4327 (comment)


Fix this TODO:

// TODO(bazel-team): This is buggy when the path contains spaces, we should fix the
// manifest format.

@laszlocsomor laszlocsomor self-assigned this Dec 19, 2017
@laszlocsomor laszlocsomor changed the title runfiles: support paths with spaces Windows, runfiles: support paths with spaces Dec 19, 2017
@izzyleung
Copy link

Hmm, this problem also applies to other platforms as well, say macOS, when running bazel build //... on a workspace living under a directory with name containing space(s), bael will tell me:

ERROR: bazel does not currently work properly from paths containing spaces (com.google.devtools.build.lib.runtime.BlazeWorkspace@5dc63968).

@laszlocsomor
Copy link
Contributor Author

@izzyleung thanks for the info! In that case I think P2 or even P3 is safe. I thought this worked on other platforms.

@laszlocsomor laszlocsomor added P2 We'll consider working on this in future. (Assignee optional) and removed P1 I'll work on this now. (Assignee required) labels Dec 20, 2017
@laszlocsomor
Copy link
Contributor Author

Update: I haven't looked into fixing this issue and I'm most likely not going to for the next couple months.

@laszlocsomor
Copy link
Contributor Author

Dropping priority. Spaces aren't well supported on Linux either. It'd be nice if they were fully supported, but this doesn't seem like a critical feature.

Demo time:

Directory structure:

  $ find .
.
./WORKSPACE
./sub
./sub/a b
./sub/a b/bar
./sub/a b/bar/x.txt
./bin.sh
./BUILD

BUILD file:

sh_test(
    name = "bin",
    srcs = ["bin.sh"],
    data = glob(["sub/**"]),
)

genrule(
    name = "gen",
    outs = ["gen.txt"],
    srcs = glob(["sub/**"]),
    cmd = "( for f in $(SRCS); do echo $$f; done ; ) > $@",
)

bin.sh:

#!/bin/bash
echo "Hello test"
find .
false

Space aren't supported in data dependencies:

  $ bazel build :bin
INFO: Analysed target //:bin (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: /tmp/aaaaaaaa.aaa/runfile_spaces/BUILD:1:1: Creating runfiles tree bazel-out/k8-fastbuild/bin/bin.runfiles failed: build-runfiles failed: error executing command 
  (cd /usr/local/google/home/laszlocsomor/.cache/bazel/_bazel_laszlocsomor/b8ecdd60a1b4498304771172bdb140cf/execroot/__main__ && \
  exec env - \
    PATH=/usr/local/google/home/laszlocsomor/bazel-releases/current/bin:/usr/local/google/home/laszlocsomor/bazel-releases/current/bin:/usr/lib/google-golang/bin:/usr/local/buildtools/java/jdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin \
  /usr/local/google/home/laszlocsomor/.cache/bazel/_bazel_laszlocsomor/install/4cfcf40fe067e89c8f5c38e156f8d8ca/_embedded_binaries/build-runfiles bazel-out/k8-fastbuild/bin/bin.runfiles_manifest bazel-out/k8-fastbuild/bin/bin.runfiles): Process exited with status 1: Process exited with status 1
/usr/local/google/home/laszlocsomor/.cache/bazel/_bazel_laszlocsomor/install/4cfcf40fe067e89c8f5c38e156f8d8ca/_embedded_binaries/build-runfiles (args bazel-out/k8-fastbuild/bin/bin.runfiles_manifest bazel-out/k8-fastbuild/bin/bin.runfiles): link or target filename contains space on line 3: '__main__/sub/a b/bar/x.txt /tmp/aaaaaaaa.aaa/runfile_spaces/sub/a b/bar/x.txt'

Target //:bin failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.187s, Critical Path: 0.01s
INFO: 0 processes.
FAILED: Build did NOT complete successfully

Spaces don't play well with genrule's $(SRCS).

  $ bazel build :gen && cat bazel-genfiles/gen.txt
...
INFO: Build completed successfully, 1 total action
sub/a
b/bar/x.txt

@laszlocsomor laszlocsomor added P3 We're not considering working on this, but happy to review a PR. (No assignee) untriaged team-Bazel General Bazel product/strategy issues and removed P2 We'll consider working on this in future. (Assignee optional) platform: windows area-Windows Windows-specific issues and feature requests labels Feb 28, 2019
@laszlocsomor laszlocsomor removed their assignment Feb 28, 2019
@laszlocsomor laszlocsomor changed the title Windows, runfiles: support paths with spaces Runfiles: support paths with spaces Feb 28, 2019
@dslomov dslomov added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc and removed team-Bazel General Bazel product/strategy issues untriaged labels Jul 23, 2019
copybara-service bot pushed a commit that referenced this issue Aug 18, 2023
The bulk of this CL is just plumbing the flag from the configuration into the RunfilesSupplier implementations. The functional changes are in RunfilesTreeUpdater; compare with the preexisting SymlinkTreeStrategy implementation.

Additionally, I've fixed some variable and method names to exactly match the flags defined in CoreOptions; the code is less confusing and easier to refactor that way.

The motivation behind this change is to investigate the feasibility of defaulting to the in-process implementation and deleting the out-of-process one, which has known issues (see #4327).

PiperOrigin-RevId: 558112830
Change-Id: I4c509be665776013ffc2d483694df515d5ab32f2
copybara-service bot pushed a commit to google/j2cl that referenced this issue Oct 5, 2023
…und it.

For more information:
bazelbuild/bazel#4327.

PiperOrigin-RevId: 571152898
copybara-service bot pushed a commit to google/elemental2 that referenced this issue Oct 12, 2023
…paths with spaces for runfiles. More information about the issue can be found in bazelbuild/bazel#4327.

PiperOrigin-RevId: 572980749
copybara-service bot pushed a commit to google/jsinterop-annotations that referenced this issue Oct 12, 2023
…paths with spaces for runfiles. More information about the issue can be found in bazelbuild/bazel#4327.

PiperOrigin-RevId: 572980749
copybara-service bot pushed a commit to google/jsinterop-generator that referenced this issue Oct 12, 2023
…paths with spaces for runfiles. More information about the issue can be found in bazelbuild/bazel#4327.

PiperOrigin-RevId: 572980749
copybara-service bot pushed a commit to google/j2cl that referenced this issue Oct 12, 2023
…paths with spaces for runfiles. More information about the issue can be found in bazelbuild/bazel#4327.

PiperOrigin-RevId: 572980749
copybara-service bot pushed a commit to google/jsinterop-base that referenced this issue Oct 12, 2023
…paths with spaces for runfiles. More information about the issue can be found in bazelbuild/bazel#4327.

PiperOrigin-RevId: 572980749
@mikhail-g
Copy link

Is there a way to use a delimiter other then a whitespace for Runfiles manifest entry? e.g. , : or a tab symbol

@sluongng
Copy link
Contributor

It seems like the workaround/fix for this issue is to turn on --experimental_inprocess_symlink_creation.

The only drawback mentioned above was fixed in 7b87ae1 which landed in Bazel 7.0.0.

There has been another inquiry on Slack about spaces in the runfiles. I suggest we turn the flag on by default in 7.3.0.

As for the manifest file itself, I think #14676 is "almost" there but needs some additional effort to take it over the finish line. If folks are interested in making the runfiles manifest format a bit easier to parse, take a look there.

@adamscybot
Copy link

adamscybot commented Aug 6, 2024

@sluongng I would just like to point out this breaking issue with rules_js that I found when --experimental_inprocess_symlink_creation is turned on in Bazel 7.2.

Unsure if its Bazel, or the rules that aren't vibing with this in the way that is expected, but the rule set is popular enough you may want to see it resolved first.

I suspect I'm the first-mover to come across this.

Here is the ticket: aspect-build/rules_js#1877

@abravalheri
Copy link

abravalheri commented Aug 27, 2024

Cross referencing this issue as the likely root cause of the problem in pypa/setuptools#4605.

As previously noted in bazelbuild/rules_python#617 previous workarounds are not bullet proof and there was no guarantee that similar problems would not happen again due the limitation in bazel itself (it is not really a bug in setuptools codebase).

CheckmkCI pushed a commit to Checkmk/checkmk that referenced this issue Oct 1, 2024
Bazel doesn't handle that gracefully.
See bazelbuild/bazel#4327

And `--experimental_inprocess_symlink_creation` doesn't currenly work
very well in our repo either.

CMK-19117

Change-Id: I2ffeca787241b23af088b9ad19b6ddc59969a664
fmeum added a commit to fmeum/bazel that referenced this issue Oct 9, 2024
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)
fmeum added a commit to fmeum/bazel that referenced this issue Oct 9, 2024
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)
github-merge-queue bot pushed a commit that referenced this issue Oct 9, 2024
(contains the refactor in f311efc to
reduce merge conflicts)

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 commits
7407cef
and f311efc)

Closes #23715

---------

Co-authored-by: Googler <jhorvitz@google.com>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.4.0 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.4.0rc1. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: feature request
Projects
None yet