-
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
Add optional startup --linux_bazel_path_from_getauxval #14391
Conversation
Add support for multiarch docker builds for tests. Test execution still fails for non-native targets due to an Bazel bug that is fixed in bazelbuild/bazel#14391. No Bazel releases contain this fix for now, so will have to wait a bit to make use of non-native test execution. Signed-off-by: Jarno Rajahalme <[email protected]>
Add support for multiarch docker builds for tests. Test execution still fails for non-native targets due to an Bazel bug that is fixed in bazelbuild/bazel#14391. No Bazel releases contain this fix for now, so will have to wait a bit to make use of non-native test execution. Signed-off-by: Jarno Rajahalme <[email protected]>
Add support for multiarch docker builds for tests. Test execution still fails for non-native targets due to an Bazel bug that is fixed in bazelbuild/bazel#14391. No Bazel releases contain this fix for now, so will have to wait a bit to make use of non-native test execution. Signed-off-by: Jarno Rajahalme <[email protected]>
Add support for multiarch docker builds for tests. Test execution still fails for non-native targets due to an Bazel bug that is fixed in bazelbuild/bazel#14391. No Bazel releases contain this fix for now, so will have to wait a bit to make use of non-native test execution. Signed-off-by: Jarno Rajahalme <[email protected]>
Wow! |
Add support for multiarch docker builds for tests. Test execution still fails for non-native targets due to an Bazel bug that is fixed in bazelbuild/bazel#14391. No Bazel releases contain this fix for now, so will have to wait a bit to make use of non-native test execution. Signed-off-by: Jarno Rajahalme <[email protected]>
I'm running into the |
Add support for multiarch docker builds for tests. Test execution still fails for non-native targets due to an Bazel bug that is fixed in bazelbuild/bazel#14391. No Bazel releases contain this fix for now, so will have to wait a bit to make use of non-native test execution. Signed-off-by: Jarno Rajahalme <[email protected]>
Add support for multiarch docker builds for tests. Test execution still fails for non-native targets due to an Bazel bug that is fixed in bazelbuild/bazel#14391. No Bazel releases contain this fix for now, so will have to wait a bit to make use of non-native test execution. Signed-off-by: Jarno Rajahalme <[email protected]>
src/main/cpp/blaze_util_linux.cc
Outdated
@@ -86,7 +87,7 @@ string GetSelfPath(const char* argv0) { | |||
// The file to which this symlink points could change contents or go missing | |||
// concurrent with execution of the Bazel client, so we don't eagerly resolve | |||
// it. | |||
return "/proc/self/exe"; | |||
return std::string((char *)getauxval(AT_EXECFN)); |
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.
supernit: getauxval can technically return nullptr (maybe check that). Also consider a more C++ reinterpret_cast<const char*>
.
Sorry for the silly question, but you mentioned running on M1 Mac in the description -- is the change in blaze_util_linux.cc
sufficient for that? Or is that the case of docker running a Linux image on M1?
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.
I think the main question here is about potentially relative vs absolute paths (also why /proc/self/exe
is not Bazel in this case). This is the path used to start the process so it can be the absolute path of the process but it also could be a symlink/relative path. The danger here is that if we end up changing the working directory at any point, GetSelfPath
may not point us effectively to usable self anymore. Granted, I don't think we do that nor that we need that path a lot of times, but it is something to keep in mind.
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.
Sorry for the silly question, but you mentioned running on M1 Mac in the description -- is the change in blaze_util_linux.cc sufficient for that? Or is that the case of docker running a Linux image on M1?
This is running a Linux/x86_64 container on Linux/arm. This is how Docker for Mac works under the hood. It runs a Linux VM running arm Linux and then installs runs the container in that with a binfmt_misc handler set up to run x86_64 binaries through qemu-user-static. This is useful because you usually want to run the same container and environment on your dev host as you do in your CI environment, rather than rebuilding everything for an extra platform. Technically, this isn't limited to running x86 on arm, it would also allow you to run an x86_64 bazel on say, MIPS, or an arm bazel on x86_64 using binfmt_misc.
The underlying issue is that when you're using binfmt_misc, /proc/self/exe
is qemu-user-static and not the bazel binary. getauxval(3) is a different api that provides similar information, but it's user-space memory that qemu-user-static can properly set up prior to handing off control to the binary (bazel in this case). arg[0] is another API that can be used which qemu-user-static will patch, but it's liable to contain links or path traversal such that resolving it is more work for us (../../bazel
, etc).
@emidln-imc Sorry, I've never heard of qemu-user-static before. Am I correct that you would still be asking for this PR even if commit d79ec03 had never happened, because target of symlink |
@haxorz correct, we'd have to do it with or without that patch because /proc/self/exe simply isn't the binary being executed by the kernel. A deeper explanation (with links to code) follows. Apologies to any glibc, kernel, or qemu hackers reading this.
The usual way, a native x86_64 linux elf binary on x86_64 linux is set up using https://github.com/torvalds/linux/blob/master/fs/binfmt_elf.c#L172 (which is accessed by getauxval(3) from glibc). All of this comes home if you examine the implementation of If we want to keep the original behavior, it seems reasonable to do one of two things: (1) provide a flag or environment variable that can use getauxval(AT_EXECFN) for users who need it or (2) change the way we call into ijar such that we can catch the failure (either through exceptions/errors and fallback to getauxval(AT_EXECFN) if we can't load via /proc/self/exe. My approach was chosen to minimize the diff. Alternate solutions that might work but are hacks would be to try to recognize when /proc/self/exe is qemu and taking the correct path. I didn't like this, and I couldn't find a consistent way to detect running under binfmt_misc, since the entries in proc, to my knowledge, only convey that binfmt_misc is possible, not that the current process is running using it. I don't think the current loader is exposed to the user, but maybe a more experienced kernel dev would know. |
@emidln-imc Thank you very much for the explanation! The true background of d79ec03 is that we were working around a Kernel bug (possibly just in Google's internal Kernel; idk) with FUSE paths. Sorry for not giving that background in the commit description or code comment! I don't want to undo d79ec03. I do acknowledge our situation is a bit weird, so as a compromise I'd be willing to make that change internal to Google's fork of Bazel, and then letting you proceed with this PR as-is. On the other hand, perhaps some Bazel users actually benefit from that change (perhaps they are putting the Bazel binary in a FUSE filesystem). Similarly, I think the qemu-user-static situation is weird. Therefore I think the best solution is one where we don't prioritize one weird situation over the other. On that note...
Yes, that's acceptable.
I had a similar idea that I think might be achievable: What if the code looked at both the target of The extra system calls entailed by this approach would be acceptable to me. And then we don't have to worry about adding an env var / flag knob. What do you think? |
Do you know if |
Hello! Do we have a timeline on when this can be merged or not? This currently has been a pain point for me as well and it looks like it's the correct fix! |
There were a couple questions and comments around this PR:
Do you think you could address those? |
This seems to be a blocker for me; possibly a lot of people on M1 Macs, perhaps someone from the core team could adopt it? |
When using qemu-user-static + binfmt_misc on Linux (e.g. running docker run --platform linux/amd64 on ARM), bazel fails to self-extract with a mysterious lseek failure. When self-extracting using "/proc/self/exe", the referred binary is the qemu-user-static emulator, not the bazel process. Instead, we use an alternative API, getauxval(3), which is properly populated when running normally on the native host platform as well as when using the qemu + binfmt_misc pattern. Practically, this allows x86_64 versions of bazel to self-extract and run under Docker hosted by Linux ARM or M1 Macs.
30ec5ce
to
e4fddd9
Compare
I added this functionality gated behind a new optional startup flag. The original functionality is still the default, but there is now a way to hint to get bazel to use getauxval. Please give me a better name for it, and please let me know if it needs "experimental" or something like that.
I'd didn't fully resolve this path. From my testing it seems like |
While figuring out the bootstrap ordering to get a flag into |
Does that resolve to the same path when running under qemu or regular Bazel? If that's the case for qemu, maybe then we just need a flag whether to Personally, I have nothing against using |
I'd like to test this change on M1, but I'm not usually a docker user. Could you post a step-by-step repro? Thank you! |
After checking, realpath("/proc/self/exe") resolves to the absolute path of the bazel binary under qemu-user-static. This would save us a weird getauxval call. I can update the PR to just call realpath if the flag is set. What name should the flag be? |
@@ -1623,6 +1616,12 @@ int Main(int argc, const char *const *argv, WorkspaceLayout *workspace_layout, | |||
ParseOptionsOrDie(cwd, workspace, *option_processor, argc, argv); | |||
StartupOptions *startup_options = option_processor->GetParsedStartupOptions(); | |||
startup_options->MaybeLogStartupOptionWarnings(); | |||
const string self_path = GetSelfPath(argv[0], *startup_options); | |||
|
|||
if (argc == 2 && strcmp(argv[1], "--version") == 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.
Unfortunately moving this check here breaks bazel --version
, and also unfortunately the test for this only present internally. The complication is --version
is a magic flag that isn't understood by the options parser. This is by design so that we traverse as little code (which could fail) as possible, including rc parsing.
I'm not sure what the best way to work around this would be... I think we'd either need to rethink how --version
is implemented wrt the options parser or see if there's another way to toggle or fall back to the feature you're proposing.
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.
I'm not sure what the best way to work around this would be...
If we're running into issues like this I'd recommend abandoning trying to use a startup flag. Instead, either:
- Use an environment variable, as proposed by @emidln-imc
in Add optional startup --linux_bazel_path_from_getauxval #14391 (comment) - Have the client dynamically determine what to do, as I proposed in Add optional startup --linux_bazel_path_from_getauxval #14391 (comment)
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.
+1 to dynamically detecting that if possible. One problem here potentially is that from what I was told, realpath("/proc/self/exe")
points to Bazel, in which case I don't know how to distinguish that from realpath(getauxval(AT_EXECFN))
. Does readlink
offer a way to distinguish those 2?
That's great! I would say something like
That would work nicely. Anyway, it looks like we may need to hijack that from Google side to better facilitate the split between internal/external. Is there a Bazel issue for that? If no, would you mind filing one, ideally with pasted error you ran into? |
I filed #15076 for this, could you share the short repro for that issue I can include in there? Also an error message would be great so others can easily find that. |
I pasted that message to #15076 description, hope you don't mind that. |
+1 to this request. Not having a repro makes it very hard for us to make best recommendation for the fix. If it is impossible for you to share one could you at least share what do we get in Bazel from following syscalls under qemu (like add printfs and copy-paste):
That is the list of the first experiments I would want to run if I had a chance to. I know that you already tried some of them, but providing that info one place could be good for posterity. Thank you in advance! |
I don't know how helpful this is, since it's not a minimal repro scenario, but I'm running into this issue on an M1 when following the TensorFlow instructions to build from source within a Docker container. I follow those instructions, but include a
However, any calls to
I've also tried (as suggested here), downloading a newer version of bazel, specific to this platform, and copying it to
|
is there any plan to merge this? |
I'm also curious when this will be merged. We have some developers receiving new M1-based MacBooks and finding that they can't run Bazel in our developer environment because it's x86-based and runs on Docker. Just today I built these changes into the version of Bazel we use and tested it on one of the new Macs. It worked great. I noticed that the new startup option doesn't appear in the help ( |
Thanks for creating this PR! The key cli commands to run on Linux x86 or in some Docker x86(!) container environment
One might need to install gcc, g++, openjdk-8-jdk, python3, unzip, patch packages, but the crucial steps are listed above. |
There are still some questions about the effectiveness of doing this with a flag (and what second order effects that has) vs figuring out how to do it in a way that "just works". Unfortunately the folks on this bug so far don't have the facilities to repro or experiment with that, so passing off to a teammate who's in a better position to do so. Should have more feedback next week. |
I can now reproduce this issue with the tensorflow docker container on a M1 Macbook Pro:
|
I'm building a binary at this branch to verify if it fixes the issue: https://github.com/bazelbuild/bazel/tree/test_14391 |
|
OK, it worked with a rerun. |
@michajlo Is there anything else you want me to test? I can confirm the flag solves the extracting problem, I'm able to start building TensorFlow inside a linux docker container on my M1 machine with a Bazel binary built at this PR. |
|
Oh, yeah,
Putting the flag |
I can also confirm this:
I'm running only the client, but it should be the same. Both readlink and realpath resolve to the absolute path for the binary. So maybe no need to call getauxval? |
Please do merge this fix.. ...or recommend a |
In Blaze, we keep using "/proc/self/exe", but in Bazel we resolve this symlink to the actual Bazel binary. This is essentially a rollback of d79ec03 only for Bazel, because it solved an issue that rare in Bazel, but it's preventing users from running Bazel inside a Linux docker container on Apple Silicon machines. Fixes #15076 Closes #14391 RELNOTES: None PiperOrigin-RevId: 441177762 Change-Id: I48d7283cf7f42f4220e2261f02809c8b5270ef70
Sorry for the silence here, after some internal discussion, we decided to fix this by rolling back d79ec03, but only in Bazel. I'm implementing the change at 417e116, and have manually verified it works. So this PR is not necessary. If you want to double check, you can do
After that change is merged, we'll cherry pick the fix for the next Bazel release. |
In Blaze, we keep using "/proc/self/exe", but in Bazel we resolve this symlink to the actual Bazel binary. This is essentially a rollback of bazelbuild@d79ec03 only for Bazel, because it solved an issue that rare in Bazel, but it's preventing users from running Bazel inside a Linux docker container on Apple Silicon machines. Fixes bazelbuild#15076 Closes bazelbuild#14391 RELNOTES: None PiperOrigin-RevId: 441198110
In Blaze, we keep using "/proc/self/exe", but in Bazel we resolve this symlink to the actual Bazel binary. This is essentially a rollback of d79ec03 only for Bazel, because it solved an issue that rare in Bazel, but it's preventing users from running Bazel inside a Linux docker container on Apple Silicon machines. Fixes #15076 Closes #14391 RELNOTES: None PiperOrigin-RevId: 441198110
@zmk-punchbowl did you solve the problem? how did you manage it? |
When using qemu-user-static + binfmt_misc on Linux (e.g.
running
docker run --platform linux/amd64
on ARM), bazelfails to self-extract with a mysterious lseek failure. When
self-extracting using "/proc/self/exe", the referred binary
is the qemu-user-static emulator, not the bazel process. Instead,
we use an alternative API, getauxval(3), which is properly
populated when running normally on the native host platform
as well as when using the qemu + binfmt_misc pattern.
Practically, this allows x86_64 versions of bazel to
self-extract and run under Docker hosted by Linux ARM or M1 Macs.