Skip to content
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

Add extra_env_vars field to experimental_shell_command target. #15742

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/python/pants/backend/shell/shell_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from pants.backend.shell.shell_setup import ShellSetup
from pants.backend.shell.target_types import (
ShellCommandCommandField,
ShellCommandExtraEnvVarsField,
ShellCommandLogOutputField,
ShellCommandOutputsField,
ShellCommandRunWorkdirField,
Expand Down Expand Up @@ -115,6 +116,7 @@ async def prepare_shell_command_process(
timeout = shell_command.get(ShellCommandTimeoutField).value
tools = shell_command.get(ShellCommandToolsField, default_raw_value=()).value
outputs = shell_command.get(ShellCommandOutputsField).value or ()
extra_env_vars = shell_command.get(ShellCommandExtraEnvVarsField).value or ()

if not command:
raise ValueError(
Expand Down Expand Up @@ -162,6 +164,9 @@ async def prepare_shell_command_process(
rationale=f"execute `{shell_command.alias}` {shell_command.address}",
)

extra_env = await Get(Environment, EnvironmentRequest(extra_env_vars))
command_env.update(extra_env)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried setting command_env = await Get(Environment, EnvironmentRequest(extra_env_vars)) from the start, but Environment is frozen so it doesn't allow the follow-up additions needed to set the tools path


transitive_targets = await Get(
TransitiveTargets,
TransitiveTargetsRequest([shell_command.address]),
Expand Down
29 changes: 29 additions & 0 deletions src/python/pants/backend/shell/shell_command_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -404,3 +404,32 @@ def test_shell_command_boot_script(rule_runner: RuleRunner) -> None:
assert sorted(res.env["TOOLS"].split()) == tools
for tool in tools:
assert res.env[tool].endswith(f"/{tool.replace('_', '.')}")


def test_shell_command_extra_env_vars(caplog, rule_runner: RuleRunner) -> None:
caplog.set_level(logging.INFO)
caplog.clear()
rule_runner.set_options([], env={"FOO": "foo"}, env_inherit={"PATH"})
rule_runner.write_files(
{
"src/BUILD": dedent(
"""\
experimental_shell_command(
name="extra-env-test",
tools=["echo"],
extra_env_vars=["FOO", "HELLO=world", "BAR"],
command='echo FOO="$FOO" HELLO="$HELLO" BAR="$BAR"',
log_output=True,
)
"""
)
}
)

assert_shell_command_result(
rule_runner,
Address("src", target_name="extra-env-test"),
expected_contents={},
)

assert_logged(caplog, [(logging.INFO, "FOO=foo HELLO=world BAR=\n")])
12 changes: 12 additions & 0 deletions src/python/pants/backend/shell/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,17 @@ class ShellCommandToolsField(StringSequenceField):
)


class ShellCommandExtraEnvVarsField(StringSequenceField):
alias = "extra_env_vars"
help = softwrap(
"""
Additional environment variables to include in the shell process.
Entries are strings in the form `ENV_VAR=value` to use explicitly; or just
`ENV_VAR` to copy the value of a variable in Pants's own environment.
"""
)
Comment on lines +306 to +312
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth extracting this help string to a templating function, maybe in src/python/pants/engine/environment.py ? It's duplicated in at least src/python/pants/backend/python/target_types.py and src/python/pants/core/goals/test.py.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... or defining a Field superclass that provides it? Maybe overkill.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be done as a follow up as well.



class ShellCommandLogOutputField(BoolField):
alias = "log_output"
default = False
Expand All @@ -324,6 +335,7 @@ class ShellCommandTarget(Target):
ShellCommandSourcesField,
ShellCommandTimeoutField,
ShellCommandToolsField,
ShellCommandExtraEnvVarsField,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this to the end but looks like it should maybe be put in alphabetical order? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm one in favor of sorting lists alphabetically, however here I think the order affects the order the fields are presented in the documentation so that is worth considering too.

)
help = softwrap(
"""
Expand Down