Persistent worker without binary dependency #517
Conversation
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
RBE is likely failing because use_worker is enabled in examples_deps.bzl - presumably any worker testing code needs to be gated to not run on RBE. The Windows issue may be due to a locked file - some brief Googling seems to throw up Windows Defender or a flag change as possible culprits, and I notice --enable_runfiles changed on the second run, a bit like |
|
@dae |
|
The get_linker_env part of 119c478 is supposed to be handling this - it should be extracting the MSVC path and adding it at the top of the path, so that it's found before the msys tools are. It seems to work on two machines here; perhaps adding print(path) before the if path: line will reveal something? Maybe your MSVC path is not at the top of the path as the code is assuming? You can try it out locally by checking out this repo/branch into c:\rules_rust and then uncommenting this section: https://github.com/ankitects/anki/blob/c505894b88221e6d31ed1b7fdb3a5d835c4de83e/repos.bzl#L41 (if you don't want to debug this, just set use_worker=False in defs.bzl for now) |
Exactly. The top of the path contains the IntelliCode path ( |
|
I've been using this for a few days on Linux and it's working well but I have a few bits of feedback:
|
…non-persistent case
Repositories should always be registered.
Hmm... this is relying on Rust's incremental compilation model to handle correctness. Do you have a specific crate and configuration that someone else can try? You could try some flags from https://docs.bazel.build/versions/master/persistent-workers.html#modifying-persistent-workers, like |
|
I fixed my immediate issue but I’m not certain what did it. I’m fairly sure it was purging the /temp/rustc-worker* directories and doing a bazel shutdown. I don’t have exact repro steps, I’ll try to figure out a consistent trigger, but I think it was a combination of removing deps (so they were no longer passed as linker params) and switching git branches (I know this is a problem for cargo because it uses timestamps for change tracking, I don’t know if rustc does the same). |
|
I've updated the docs to mention this code is experimental, and added a warning about possibly needing to purge the files in /tmp. Despite the possible correctness issues it may harbor, I still think this might be a nice thing to have available as an experimental/subject-to-change-or-removal feature - it's opt-in, so should not affect users unless they explicitly enable it, and it can make the edit/compile/test cycle during development somewhat nicer - it brings a 15s compile down to about 6s here. |
|
@djmarcin would be interested to hear if worker_sandboxing makes any difference - since it seems to only alter which input files are available, I wonder if that would help or not. Was your error similar to this one? rust-lang/rust#59535 |
It does look like it may be similar to that one. I'll try updating to the latest nightly. --edit: Confirmed this was caused by bugs in rustc itself |
|
One thought - maybe instead of placing the rustc-worker files in $TEMP, it might make more sense for the config option to define the path they should be stored, similar to how Bazel's --disk_cache and --repository_cache work - that would make it more obvious that files are being persisted across runs, and make it easier to clean them up as required. |
|
Just an update: I've been running this for the last month and have not run into any issues with fastbuilds, but have seen ICEs and other issues like djmarcin described when doing optimized builds after bumping dependencies, and the last time I hit this, purging the worker folder did not solve the problem - I suspect a bazel clean would have been required. Since incremental compilation is something I only want during development, I've just got my local repo set up to only use the worker in development builds, and that has been running well. |
|
I ran into an ICE again while running in fastbuild mode, because a build script was being compiled with optimizations. I use --disk-cache with Bazel, and found I had to purge it to fix the issue - removing the rustc-worker folders and using 'bazel clean -expunge' did not help. |
I think this would happen with any ICE though, because bazel will cache the failure for those inputs, so I don't think it's a blocker to merging this PR. We've been using this since December and it has been very stable for us. |
|
We recently switched our toolchain to use This seems like some kind of stale object file issue, and we are already using
The reason this seems worker related to me is that repeated invocations of bazel build continue to fail, but if I |
|
For the ICE in the compiler, it may be worth finding a reduced test case that fails outside your infrastructure and filing it as a rustc bug. As for having over 400 processes, it is possible there is some bug where the worker is not quitting, or perhaps Bazel determines your machines have enough capacity to run these processes? I'm not sure what heuristics Bazel uses. |
|
Minor changes were required to get this going with the latest rules_rust - they're available here (see parents) if anyone needs them. |
|
I ran into more linking issues, this time in debug build, and it motivated me to dig into this a bit more. Since rustc doesn't seem to be properly splitting the output based on things like architecture, I've changed the worker code to generate a hash for each request, and include things like target architecture in the hash, so we no longer need to rely on rustc doing the correct thing. Hopefully that will solve the ICEs. I've also added an experimental option to limit the incremental output to specific crates with a regex, so that disk space is not used up on infrequently changing third-party crates. |
This is considerably less invasive than bazelbuild#667, bazelbuild#517 and bazelbuild#421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on bazelbuild#667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. It also requires targets to be tagged "incremental" - any without the tag will be compiled normally, even if a base folder is provided. Cargo's behaviour appears to be to only store incremental products for the product being built (eg, all the dependencies you pull in from crates.io do not have incremental build products stored), so manual tagging seems preferable to an "include everything" approach.
This is considerably less invasive than bazelbuild#667, bazelbuild#517 and bazelbuild#421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on bazelbuild#667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. It also requires targets to be tagged "incremental" - any without the tag will be compiled normally, even if a base folder is provided. Cargo's behaviour appears to be to only store incremental products for the product being built (eg, all the dependencies you pull in from crates.io do not have incremental build products stored), so manual tagging seems preferable to an "include everything" approach.
This is considerably less invasive than bazelbuild#667, bazelbuild#517 and bazelbuild#421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on bazelbuild#667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. It also requires targets to be tagged "incremental" - any without the tag will be compiled normally, even if a base folder is provided. Cargo's behaviour appears to be to only store incremental products for the product being built (eg, all the dependencies you pull in from crates.io do not have incremental build products stored), so manual tagging seems preferable to an "include everything" approach.
This is considerably less invasive than bazelbuild#667, bazelbuild#517 and bazelbuild#421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on bazelbuild#667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. The default behaviour will incrementally compile crates in the local workspace, like cargo does. The behaviour can be adjusted with another flag, which is covered in docs/index.md.
This is considerably less invasive than bazelbuild#667, bazelbuild#517 and bazelbuild#421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on bazelbuild#667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. The default behaviour will incrementally compile crates in the local workspace, like cargo does. The behaviour can be adjusted with another flag, which is covered in docs/index.md.
This is considerably less invasive than bazelbuild#667, bazelbuild#517 and bazelbuild#421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on bazelbuild#667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. The default behaviour will incrementally compile crates in the local workspace, like cargo does. The behaviour can be adjusted with another flag, which is covered in docs/index.md.
This is considerably less invasive than bazelbuild#667, bazelbuild#517 and bazelbuild#421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on bazelbuild#667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. The default behaviour will incrementally compile crates in the local workspace, like cargo does. The behaviour can be adjusted with another flag, which is covered in docs/index.md.
This is considerably less invasive than bazelbuild#667, bazelbuild#517 and bazelbuild#421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on bazelbuild#667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. The default behaviour will incrementally compile crates in the local workspace, like cargo does. The behaviour can be adjusted with another flag, which is covered in docs/index.md.
This is considerably less invasive than bazelbuild#667, bazelbuild#517 and bazelbuild#421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on bazelbuild#667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. The default behaviour will incrementally compile crates in the local workspace, like cargo does. The behaviour can be adjusted with another flag, which is covered in docs/index.md.
This is an experimental change to #421 that uses cargo to build the worker, instead of relying on an external binary. It is working for me on a Linux, Mac and Windows system here, but I have only given it cursory testing so far.
It probably is not suitable for merging as-is due to the hacky way I've added Windows support, but people might like to have a play with it and see how it works in practice.