Skip to content

Support escaped runfile paths#3910

Merged
illicitonion merged 2 commits intobazelbuild:mainfrom
meroton:escaped-manifests
Mar 23, 2026
Merged

Support escaped runfile paths#3910
illicitonion merged 2 commits intobazelbuild:mainfrom
meroton:escaped-manifests

Conversation

@moroten
Copy link
Contributor

@moroten moroten commented Mar 20, 2026

Add support for spaces, newlines and backslashes in paths written to the runfiles MANIFEST file.

@moroten moroten force-pushed the escaped-manifests branch from 54b3d78 to 14e2fae Compare March 23, 2026 08:33
@moroten
Copy link
Contributor Author

moroten commented Mar 23, 2026

Rebased: 54b3d78a9 -> 14e2faeb9

@moroten
Copy link
Contributor Author

moroten commented Mar 23, 2026

CI fails for //rust/runfiles:runfiles_test on Bazel 7.4.1 but passes on my local machine. Is there a way to look at the test output?

@UebelAndre
Copy link
Collaborator

UebelAndre commented Mar 23, 2026

Seems like you unfortunately hit bazelbuild/continuous-integration#1838

If you run bazel coverage //rust/runfiles:runfiles_test can you see the test failures?

@moroten
Copy link
Contributor Author

moroten commented Mar 23, 2026

Coverage succeeds locally.

@UebelAndre
Copy link
Collaborator

UebelAndre commented Mar 23, 2026

I opened bazelbuild/continuous-integration#2508 but in the mean time you could update the presubmit file for this job to add --test_output=errors to coverage_flags to force information to start appearing.

@moroten moroten force-pushed the escaped-manifests branch from 54cba5f to 14e2fae Compare March 23, 2026 10:56
@moroten
Support escaped runfile paths
97481c5 
Add support for spaces, newlines and backslashes in paths written to the
runfiles MANIFEST file.
@moroten moroten force-pushed the escaped-manifests branch from 945e638 to 97481c5 Compare March 23, 2026 10:58
UebelAndre
UebelAndre requested changes Mar 23, 2026
View reviewed changes
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just one request!

rust/runfiles/runfiles.rs Outdated
.ok_or(RunfilesError::RunfilesManifestInvalidFormat)?;
Ok::<(PathBuf, PathBuf), RunfilesError>((pair.0.into(), pair.1.into()))
let pair = if line.starts_with(' ') {
// Unescape according to src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java.
Copy link
Collaborator

@UebelAndre UebelAndre Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you leave a link with a commit hash in it so the context isn't invalidated by upstream changes when folks go to look this up?

Suggested change
// Unescape according to src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java.
// Unescape according to SourceManifestAction.java
// https://github.com/bazelbuild/bazel/blob/5b3e143adf7ccf40b05f73b0711f60f1ceade180/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java

Copy link
Contributor Author

@moroten moroten Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I choose a link with line numbers as well. Thank you for the review!

@moroten moroten force-pushed the escaped-manifests branch from 8479325 to 97481c5 Compare March 23, 2026 18:55
@UebelAndre
Copy link
Collaborator

UebelAndre commented Mar 23, 2026

Looks like CI failed with a network flake 😞

If you push a new commit (e.g. git commit --amend --date=now) then you'll probably get lucky enough for a passing build

@moroten moroten force-pushed the escaped-manifests branch from 4dedc4b to 642ecb3 Compare March 23, 2026 19:14
@illicitonion illicitonion enabled auto-merge March 23, 2026 19:54
@illicitonion illicitonion added this pull request to the merge queue Mar 23, 2026
Merged via the queue into bazelbuild:main with commit 487ea0e Mar 23, 2026
3 checks passed
@moroten moroten deleted the escaped-manifests branch March 23, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UebelAndre UebelAndre UebelAndre approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@moroten @UebelAndre @illicitonion