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.

Sign up for GitHub

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

Jump to bottom

Use long executable path instead of argv[0] in all launchers #16916

Closed
wants to merge 3 commits into from
Closed

Use long executable path instead of argv[0] in all launchers #16916

wants to merge 3 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 3, 2022 •
edited
Loading

argv[0] can differ from the path of the launcher executable. The latter can contain 8.3 style filenames, which need to be resolved to long paths before path manipulation (e.g. appending ".runfiles") can succeed.

The Python launcher handled this correctly, but other launchers didn't use the long executable path consistently.

@fmeum
Use long executable path instead of argv[0] in all launchers
70e60d9 
`argv[0]` can differ from the path of the launcher executable and can
contain 8.3 style filenames, which need to be resolved to long paths
before path manipulation (e.g. appending ".runfiles") can succeed.

The Python launcher handled this correctly, but other launchers didn't
use the long executable path consistently and thus spuriously failed
when Bazel emitted an 8.3 path.
@fmeum fmeum marked this pull request as ready for review December 3, 2022 21:38
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 3, 2022 •
edited
Loading

@meteorcloudy Could you review this bug fix?

I hit in a real world scenario with a Java tool that ended up falling back to the non-existent runfiles directory as it couldn't find the manifest. The error caused by that is indeed very confusing.

I will flag this for cherry-picking into some Bazel 6 release.

@fmeum fmeum mentioned this pull request Dec 3, 2022
Closed
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 3, 2022

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 3, 2022
@sgowroji sgowroji added team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website team-Documentation Documentation improvements that cannot be directly linked to other team labels awaiting-review PR is awaiting review from an assigned reviewer labels Dec 4, 2022
@meteorcloudy meteorcloudy requested review from meteorcloudy and removed request for gregestren and fweikert December 5, 2022 11:19
@meteorcloudy meteorcloudy removed the team-Documentation Documentation improvements that cannot be directly linked to other team labels label Dec 5, 2022
meteorcloudy
meteorcloudy requested changes Dec 5, 2022
View reviewed changes
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Maybe we should add a simple test case in https://github.com/bazelbuild/bazel/blob/master/src/test/py/bazel/launcher_test.py#L615?

@meteorcloudy
Copy link
Member

@fmeum Since this is not a new regression in Bazel 6.0, I prefer to cherry pick it into 6.1 to not delay the release of 6.0 (tomorrow), WDYT?

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 5, 2022

Sounds good!

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 5, 2022

@meteorcloudy I added tests. In fact, I added one for invalid argv[0] and fixed the one for short paths as I believe it didn't work as expected as the executable basenames used in the test were all valid 8.3 filenames.

@meteorcloudy
Copy link
Member

Ah, thanks! That explains why we didn't catch this bug before.

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 5, 2022
@meteorcloudy
Copy link
Member

@bazel-io fork 6.1.0

@bazel-io bazel-io mentioned this pull request Dec 5, 2022
Closed
@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 5, 2022
@copybara-service copybara-service bot closed this in 53e9fea Dec 7, 2022
@fmeum fmeum deleted the java-launcher-argv0-fixes branch December 7, 2022 17:17
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Dec 8, 2022
@ShreeM01 ShreeM01 mentioned this pull request Jan 19, 2023
Merged
ShreeM01 added a commit that referenced this pull request Jan 20, 2023
@ShreeM01 @fmeum
Use long executable path instead of argv[0] in all launchers (#17272)
795779e 
`argv[0]` can differ from the path of the launcher executable. The latter can contain 8.3 style filenames, which need to be resolved to long paths before path manipulation (e.g. appending ".runfiles") can succeed.

The Python launcher handled this correctly, but other launchers didn't use the long executable path consistently.

Closes #16916.

PiperOrigin-RevId: 493615543
Change-Id: Ic8161890181c0110ecdf6893b9835e6f99d01097

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meteorcloudy meteorcloudy meteorcloudy approved these changes

Assignees
No one assigned
Labels
team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fmeum @meteorcloudy @bazel-io @sgowroji