Apply genrule-setup.sh to Starlark shell actions

[brandjon]

This is a proposal to have the custom initialization hook for genrules also apply to Starlark shell actions.

Initial PR for draft.

[brandjon]

Comments copied from pre-markdown version:

• laurentlb@: What about changing the shell?
  We keep the command "shell -c ‘<cmd>’", but shell is free to execute its argument the way it wants?
• brandjon@: Then we'd need a wrapper script around the shell -- an alternative shell toolchain, no? Not sure what the benefit is.
• laurentlb@: This means that Bazel doesn't need to know/care about it.

Also:

• laurentlb@: This assumes that the shell knows about "source". I'm not sure what happens on Windows, but it's better to reduce the number of assumptions about the shell behavior.

[aiuto]

Can you take the bit of functionality you are working towards and make it apply in a sh-less environment on Windows? Perhaps failfast is a setting on the genrule (with a default in the global workspace config). The executor then can add the right flags for bash or cmd.exe.

[brandjon]

This assumes that the shell knows about "source". I'm not sure what happens on Windows, but it's better to reduce the number of assumptions about the shell behavior.

It seems that we do assume the shell is Bash or Bash-compatible, so hopefully set/source will work even on Windows.

What about changing the shell?

If we make the existing shell strict, that'll affect all genrules and Starlark shell actions (and maybe other things too). If we add an alternative shell tool, and allow genrules and Starlark shell actions to choose the tool via an attribute/param, then we still are making Bazel know about shell strictness, so we may as well implement it using a single underlying tool. If we do an incompatible change to make all Starlark shell actions use the strict tool, then maybe you can argue it's cleaner to not have SkylarkActionFactory know about set/source, but I think you pay the extra cost of having a wrapper script.

make it apply in a sh-less environment on Windows

Is Windows now, or will it be, sh-less? If so, what are genrule and run_shell() going to do?

[brandjon]

Separate from how we factor strictness between run_shell() and the toolchain, there's the issue of whether or not expanding the hook for customizing shell initialization is the Right Thing To Do. Maybe we don't want to let repo owners declare arbitrary custom semantics for run_shell() -- i.e., maybe genrules-setup.sh was a mistake.

We could still get sandboxing support for remote execution platforms without genrule-setup.sh, by having those platforms provide their own shell toolchain, which would be selected in place of the default.

[laurentlb]

cc @laszlocsomor @meteorcloudy

[laurentlb]

also, cc @philwo

[meteorcloudy]

The plan on Windows is to make Bazel sh-less. For genrule, people can choose to use bash, cmd or even powershell. How complicated is genrule-setup.sh going to be? Maybe we can later implement a genrule-setup.bat for cmd.exe and genrule-setup.ps1 for powershell?

Currently, genrule and run_shell on Windows still require bash, so genrule-steup.sh should also work.

[brandjon]

For genrule, people can choose to use bash, cmd or even powershell.

I suppose we'll have to do the same for run_shell(), but at that point, wouldn't it just be run()?

How complicated is genrule-setup.sh going to be?

Currently it just makes shells strict via set -euo pipefail, but it could also be used for custom sandboxing logic on remote execution platforms.

But we may be able to get away with not using this file at all. For strictness, as an alternative to sourcing the file, we can instead bake the set command into either the shell toolchain (possibly at the cost of a wrapper process) or the run_shell() implementation (which adds shell assumptions to Bazel, just like genrule currently has). For sandboxing, we can bake whatever setup logic is required into a separate shell toolchain, and use the execution platform to select the sandboxing toolchain over the standard one.

[laurentlb]

If we want to make Bazel sh-less on Windows, I question the value of run_shell(). It seems to add some convenience at the expense of the portability.

Is it possible to write a function like run_shell() in a bzl file (using actions.run())? If so, it might be simpler to do so and keep Starlark shell-agnostic.

[brandjon]

That seems appropriate and necessary if shell as a builtin concept is disappearing. +@tomlu for thoughts on reducing run_shell() to run().

[laszlocsomor]

Adding to @laurentlb 's comment:

If we want to make Bazel sh-less on Windows, I question the value of run_shell(). It seems to add some convenience at the expense of the portability.

run_shell()'s other hindrance to portability is its inability to declare what shell (Bash, Zsh, Sh, cmd.exe, PowerShell) and toolset it requires. See #5265

[brandjon]

I don't think I'm going to pursue this at the moment, but the doc's there if someone else wants to pick it up. I think the main issues are:

1. Ensuring that run_shell() does, or can be made to, use an alternative shell toolchain that can inject sandboxing setup for remote execution platforms

2. Deciding whether to do set -euo pipefail for run_shell() (is that our role?) and how it should be implemented (toolchain or hard-coded?)

3. Deciding how this interacts with the larger trajectory of shell support across multiple platforms and for run_shell() and genrule

[EdSchouten]

Out of curiosity, why does genrule-setup.sh need to be a separate script? Flags passed to set can also be provided as arguments to the shell.

bash -c 'set -e; set -u; set -o pipefail; ...'

Is the same as:

bash -e -u -o pipefail -c '...'

[brandjon]

The notion is that genrule-setup can apply workspace-wide common definitions, sandboxing, and (for builds with only one execution platform) execution environment setup. The counterargument is that these features do not belong in a workspace-scoped file that alters the semantics of all genrule / run_shell actions.

[alandonovan]

@brandjon What is the status of this?

[brandjon]

The status is that I've pretty much forgotten everything not written down in this thread or the design doc.

Since this issue clearly doesn't affect the core interpreter, we should probably Transfer it to the bazelbuild/bazel repository (and mark it P4).

[cpsauer]

Hey guys! Just ran into some tricky error cases with this that makes me think it might be worth something a little higher than a P4.

One can get some fairly weird behavior without -euo pipefail. Like consider the example given in the docs for run_shell. One could get a zero status code (and therefore a successful action, right?) even if the input file didn't exist or was unreadable, since without -o pipefail, we'd just be taking the status of the last part of the pipe.

Anyway, seems fairly worthwhile to add -euo pipefail for all the same reasons as with genrule.

[Edit/side note btw, also might be worth defaulting --shell_executable to bash to be resolved via the PATH, at least on macOS? System Bash is from '07, so most people replace it with something newer. I think that'd do more what the user expects in more cases.]