-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Windows, native launcher: use C++ runfiles library #7853
Windows, native launcher: use C++ runfiles library #7853
Conversation
In this PR: - //src/tools/launcher now uses the C++ runfiles library, depending on it's source and not on the released version under @bazel_tools (so it's built with the same runfiles library sources as those embedded into the Bazel binary) - The launcher no longer discovers nor loads the runfiles manifest. Fixes bazelbuild#7809
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
Looks like there are some test failures
} | ||
query_path += L"/" + path; | ||
return query_path; | ||
if (path.find(L"external/") == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if users has an external
directory in their source tree? This could be wrong. Maybe path.startwith(L"external/")
is better? Or just don't check it at all, because <workspace_name>/external/<external_repo_name><file_path_in_external_repo>
also exits in manifest file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checks if "external/" is found at index 0, i.e. same as startswith(). There's no startswith() function.
Change-Id: I135892862f7fb2d9f6c0492f2071507d4f14b1d3
Change-Id: I417d90fec446a127793a4ed133e7b1f611240a26
Change-Id: I5439f2012432cc95eca382371b0da7f72155cd58
I added d2649fa to fix the Windows tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM!
The native launcher's own Rlocation() implementation correctly handles runfile paths that start with "external/". Fixes bazelbuild#7809 This PR subsumes bazelbuild#7853. That PR changed the launcher to use the C++ runfiles library. Alas that broke some tests.
Redone as #7873 |
In this PR:
//src/tools/launcher now uses the C++ runfiles
library, depending on it's source and not on
the released version under @bazel_tools (so it's
built with the same runfiles library sources as
those embedded into the Bazel binary)
The launcher no longer discovers nor loads the
runfiles manifest.
Fixes #7809