Add process wrapper #257
Add process wrapper #257
Conversation
jelmansouri
requested review from
aiuto,
c-parsons,
jin and
laurentlb
as code owners
|
Could you start with a design discussion somewhere? I think I agree something like this is worthwhile because bash scripts have no place in a portable world, but there are some problems
|
|
I created a google doc to discuss this! |
|
+bazel-discuss <bazel-discuss@googlegroups.com>
…On Wed, Jul 8, 2020 at 11:09 PM Jin ***@***.***> wrote:
cc @meteorcloudy <https://github.com/meteorcloudy>
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#257 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXHHHAXZAUW5LIB4MWKOWTR2UYIDANCNFSM4OU4MPTA>
.
|
|
Given that Bazel already has a process-wrapper tool, should this be called something else? |
I don't have any strong opinion about the name :) if you have a suggestion I would be happy to change it! |
|
Updated the design doc with more information! |
There was a problem hiding this comment.
Back to my comment about the name... but taking a step further: given that Bazel's process-wrapper already does a lot of what this does... maybe that one could be extended and reused in some way instead? Seems weird to have two very similar tools. (https://jmmv.dev/2019/11/bazel-process-wrapper.html)
Also, I'm wondering... all this argument rewriting... why cannot it be done from Starlark? I think the design doc should better explain why a separate tool is necessary. A separate tool feels a bit awkward to me, and it risks becoming its own "shell interpreter" over time.
| return -1; | ||
| } | ||
|
|
||
| pid_t child_pid = fork(); |
There was a problem hiding this comment.
Why do we need to fork at all? From what I see in the description of this wrapper, there is no need to do anything upon process termination, so the wrapper can set things up and then give control to the wrapper process, keeping the process tree cleaner (and being much faster e.g. on macOS).
There was a problem hiding this comment.
The touch file feature, needs getting back the control.
Ideally it would, but looking at some open issues dating back to 2018 like this one bazelbuild/bazel#5511, it might be harder to make these changes to support remote executions platfoms. But I'm not an expert here! |
As we are working toward having native rule compatibility with windows, we encountered a lot of run_shell usage to have access to some basic shell functionality like substituting $pwd in command line arguments or environment variables or redirecting the output. Or reading a file that contains arguments generated by another rule (build.rs scripts in rust).