-
Notifications
You must be signed in to change notification settings - Fork 533
Fix processwrapper in multiplatform builds #3790
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
base: main
Are you sure you want to change the base?
Conversation
44890a7 to
8de46ac
Compare
8de46ac to
4c2e5f8
Compare
UebelAndre
left a 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.
I haven't fully understood the implementation here but I feel like cfg = "exec" should be whatever the configuration is for the platform running an action. If this is not the case I feel this should be addressed in Bazel directly and not something rules have to account for. Is there an issue for this somewhere already?
It looks like this may not be necessary for correctness in rules_rust today as we are emitting a single action, but as soon as we have cases where a rule does multiple actions, it will be broken because there's nothing forcing the tool into the same exec as the toolchain. (This is what exec groups do, so fixing it that way could be another fix). However, I do think it's nicer to have this binary (which is conceptually part of the toolchain needed to run rustc_compile) be on the toolchain itself. That means third-party rules that need to rust rustc_compile can just use the toolchain without needing to add an extra magic attribute; i.e. the toolchain is encapsulated better. cc @fmeum in case you have additional thoughts. |
This behavior wouldn't necessarily be correct: An action could be executed on Windows but run a Linux tool (configured for a special Linux exec platform) in a container. It also wouldn't be possible to implement this since the action is considered by arbitrary Starlark in the rule impl, but the configuration of the deps has to be known before that's executed. @dzbarsky An alternative would be a dedicated toolchain that's resolved together with rustc. |
True, do you think that's preferable though? I feel like that would also leak this implementation detail to consumers (they'd need to depend on an extra toolchain when conceptually they just want a rust compiler). By consumer I mean rulesets that want to invoke rust compilation such as https://github.com/google/crubit etc |
|
Right, the toolchain would have to be a toolchain requirement of the rust toolchain target so that it's picked up automatically. But that's probably just more complex than what you are doing in this PR. |
The problem is that
cfg = "exec"is not well-defined when you have multiple exec platforms. We want to build the wrapper for the same platform that the toolchain will be used on, and the way to achieve that is by making it an attribute of the toolchain itself.Previously, building on a linux remote executor from a windows host was broken.