New `experimental_shell_command` #12878

kaos · 2021-09-14T10:17:49Z

The new experimental_shell_command added to the shell backend allows running arbitrary commands during pants execution.

This target is for introducing side effects in the build, either as new or modified files, calling out to external services or managing some other state. It remains important to ensure idempotency however, as the command may be cancelled or retried on the sole discretion of Pants.

For those familiar with Bazel, the experimental_shell_command has similarities with the Bazel genrule.

Fixes #3734

Example BUILD file usage:

shell_library(name="build-tools")
experimental_shell_command(
  command="./build-util.sh -o output do-things",
  tools=["bash", "env", "cat", "curl", "tar"],
  outputs=["output/"],
  dependencies=[":build-tools"],
)

The dependencies will pull in scripts from shell_library, arbitrary files from files and other experimental_shell_command targets, the outputs lists directories and files to capture, which may be included by consuming targets, and tools lists all required executables that command may be using.

The [shell-setup].executable_search_paths option is used when finding the specified tools.

Signed-off-by: Andreas Stenius <[email protected]> # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

kaos · 2021-09-15T18:23:48Z

@stuhood any thoughts on the direction of this?

stuhood · 2021-09-15T18:26:14Z

Sorry for the delayed review! Excited about it, but haven't taken a look yet. Will do today.

stuhood

Thanks a lot! This looks like the right track. Allowing gen_rule outputs to be consumed by other targets as Files is probably the most critical bit to implement.

src/python/pants/core/util_rules/gen_rule.py

src/python/pants/core/util_rules/gen_rule_test.py

src/python/pants/core/target_types.py

stuhood · 2021-09-16T03:58:46Z

Also curious whether @Eric-Arellano thinks this might need/want to integrate with shell_library.

Signed-off-by: Andreas Stenius <[email protected]>

kaos

I'd be happy to rename gen_rule => shell or something like that. Apart from that, I feel this is getting pretty close to done.

I don't think we have to consider the difference in glob behaviour between git style and shell style too much. As long as it is clear that the command line is parsed by a shell, it shouldn't be any surprises there.

We could potentially however provide more data in the environment, regarding inputs and outputs etc.

src/python/pants/core/target_types.py

src/python/pants/core/util_rules/gen_rule.py

src/python/pants/core/util_rules/gen_rule_test.py

src/python/pants/core/util_rules/gen_rule.py

stuhood

This looks really great: thanks a lot!

I think that this is ready to land with an experimental_ prefixed name.

src/python/pants/core/target_types.py

src/python/pants/core/util_rules/gen_rule.py

src/python/pants/core/util_rules/gen_rule_test.py

Eric-Arellano · 2021-09-21T05:26:29Z

Also curious whether @Eric-Arellano thinks this might need/want to integrate with shell_library.

What might that look like? I think it makes sense to be separate. Fwict, shell_library describes shell source code checked in to disk. Iiuc, this new target does not require anything on-disk necessarily.

Eric-Arellano

Cool! I looked mostly at the modeling of the target, not the actual rule implementation

src/python/pants/core/target_types.py

Eric-Arellano · 2021-09-21T05:34:22Z

src/python/pants/core/util_rules/gen_rule.py

+
+class GenerateFilesFromGenRuleRequest(GenerateSourcesRequest):
+    input = GenRuleSources
+    output = FilesSources


Note that Files are not (currently) included when building a pex_binary and python_awslambda. They're mostly helpful for tests and for archive. Is that okay?

Ah, I'd like the output to be as generic as possible, is plains Sources better, then?
Or rather, I want to make sure that whatever is produced by the shell_command (if we call it that) may be used as sources by another target.

Not sure if simply depending on a shell_command, from say a python_library is enough, or if you should include the output files from shell_command in the python_library sources field? But that may complain that the "glob doesn't match" any files, until the shell_command has been executed, if that is an issue?

Co-authored-by: Eric Arellano <[email protected]> Co-authored-by: Stu Hood <[email protected]>

