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

[7.4.0] Allow all characters in runfile source and target paths #23912

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented 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

…ct`.

There is only one caller that needs to do the transformation from `Artifact` to `PathFragment`, so pushing it there doesn't lead to any duplication.

PiperOrigin-RevId: 645026800
Change-Id: I04cb980a547442c168c9ad1363b7a7d36dbb7d65
(cherry picked from commit f311efc)
@fmeum fmeum requested a review from a team as a code owner October 9, 2024 08:30
@fmeum fmeum requested review from lberki and removed request for a team October 9, 2024 08:30
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-CPP Issues for C++ rules team-Rules-Java Issues for Java rules labels 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)
@keertk keertk added this pull request to the merge queue Oct 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2024
@keertk keertk added this pull request to the merge queue Oct 9, 2024
github-merge-queue bot pushed a commit that referenced this pull request 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 <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2024
@iancha1992 iancha1992 added this pull request to the merge queue Oct 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2024
@iancha1992 iancha1992 added this pull request to the merge queue Oct 9, 2024
Merged via the queue into bazelbuild:release-7.4.0 with commit c911530 Oct 9, 2024
50 checks passed
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Oct 9, 2024
@fmeum fmeum deleted the 23715-cherry branch October 9, 2024 18:43
github-merge-queue bot pushed a commit to bazelbuild/rules_python that referenced this pull request Nov 28, 2024
…#2456)

Bazel 7.4.0 introduced support for all characters in runfile source and
target paths: bazelbuild/bazel#23912

This is a backwards-compatible change, based on a similar change in
rules_go: bazel-contrib/rules_go#4136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-CPP Issues for C++ rules team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants