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.

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

process_wrapper: replace C++ implementation with rust. #1159

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

gigaroby
Copy link
Contributor

The main advantages of this work are that it eliminates a bunch of
platform-specific C++ code in favor of the Rust standard library
(mainly std::process::Command and std::fs::File) and it simplifies
future improvements in the area of pipelining and incremental
compilation. Building a Rust process_wrapper requires a way to use
rust_binary without support from process_wrapper itself. This is
achieved through a transition in //util/process_wrapper/transitions.bzl.

@gigaroby gigaroby force-pushed the process-wrapper-rust branch 6 times, most recently from a010585 to 137f474 Compare February 28, 2022 18:54
@hlopko hlopko self-requested a review February 28, 2022 19:02
@gigaroby gigaroby force-pushed the process-wrapper-rust branch 10 times, most recently from e76f5c4 to 8065ee5 Compare March 1, 2022 10:45
@gigaroby gigaroby force-pushed the process-wrapper-rust branch 2 times, most recently from e3763f5 to 0dea049 Compare March 1, 2022 13:31
util/process_wrapper/transitions.bzl Outdated Show resolved Hide resolved
util/process_wrapper/transitions.bzl Outdated Show resolved Hide resolved
util/process_wrapper/BUILD.bazel Outdated Show resolved Hide resolved
util/process_wrapper/BUILD.bazel Outdated Show resolved Hide resolved
util/process_wrapper/BUILD.bazel Show resolved Hide resolved
util/process_wrapper/util.rs Outdated Show resolved Hide resolved
util/process_wrapper/util.rs Outdated Show resolved Hide resolved
util/process_wrapper/flags.rs Outdated Show resolved Hide resolved
util/process_wrapper/flags.rs Show resolved Hide resolved
util/process_wrapper/main.rs Outdated Show resolved Hide resolved
@gigaroby gigaroby force-pushed the process-wrapper-rust branch 9 times, most recently from e02228f to 8bce446 Compare March 4, 2022 18:13
@gigaroby gigaroby force-pushed the process-wrapper-rust branch 3 times, most recently from 6fe93df to d626996 Compare March 7, 2022 09:46
rust/private/transitions.bzl Outdated Show resolved Hide resolved
util/process_wrapper/BUILD.bazel Show resolved Hide resolved
util/process_wrapper/util.rs Outdated Show resolved Hide resolved
util/process_wrapper/util.rs Show resolved Hide resolved
util/process_wrapper/main.rs Outdated Show resolved Hide resolved
util/process_wrapper/flags.rs Outdated Show resolved Hide resolved
util/process_wrapper/flags.rs Outdated Show resolved Hide resolved
util/process_wrapper/flags.rs Outdated Show resolved Hide resolved
util/process_wrapper/flags.rs Outdated Show resolved Hide resolved
util/process_wrapper/flags.rs Outdated Show resolved Hide resolved
util/process_wrapper/flags.rs Outdated Show resolved Hide resolved
@gigaroby gigaroby force-pushed the process-wrapper-rust branch 4 times, most recently from 968201f to 254e1fb Compare March 8, 2022 13:18
Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

What a journey!!! Thank you!

The main advantages of this work are that it eliminates a bunch of
platform-specific C++ code in favor of the Rust standard library
(mainly std::process::Command and std::fs::File) and it simplifies
future improvements in the area of pipelining and incremental
compilation. Building a Rust process_wrapper requires a way to use
rust_binary without support from process_wrapper itself. This is
achieved through a transition in //util/process_wrapper/transitions.bzl.
@UebelAndre
Copy link
Collaborator

This change may have introduced a regression. d9ba374#commitcomment-68356750

@gigaroby @hlopko would either of you be able to take a look?

@ddeville
Copy link
Contributor

fwiw I've been trying out the PR on Windows and I'm not having issues.

korDen added a commit to korDen/rules_rust that referenced this pull request Mar 10, 2022
@dfreese
Copy link
Collaborator

dfreese commented Mar 11, 2022

Bisected to this commit, when trying to upgrade rules_rust. This triggered the following error for me:

    //proto/protobuf:protobuf_prost_generated (782077f4a782b271e6081e5d253d233307fbb065ebd24cbeda52275882be7e2c)
    //tools/prostgen:prostgen (ad7becf1f627c98f4c44ac49f5991ab41aa98662628ab4c26b029d7fb1035392)
.-> @rules_rust//util/process_wrapper:process_wrapper (ad7becf1f627c98f4c44ac49f5991ab41aa98662628ab4c26b029d7fb1035392)
|   @rules_rust//util/process_wrapper:process_wrapper_impl (ad7becf1f627c98f4c44ac49f5991ab41aa98662628ab4c26b029d7fb1035392)
|   @rules_rust//util/process_wrapper:process_wrapper_bin (ad7becf1f627c98f4c44ac49f5991ab41aa98662628ab4c26b029d7fb1035392)
`-- @rules_rust//util/process_wrapper:process_wrapper (ad7becf1f627c98f4c44ac49f5991ab41aa98662628ab4c26b029d7fb1035392)

Where the protobuf:protobuf_prost_generated target depends on the prostgen tool. This was in the host configuration. If I switched that to the exec configuration, the error went away, but it does seem like there's potentially a problem for rules that expected to be run on host.

@hlopko
Copy link
Member

hlopko commented Mar 21, 2022

@dfreese - FYI from https://docs.bazel.build/versions/main/skylark/rules.html#configurations:

Historically, Bazel didn’t have the concept of execution platforms, and instead all build actions were considered to run on the host machine. Because of this, there is a single “host” configuration, and a “host” transition that can be used to build a dependency in the host configuration. Many rules still use the “host” transition for their tools, but this is currently deprecated and being migrated to use “exec” transitions where possible.

I think it's reasonable to proactively swift from using host to exec. Resiliency on transitions is one good reason (host doesn't work well with transitions - you can't transition from host).

@UebelAndre
Copy link
Collaborator

I think I found another related issue: #1209

(It's kinda cool though to see a panic 😄 Yay Rust!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants