Stabilising experimental_shell_command and friends

[chrisjrn]

tl;dr: link to the current list of proposals under consideration

---

After a bunch of work, we're now in a position where the contents of shell_command.py are ready for their forever home in Pants.

This ticket describes the things that need to be done, along with initial proposals for what we do to accomplish these. These are subject to discussion.

Note that experimental_wrap_as_*, while written largely to support experimental_shell_command (esc) and experimental_run_in_sandbox (eris), are not subject to this stabilisation work, since there are known deficiencies with the current approach.

Moving and renaming experimental_run_in_sandbox

eris solves the technical problem of not being able to use build scripts written in Pants-supported languages as ad-hoc steps in Pants, along with the secondary goal of allowing 3rd-party tools to be integrated into a Pants installation in an ad-hoc fashion.

It also fulfils a philosophical goal of Pants: code quality is better if it's validated and tested. Currently, we have a great answer for production code, but not for code that is run in the build itself. With eris, these ad-hoc steps can be quality-checked by other Pants goals (test/lint/fmt/fix/check). Such validation is not possible with esc as it is currently designed.

We should encourage using code that can be quality-checked over code that cannot.

PROPOSAL: eris is documented as the preferred way to run user-supplied code as a build step, whenever it is possible, not esc. esc should be introduced only for cases where Pants does not provide any support. See future work for an important realisation on this topic.

PROPOSAL: Rename and move experimental_run_in_sandbox from the shell subsystem. The proposed new subsystem name is pants.backend.adhoc, and the target be named adhoc_tool.

I'm proposing the name adhoc over generic, because I feel it describes the use cases for these targets: one-off cases where a plugin does not exist, or would not be suitable. To me, generic, invokes the idea of re-use, which doesn't really apply here. On the other hand, adhoc invokes the idea of "this is a thing that is being done instead of a plugin". Happy to bikeshed this one.

Renaming experimental_shell_command

PROPOSAL: Rename experimental_shell_command to shell_command.

Once shell_source is runnable through eris, esc will only be the preferred option for trivial shell commands. At this point, shell_command is the obvious name for the thing that executes a line of shell script

PROPOSAL: Rename experimental_test_shell_command to shell_command_test

This is in line with nomenclature for other targets.

Removing experimental_run_shell_command

PROPOSAL: Delete experimental_run_shell_command (ersc), and make shell_command runnable.

Now that esc supports workdir, and tools is optional for esc, there is negligible difference between esc and ersc from an interface perspective. Making esc runnable would reduce the number of targets (and would be in line with the goal of making more targets runnable, for use in eris). It would also open up the possibility of esc being used as the runnable for eris, which is important (see Future work).

Future work

This list addresses possibilities that are enabled by following the above proposal.

Enable shell usage through eris

Note that the shell backend currently does not implement RunRequest on shell_source targets. However, if RunRequest were implemented, then shell_source targets could be executed using eris, which would make it possible to quality-check those build steps.

With this in mind, we should be encouraging eris(runnable=some_shell_source) as the preferred way to execute ad-hoc shell code in all but the most trivial of cases.

### Allow eris to run multiple runnables in sequence in the same sandbox

If shell_command becomes a runnable target (that could optionally be supplied to eris), then it would be possible to string together several runnable steps in the same sandbox if you needed to compose the results of multiple ad-hoc steps.

Consensus/decision making.

I'll leave this discussion open for 48 hours to allow for initial thoughts and to see if consensus emerges. If some alternate viewpoints emerge, I'll try to find consensus, and allow 24 hours of cooling off to see if we've actually met consensus. Ideally the contentious decisions policy won't come into play, but if we don't achieve consensus, then that's what it's there for.

[kaos]

I like the adhoc_tool target name.

[stuhood]

Moving and renaming experimental_run_in_sandbox

...
PROPOSAL: eris is documented as the preferred way to run user-supplied code as a build step, whenever it is possible, not esc. esc should be introduced only for cases where Pants does not provide any support. See future work for an important realisation on this topic.

PROPOSAL: Rename and move experimental_run_in_sandbox from the shell subsystem. The proposed new subsystem name is pants.backend.adhoc, and the target be named adhoc_tool.

As mentioned on the stabilization ticket, I remain concerned that experimental_run_in_sandbox is redundant with experimental_shell_command, and not composable.

I agree with the benefits that you point out, but those would still be achievable with any mechanism used to "run a runnable". Something similar to the syntax that I floated a while back (#17405 (comment)) still seems like it would allow for achieving that. Updating that for the current shape of the "run a runnable" syntax, that might instead look like environment variables which expand to a complete way to run the runnable:

experimental_shell_command(
  command="$RUNNABLE_NPM_THING && $RUNNABLE_PYTHON_THING",
  dependencies=[
    ":running-python-thing",
    ":running-npm-thing",
  ],
)

This would compose across multiple languages and tools, and would avoid having two very similar shaped target types.

[stuhood]

I am somewhat swayed the composability argument, but I'm still not sure it should be the preferred/only way to introduce running first/third party targets.

During an out-of-band discussion, @chrisjrn and I realized that composability is possible either with eris or runnables-in-esc, and found a better way to describe the difference in opinion:

• eris achieves composability by chaining a series of targets at the build graph level.
  • If you want to sequence two commands (including bash builtins like mv, ln, etc), you do so with two targets
  • Some "commands" in your graph might be Pants builtins like relocated_files
• runnables-in-esc achieves composability by inlining a series of commands into a single target, or via target dependencies.
  • As @danxmoran mentioned: you might compose two commands into one target if you wanted them to run in the same sandbox, and break them into two targets if you want different cache keys/sandboxes.

---

With that framing, the argument between the two seems to come down to the amount of boilerplate involved, and the learnability.

It is fairly safe to assume that our users are familiar with shell scripting (and that if they are not, we might actually be doing them a favor by requiring them to learn enough of shell scripting to run commands, move files, etc). Given that, the primary things that you need to learn with each approach seem to be:

• eris
  • the semantics of the experimental_run_in_sandbox target itself, including:
    • how inputs are produced/captured relative to your process
    • how stdio is captured, if at all
  • (potentially) relocated_files if you need different semantics than the workdir/output_root/etc
• runnables-in-esc
  • the semantics of the experimental_shell_command target itself, including:
    • how inputs are produced/captured relative to your process
    • how input runnables (if any) are presented

From a boilerplate perspective, composing multiple commands in a shell script inside one target will ~always be shorter, assuming more than one command is in use. It is slightly longer when running exactly one command.

[chrisjrn]

Note that I am arguing in favour of including both adhoc_tool (eris) as the 80%-case and runnable_dependencies for shell_command as the advanced case.

Given that, the primary things that you need to learn with each approach seem to be:

These lists are accurate as far as the basic things you need to learn are concerned, but also important are the consequences and side-effects of each approach. As you point out, you need to understand Pants' constructs for moving things around the filesystem if your adhoc_tool does not do the right thing, which is more difficult if you need the output from a tool to be in precisely one place in all subsequent dependents.

There are composition patterns introduced by runnable_dependencies that -- outside of sandboxed execution -- should be simple, but end up being more difficult to achieve.

As an example (by no means the only one): Say you have two runnable targets, a and b, and a needs to run b. There is a shell command called c that runs a.

An understanding of how Pants does dependencies says that you should probably declare b as a dependency of a, and supply a as the sole runnable_dependency for c. This is actually not the case, you'd need to declare a and b as runnable_dependencies for c (unless we start defining runnable_dependencies in a bunch of other places, but that does get complicated). This is a fundamentally different understanding of how dependencies work, and it deserves a good explanation in our docs.

I think the point about structuring documentation remains a salient one here: I think the 80% case becomes easier to explain if we have adhoc_tool, which will allow us to give readers the detail they deserve when we discuss runnable_dependencies in shell_command.

[kaos]

(by no means not the only one)

can't resist pointing out the double-negate mistake on this one 🤣

[chrisjrn]

can't resist pointing out the double-negate mistake on this one 🤣

fixed

[jsirois]

Pants should be guiding users towards making adhoc build steps (using this terminology/variations of it to describe esc and eris collectively) that are as testable and verifiable as the rest of their codebase.

Re the above and other similar concerns: Isn't it the case the inline script could be materialized (in a sandbox) as a file and then acted upon by the normal Pants infra? If I ./pants fmt lint check test a/rule:inline I'd hope we would do that including re-writing the BUILD file with the inline script to include the formatted inline script!. It would also be great if the type of shell could be specified. I might even flip it to python in that case and write a 10-line inline python script with the assurance its formatted, linted, tested, etc. As a non-Pants user over in Pex, Pants getting out of the way more and more will tempt me more and more. Having to define a bunch of targets (what the heck are those?) is very offputting. The less the better. The more I can see in 1 spot the better. 1 target is ~1 spot and the small evil. 10 targets is pretty unfriendly.

[chrisjrn]

Note that I am considering the naming discussion concluded at this point.

Following the above discussion:

I'm in favour of having adhoc_tool as a way of running a single runnable (which I believe is a use case we should be directly encouraging), and adding a runnable_dependencies field as an advanced feature for shell_command.

An alternative way forward is to pursue only adding the runnable_dependencies field to shell_command.

[stuhood]

I'm in favour of having adhoc_tool as a way of running a single runnable (which I believe is a use case we should be directly encouraging), and adding a runnable_dependencies field as an advanced feature for shell_command.

An alternative way forward is to pursue only adding the runnable_dependencies field to shell_command.

It sounds like a prerequisite for both of these would be actually implementing the "runnable_dependencies for shell_command" option, so that folks can compare and contrast them more concretely?

[chrisjrn]

It sounds like a prerequisite for both of these would be actually implementing the "runnable_dependencies for shell_command" option, so that folks can compare and contrast them more concretely?

Given @cognifloyd's contribution (and reading a bit into @kaos' 👍 reactions in various places), it sounds like other maintainers share my hesitancy to make shell_command more powerful as a first resort.

Specifically, I think @cognifloyd highlighting the unmaintanability of arbitrary shell snippets in a build graph context is a powerful argument against immediately proceeding with the expanded shell_command functionality. We already have composability in the form of various Pants target types*, and I'm not convinced that it's limiting, merely cumbersome, and even then in ways that impact the usability of every target type (not just adhoc_tool), which we'd do well to fix across the board.

I'm leaning towards marking runnable_dependencies as future work (strictly as an advanced option at first; possibly even as a fastish-follow), leaving open the option of deprecating adhoc_tool if it turns out to be exactly the wrong choice.

---

*: Our digest manipulation targets may well be too verbose at the moment, but I think the goal of keeping these entirely within BUILDfile syntax is a sound one.

[kaos]

Don't read too much into my 👍🏽 ;)

I like the adhoc_tool as target name and for its purpose. I've not invested the time to review all the arguments presented regarding the shell command targets and fields to have a well informed opinion for either side here.. I appreciate the amount of time and energy invested in these questions to which I've not kept up 😬

[stuhood]

Specifically, I think @cognifloyd highlighting the unmaintanability of arbitrary shell snippets in a build graph context is a powerful argument against immediately proceeding with the expanded shell_command functionality. We already have composability in the form of various Pants target types*, and I'm not convinced that it's limiting, merely cumbersome, and even then in ways that impact the usability of every target type (not just adhoc_tool), which we'd do well to fix across the board.

My read of @cognifloyd 's comment put more weight on:

That said, I think it can also be taken too far in the other direction. A BUILD file with a bunch of targets is harder to grok than a BUILD file with just a few targets. (Concise is better). I'm trying very hard to get away from Makefiles because it can be so hard to understand and follow what is going on. So, I really hope that whichever solution we use in BUILD files is easier to follow than a Makefile with its crazy graph of shell snippets (both the shell and the graph make that hard to grok).

...i.e., that conciseness is important, whether it's shell or a large graph of targets.

[cognifloyd]

Ooh... I can play this game too 😁 I think @cognifloyd wanted to emphasize two things:

I dislike Dockerfiles and Makefiles because they are a collection of shell steps. Shell snippets invariably become quite complex as edge cases have to be handled and guarded against.

So, I really hope that whichever solution we use in BUILD files is easier to follow than a Makefile with its crazy graph of shell snippets (both the shell and the graph make that hard to grok).

So, to rephrase:

1. Please don't advertise shell_command as the "pants-blessed" solution because I really dislike embedded shell snippets; I want to push people away from using shell. adhoc_tool feels like an excellent step in that direction (maybe rename experimental_run_in_sandbox to experimental_adhoc_tool for now).

2. And, I want the non-shell solution in BUILD files to be concise.

[chrisjrn]

OK, this has gone quiet for a day or two, so here's where I think we're at:

1. People like shell_command and adhoc_tool as names for experimental_shell_command and experimental_run_in_sandbox respectively
2. There are concerns about the utility of adhoc_tool with regards to composability of runnables within the same sandbox
3. There are concerns about the maintainability of arbitrary shell snippets in a build graph context, which merit treating further extending shell_command with caution

With that in mind, here's how I would like to proceed:

PROPOSAL: Rename and move experimental_run_in_sandbox from the shell subsystem. The proposed new subsystem name is pants.backend.adhoc, and the target would be named adhoc_tool.

PROPOSAL: adhoc_tool is documented as the preferred way to run third-party tools or user-supplied code as a build step, whenever it is possible. shell_command should be introduced only for cases where Pants does not provide any support.

PROPOSAL: Rename experimental_shell_command to shell_command, documenting it as the preferred option for handling small shell snippets.

PROPOSAL: Create a ticket describing an experimental_runnable_dependencies field for shell_command, allowing users to invoke Pants runnables from within a shell_command. The ticket involves design and documentation work; it may be better to leave this until we approach scoped dependencies.

PROPOSAL: Create a ticket for making shell_source targets runnable. This will need to allow users to describe the tools used by a shell script (nb. maybe this means folding the tools field into runnable_dependencies?)

PROPOSAL: Create a ticket (or update an existing one) for making digest manipulation actions (e.g. alter prefix, rename, etc) easier within BUILD files

This effectively represents stabilising the features that already exist within Pants' main, leaving open the possibility of deprecating adhoc_tool should the runnable_dependencies concept prove truly insufficient.

---

EDIT: 2023-02-06 Per @jsirois comment I'm adding the following two proposals

PROPOSAL: Create a ticket describing a feature that allows users to select a shell other than bash for use in shell_command or runnable shell_source targets.
PROPOSAL: Create a ticket describing a feature that will lint/fix/format shell one-liners within a shell_command target.

[stuhood]

Also, although genrules are poorly named (imo), this is a relevant bar for comparison: https://jin.crypt.sg/articles/bazel-in-5-minutes-genrule.html

[chrisjrn]

It turns out the antlr demo could have been a lot simpler, if I'd bothered to learn the command line arguments for antlr and checked whether the touch __init__.py were actually necessary (it's not):

experimental_run_in_sandbox(
    name="run_antlr",
    runnable=":antlr4",
    args=["Expr.g4", "-Dlanguage=Python3", "-o", "../expr_parser", "-package", "expr_parser",],
    output_directories=["../expr_parser",],
    execution_dependencies=[":grammars"],
    workdir=".",
)

This will be even simpler once I add the output_directory field!

[jsirois]

That's nice in this case, but I think it makes sense to accept there will be plenty of not 1-liners. Instead of judging the person using Pants in their repo, I'd love it if we made it easy for them to do the right thing. If we allowed picking the shell and we allowed applying Pants goals to the inline script for that shell, including re-formating that inline script using pants fmt - that would be lovely. I'm pretty sure I'd only use that and if I needed more - I'd write rules.

[cognifloyd]

If we allowed picking the shell and we allowed applying Pants goals to the inline script for that shell, including re-formating that inline script using pants fmt - that would be lovely.

+1: applying fmt/lint to inline scripts (that mitigates some of the evil of those embedded snippets).
+1: Allow defining the interpreter to use for that script is also good. embedding python is a fascinating idea.

So maybe shell_command is also the wrong approach - it doesn't even specify which shell to use. So maybe we have two targets:

• adhoc_tool
• adhoc_script (replacing experimental_shell_command, but requiring specifying the sh/bash/zsh/python/expect/etc)

[chrisjrn]

Instead of judging the person using Pants in their repo, I'd love it if we made it easy for them to do the right thing.

I don't think it's a matter of judgment, merely of giving people the right tool for the job. My gut feeling is that for 80% of cases,
adhoc_tool is the thing that will look least out of place in a BUILD file. As of main today, the antlr example is very natural indeed:

experimental_run_in_sandbox(
    name="run_antlr",
    runnable=":antlr4",
    args=["Expr.g4", "-Dlanguage=Python3", "-o", "expr_parser", "-package", "expr_parser",],
    output_directories=["expr_parser",],
    execution_dependencies=[":grammars"],
)

No relocated_files, no extra touch commands, etc. It doesn't even need to specify a workdir any more.

For people who have an understanding of how Pants does sandboxing, shell_command with extra bells and whistles is a good solution. I'm happy documenting it as the advanced case, and am doing work to make it something that we can implement (#18168, in combination with synthetic targets pointing at system binaries should make a runnable_dependencies field very achievable).

If we allowed picking the shell and we allowed applying Pants goals to the inline script for that shell, including re-formating that inline script using pants fmt - that would be lovely.

I think these are both good features to include, and I'm going to add ticket-creating tasks as action items. I don't think they block stabilisation thought.

[benjyw]

Reading through all this, and with my not-letting-the-great-be-the-enemy-of-the-good hat on, I think stabilizing eris makes sense (whether in 2.16 or 2.17 is open to debate). It covers substantial real use-cases in a streamlined way (although I still want to see some JS examples...)

I'd like to see real-world demand for the proposed composability of runnables inside an esc before deciding that going down that route is worth stalling for.

[stuhood]

It covers substantial real use-cases in a streamlined way (although I still want to see some JS examples...)

Yea, to be clear: being able to run a runnable is super useful. The debate is about UX, rather than about whether it's functionality we want.

I'd like to see real-world demand for the proposed composability of runnables inside an esc before deciding that going down that route is worth stalling for.

There are examples in @danxmoran's comments above re: Javascript ("I will lump..."), and in @chrisjrn's antlr example.

It's worth noting that the composition is about more than just composition of runnable target types: it's also about composition of other shell_command tools... i.e. mv, cp, ln, echo, cat, etc.

[benjyw]

Also, we haven't (I think?) discussed the option of a shell script itself being a runnable via a "shell_script()" target.

Right now eris/esc and friends are in the shell backend, but that is confusing things. The shell backend should be for "I have shell sources in my codebase", just like python, go, java etc. eris/esc and friends don't interact with any of that (afaict) and are really more like core Pants mechanisms.

But just as eris can run a Python or Java runnable, if the relevant backend is enabled, it should be able to run a Shell one if that backend is enabled.

[chrisjrn]

This is the 5th proposal in the most recent list of proposals.

Note that I think shell_command still makes sense for genuine one-liners, which a lot of shell_commands seem to be.

[benjyw]

So, we have what appears to be a useful thing with adhoc_tool. Why not release it, and see how much use people get out of it, as we continue to look at expanding the capabilities of shell_command ?

[chrisjrn]

With #18237, I've implemented something close to what @cognifloyd suggested in his comment:

• experimental_run_in_sandbox is now called adhoc_tool
• registration for adhoc_tool lives in an experimental package called pants.backend.experimental.adhoc

With this change adopted, we would be in a position to stabilise shell_command as-is in time for 2.16.

Thoughts?

[chrisjrn]

The changes in #18082 (comment) have now been applied. I'll be creating the tickets suggested earlier on in the ticket shortly.

[chrisjrn]

New issues:

• Extend shell_command tutorial/how-to documentation with discussion of adhoc_tool #18319
• Add support for describing "runnable" dependencies of a target #18320
• Add support for consuming shell_source through pants run/adhoc_tool #18321
• Create a new target for bulk file layout manipulation options #18322
• Allow users to define the shell to use with shell_command/shell_source #18323
• Allow shell_command's command field to be linted/formatted #18324

Thanks everyone for the thoughtful discussion.