[ci skip-rust]

kaos · 2021-09-21T07:00:49Z

Seing now this as a (experimental_)shell_command, maybe it ought to live in the shell backend?

kaos · 2021-09-21T07:51:25Z

I just realize that I really doesn't like to use $tool instead of just tool. That puts a lot of constraints on the script to run, and also complicates if you want to run it outside of a pants execution.

Just take the leading shebang, for example (is what prompted me down this lane to begin with):

#!/usr/bin/env bash
...

This won't work, but should instead be just

#!$bash
...

Which doesn't work either, as that line is not expanded.

So, I propose that we setup a .tools/bin/ folder, and symlink all requested tools in there, and list that in the search path PATH=/path/to/sandbox/.tools/bin so that ordinary env works, etc.

Edit: hmm... can I create symlinks in a Digest? otherwise, I guess this doesn't work, or?

Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

kaos · 2021-09-21T14:26:07Z

I moved the whole implementation of experimental_shell_command to the shell backend.
Added a new field log_output, anticipating that it will be good to be able to print something informative from the command, in case it is only side-effecting without any outputs captured, at least.
Sets up a .bin dir with symlinks to all tools specified for use, and points PATH at that .bin dir, so we can use cd, cat etc without having to use parameter expansion from the environment on them.

stuhood

This looks really fantastic: thanks a lot @kaos!

stuhood · 2021-09-21T16:43:46Z

src/python/pants/backend/shell/shell_command.py

+    )
+
+    command_env = {
+        "TOOLS": " ".join(tools),


It's unlikely, but tools might have spaces in their paths. Should shlex.escape them before joining them.

Eric-Arellano

This is awesome, great work!

Eric-Arellano · 2021-09-21T17:36:07Z

src/python/pants/core/register.py

@@ -33,22 +33,22 @@ def rules():
        *package.rules(),


Thanks for fixing the ordering :)

Yeah, it was hurting my eyes, and C-space C-s ] C-p M-x so-li enter is a mere 2.5 second fix :P

Eric-Arellano · 2021-09-21T17:37:53Z

src/python/pants/backend/shell/target_types.py

+    help = dedent(
+        """\
+        Execute any external tool for its side effects.
+        This may be retried and/or cancelled, so ensure that it is idempotent.


Nit, should probably move below the example. This is a nuanced detail that doesn't explain what this target is, only a limitation when using it. The example is really helpful to figure out what it is.

Might also be worth adding a paragraph below the example explaining how you can depend on shell_library targets to run its script, or you can directly invoke Bash commands like touch my_file.ext.

Oh! Another key detail missing from this help, you must add this target to the dependencies of each consumer, such as your python_tests. When relevant, Pants will run your command and insert the output_files into that build's context.

Might also be worth adding a paragraph below the example explaining how you can depend on shell_library targets to run its script

Oh, wait, I don't think I fully understand what you mean here.

Oh, could we get around this limitation, at least to some extent, if we implement the GenerateTargetsRequest union for shell_command as well (and generate file targets for any output files)?

IMO, only being able to bring in all of the output files of the script is totally fine for now. And files is almost certainly the right output type.

GenerateTargetsRequest wouldn't be able to fully determine which files existed without actually running the script, because directory/ entries include the children of the directory.

(@Eric-Arellano : Which reminds me: we probably ought to document an expectation that target generation runs quickly, and/or is stably cacheable, since it will run for all graph introspection)

Oh, wait, I don't think I fully understand what you mean here.

What I mean is that if you set your command to be ./my_script.sh, you should add a shell_library target which includes my_script.sh in its sources and include that in the dependencies field.

Eric-Arellano · 2021-09-21T17:40:16Z

src/python/pants/backend/shell/shell_command.py

+
+class GenerateFilesFromShellCommandRequest(GenerateSourcesRequest):
+    input = ShellCommandSources
+    output = FilesSources


To respond to the earlier thread: I think this is the right choice for now to start, rather than Sources.

Note also that callers must set enable_codegen=True for this codegen to happen.

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood · 2021-09-22T15:58:45Z

With a fresh description and title, I think that this is probably ready to land! Thanks again.

Eric-Arellano · 2021-09-22T17:17:23Z

src/python/pants/backend/shell/target_types.py

+        "Execute any external tool for its side effects.\n"
+        + dedent(


Yay thanks for doing that formatting :) This is great because Pants wraps lines based on your terminal width, but we need to use implicit string concatenation for that to work properly.

(The docs also work better when using implicit string concatenation so that the browser can wrap for you.)

Yes, I remember you told me this once before ;)

kaos · 2021-09-22T17:47:15Z

Right, I forgot there is one more thing I'd like to include here before I feel it is ready to go.. that is I'd like to use the executable-search-paths option from the shell-setup subsystem, rather than using a hard coded list of the most common search paths.

Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano · 2021-09-22T18:36:59Z

src/python/pants/backend/shell/target_types.py

+        "and these tools must be found on the paths provided by "
+        "[shell-setup].executable-search-paths (which defaults to the system PATH)."


Love it. Great detail!

I am so excited for this!

Random nit: Don't option names in config have to use underscores? i.e., executable_search_paths? We let them be with dashes on the cmd line (--shell-setup-executable-search-paths) because --shell-setup-executable_search_paths looks awful, but I think we at least encourage underscores in option names (but dashes in scope names, so it's still shell-setup).

Oh yeah true, I think it needs to be underscores for the option part

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

kaos · 2021-09-22T20:15:06Z

I copied the PR description in as merge commit message, kept author info. I assume you can disable, and fixup if that is not what you want? :)

Eric-Arellano · 2021-09-22T20:24:21Z

I copied the PR description in as merge commit message, kept author info. I assume you can disable, and fixup if that is not what you want? :)

That sounds good! The most important part is the PR title being useful for the changelog, which this is. (Nit that our changelog is Markdown, so escaping with ` is useful)

For the description, copying in the PR description is great too. And totally fine to include author info. Also you can copy the [ci skip-rust] part when it's safe to.

Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

gen_rule poc.

22dd5d7

Signed-off-by: Andreas Stenius <[email protected]> # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood reviewed Sep 16, 2021

View reviewed changes

kaos added 2 commits September 17, 2021 08:34

Merge remote-tracking branch 'upstream/main' into issue/3734_gen_rule

fc49e13

gen_rule add deps etc.

1815f7a

Signed-off-by: Andreas Stenius <[email protected]>

kaos marked this pull request as ready for review September 17, 2021 12:38

kaos commented Sep 17, 2021

View reviewed changes

src/python/pants/core/util_rules/gen_rule.py Outdated Show resolved Hide resolved

stuhood approved these changes Sep 21, 2021

View reviewed changes

Eric-Arellano reviewed Sep 21, 2021

View reviewed changes

kaos and others added 2 commits September 21, 2021 08:37

Apply suggestions from code review

d1e499f

Co-authored-by: Eric Arellano <[email protected]> Co-authored-by: Stu Hood <[email protected]>

Merge remote-tracking branch 'upstream/main' into issue/3734_gen_rule

f82a319

[ci skip-rust]

kaos added 7 commits September 21, 2021 09:57

refactoring after review.

3b610f3

Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

Setup .bin/ dir for PATH.

a22994b

Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

Test without output.

88ac2d4

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

drop the dollars.

b5bd5cf

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

setup PATH inside command process, to get sandbox dir.

55e3461

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

Add log_output field. todo: add tests to ensure correct info is logged.

4153ed0

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

test log_output.

182d97f

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

stuhood approved these changes Sep 21, 2021

View reviewed changes

Eric-Arellano approved these changes Sep 21, 2021

View reviewed changes

review feedback.

e16e15a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano reviewed Sep 22, 2021

View reviewed changes

kaos changed the title ~~gen_rule poc.~~ New experimental_shell_command Sep 22, 2021

kaos added 2 commits September 22, 2021 20:24

use executable search paths from shell-setup.

e3acfa5

Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Merge remote-tracking branch 'upstream/main' into issue/3734_gen_rule

92c53d4

Eric-Arellano reviewed Sep 22, 2021

View reviewed changes

stuhood and others added 2 commits September 22, 2021 12:47

Add missing rule to shell_command_test.py.

49c0d54

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

fix option typo.

63eff79

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano approved these changes Sep 22, 2021

View reviewed changes

kaos enabled auto-merge (squash) September 22, 2021 20:14

kaos changed the title ~~New experimental_shell_command~~ New experimental_shell_command Sep 22, 2021

kaos mentioned this pull request Sep 23, 2021

docker: add plugin hook for customizing build context. #12864

Closed

kaos added 2 commits September 23, 2021 10:19

do not run binary path discovery for builtin commands.

0dc4601

Signed-off-by: Andreas Stenius <[email protected]> # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

do not expose builtin to TOOLS env.

8bd305c

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

kaos merged commit 2da07b6 into pantsbuild:main Sep 23, 2021

kaos deleted the issue/3734_gen_rule branch September 23, 2021 09:10

huonw mentioned this pull request Jun 23, 2023

Builtin bash function restriction limits usability of shell commands #19367

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New `experimental_shell_command` #12878

New `experimental_shell_command` #12878

kaos commented Sep 14, 2021 •

edited

Loading

kaos commented Sep 15, 2021

stuhood commented Sep 15, 2021

stuhood left a comment

stuhood commented Sep 16, 2021

kaos left a comment

stuhood left a comment

Eric-Arellano commented Sep 21, 2021

Eric-Arellano left a comment

Eric-Arellano Sep 21, 2021

kaos Sep 21, 2021

kaos commented Sep 21, 2021

kaos commented Sep 21, 2021 •

edited

Loading

kaos commented Sep 21, 2021

stuhood left a comment

stuhood Sep 21, 2021

Eric-Arellano left a comment

Eric-Arellano Sep 21, 2021

kaos Sep 21, 2021

Eric-Arellano Sep 21, 2021

Eric-Arellano Sep 21, 2021

kaos Sep 22, 2021

kaos Sep 22, 2021

stuhood Sep 22, 2021 •

edited

Loading

Eric-Arellano Sep 22, 2021

Eric-Arellano Sep 21, 2021

stuhood commented Sep 22, 2021 •

edited

Loading

Eric-Arellano Sep 22, 2021

kaos Sep 22, 2021

kaos commented Sep 22, 2021

Eric-Arellano Sep 22, 2021

benjyw Sep 22, 2021

Eric-Arellano Sep 22, 2021

kaos commented Sep 22, 2021

Eric-Arellano commented Sep 22, 2021

		"Execute any external tool for its side effects.\n"
		+ dedent(

		"and these tools must be found on the paths provided by "
		"[shell-setup].executable-search-paths (which defaults to the system PATH)."

New experimental_shell_command #12878

New experimental_shell_command #12878

Conversation

kaos commented Sep 14, 2021 • edited Loading

kaos commented Sep 15, 2021

stuhood commented Sep 15, 2021

stuhood left a comment

Choose a reason for hiding this comment

stuhood commented Sep 16, 2021

kaos left a comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Eric-Arellano commented Sep 21, 2021

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaos commented Sep 21, 2021

kaos commented Sep 21, 2021 • edited Loading

kaos commented Sep 21, 2021

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaos commented Sep 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaos commented Sep 22, 2021

Eric-Arellano commented Sep 22, 2021

New `experimental_shell_command` #12878

New `experimental_shell_command` #12878

kaos commented Sep 14, 2021 •

edited

Loading

kaos commented Sep 21, 2021 •

edited

Loading

stuhood Sep 22, 2021 •

edited

Loading

stuhood commented Sep 22, 2021 •

edited

Loading