Switch pipelining metadata action to hollow rlib (-Zno-codegen)#3870
Switch pipelining metadata action to hollow rlib (-Zno-codegen)#3870walter-zeromatter wants to merge 1 commit intobazelbuild:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hey @walter-zeromatter, we'll need a signed CLA to proceed with the review, could you let us know if that works for you? |
|
It's Walter's work. We just merged it to my fork a while ago so the world can benefit from it via rules_rs. |
|
Thanks for the clarification! In that case we only need to wait for the CLA. |
|
Sorry, I've been waiting on our legal. Just got approval, should have the cla signed later today |
|
@scentini Sorry about the delay, should be all good now. |
349e91c to
841c962
Compare
|
Apologies - This code was written some time ago while I was doing some early experimenting with LLMs, and while I do think replacing the -emit=metadata,link + kill rustc approach with -Zno-codegen, A number of the claimed benefits here simply aren't real - I've since learned better, and will be overhauling this PR to something more appropriate. Please hold. |
841c962 to
b55be1b
Compare
|
@scentini This is now a proper fix. Sorry for prior confusion. I do think this is the right approach, and I'll be making a few follow-up PRs to adopt a few other improvements from buck2's rust rules including lightweight metadata for check passes & hopefully some kind of lint check people can run to identify if they have non-deterministic proc macro dependencies. |
e470d22 to
63ca376
Compare
| out_dir = out_dir, | ||
| build_env_files = build_env_files, | ||
| build_flags_files = build_flags_files, | ||
| emit = ["dep-info", "metadata"], |
There was a problem hiding this comment.
Why are we removing dep-info?
There was a problem hiding this comment.
Mostly because it's not necessary - Nothing in rules_rust uses the outputs
| disable_pipelining = getattr(ctx.attr, "disable_pipelining", False), | ||
| ): | ||
| rust_metadata = ctx.actions.declare_file( | ||
| paths.replace_extension(rust_lib_name, ".rmeta"), |
There was a problem hiding this comment.
If the .rlib and _meta.rlib files are in the same directory, how will the compiler know which one to pick when it encounters a -Ldependency=blaze-bin/rlib/containing/directory/? I think this won't necessarily be an issue with sandboxed and remote execution, which may only have the _meta.rlib file present, but in local non-sandboxed builds I can see how this can pose a problem.
There was a problem hiding this comment.
Yeah, you're right. I ran into this while playing with an experimental mode to allow incremental builds with local execution & I've pushed a fix.
| # RUSTC_BOOTSTRAP=1 is required for -Zno-codegen on stable rustc, and must | ||
| # be set on both the metadata and full actions for SVH compatibility (since | ||
| # RUSTC_BOOTSTRAP affects the crate hash). | ||
| env["RUSTC_BOOTSTRAP"] = "1" |
There was a problem hiding this comment.
This change makes pipelining work only for users that are fine with relying on unstable features. I think that's fine, however I think we should go through the incompatible change process, where we add an --incompatible_use_unstable_no_codegen_for_pipelining build_setting, and gate the changes in this PR behind it.
| experimental_use_cc_common_link = experimental_use_cc_common_link, | ||
| ) | ||
|
|
||
| compile_inputs_metadata = compile_inputs |
There was a problem hiding this comment.
Seems like compile_inputs_metadata and compile_inputs are now the same thing. If so, could we get rid of this variable?
63ca376 to
ee6c5e6
Compare
|
I've updated to use a gating incompatible flag, and also added machinery so that this doesn't automatically enable all unstable features for downstream users - if you pass RUSTC_BOOTSTRAP or manually specify -Zallow-features, that behavior should still work |
ee6c5e6 to
e78e7de
Compare
Introduce `incompatible_use_unstable_no_codegen_for_pipelining` to switch pipelined compilation from the process-wrapper `--rustc-quit-on-rmeta` approach to Buck2-style `-Zno-codegen` hollow rlibs. Defaults to off; the existing rmeta-interception path remains the default behavior. When enabled: - Metadata action runs `rustc -Zno-codegen --emit=link=<path>` to produce a hollow rlib (`_meta.rlib` in a `_meta/` subdir of the full rlib). - Full action emits only `--emit=link` (no metadata, no dep-info). - `RUSTC_BOOTSTRAP=1` is set on both actions so SVH matches. - Metadata-consuming actions get an extra `-Ldependency` pointing at the `_meta/` subdir for transitive resolution. Tracked in bazelbuild#3977.
e78e7de to
67f7aaa
Compare
Replace the --rustc-quit-on-rmeta / .rmeta approach with Buck2-style hollow rlibs: the RustcMetadata action runs rustc to completion with -Zno-codegen, emitting a .rlib archive. This approach mirrors the one used by buck2 and avoids needing to kill rustc mid-output in order to produce metadata.
While not fixing problems with SVH mismatches when non-determinism, this does simplify the codepath and uses a production tested technique that doesn't have any of the dangers associated with killing the rustc process while it's still active.
Previous work on this branch & the claimed prevention of SVH mismatches proved to be from changes which effectively removed all the actual benefits of the pipelined compilation