From df352ef8150b4b25bd5093fc94ea971ec664211c Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 27 Apr 2023 06:02:30 +1000 Subject: [PATCH] Write adhoc_tool(stdout/stderr="...") relative to workdir, support absolute paths (#18814) This fixes #18810 by ensuring `adhoc_tool(stdout="...", stderr="...")` are written relative to the working directory, and before the `root_output_directory` filtering happens. This also fixes a second bug I noticed while working on this: if the path is absolute, e.g. `stdout="/out.txt"`, pants crashes: ```panic at 'called `Result::unwrap()` on an `Err` value: "Absolute paths are not allowed: \"/out.txt\""', src/intrinsics.rs:420```. After this PR, they are supported, and taken to start at the build root. --- src/python/pants/backend/adhoc/adhoc_tool.py | 22 ++------ .../pants/backend/adhoc/adhoc_tool_test.py | 51 ++++++++++++------- .../pants/backend/adhoc/target_types.py | 10 ++-- .../backend/shell/util_rules/shell_command.py | 2 + .../core/util_rules/adhoc_process_support.py | 32 +++++++++++- 5 files changed, 75 insertions(+), 42 deletions(-) diff --git a/src/python/pants/backend/adhoc/adhoc_tool.py b/src/python/pants/backend/adhoc/adhoc_tool.py index 6447e11b7b0..61971898cd9 100644 --- a/src/python/pants/backend/adhoc/adhoc_tool.py +++ b/src/python/pants/backend/adhoc/adhoc_tool.py @@ -35,7 +35,7 @@ from pants.core.util_rules.environments import EnvironmentNameRequest from pants.engine.addresses import Addresses from pants.engine.environment import EnvironmentName -from pants.engine.fs import CreateDigest, Digest, FileContent, MergeDigests, Snapshot +from pants.engine.fs import Digest, MergeDigests, Snapshot from pants.engine.internals.native_engine import EMPTY_DIGEST from pants.engine.rules import Get, collect_rules, rule from pants.engine.target import ( @@ -167,6 +167,8 @@ async def run_in_sandbox_request( supplied_env_var_values=FrozenDict(extra_env), log_on_process_errors=None, log_output=target[AdhocToolLogOutputField].value, + capture_stderr_file=target[AdhocToolStderrFilenameField].value, + capture_stdout_file=target[AdhocToolStdoutFilenameField].value, ) adhoc_result = await Get( @@ -177,23 +179,7 @@ async def run_in_sandbox_request( }, ) - result = adhoc_result.process_result - adjusted = adhoc_result.adjusted_digest - - extras = ( - (target[AdhocToolStdoutFilenameField].value, result.stdout), - (target[AdhocToolStderrFilenameField].value, result.stderr), - ) - extra_contents = {i: j for i, j in extras if i} - - if extra_contents: - extra_digest = await Get( - Digest, - CreateDigest(FileContent(name, content) for name, content in extra_contents.items()), - ) - adjusted = await Get(Digest, MergeDigests((adjusted, extra_digest))) - - output = await Get(Snapshot, Digest, adjusted) + output = await Get(Snapshot, Digest, adhoc_result.adjusted_digest) return GeneratedSources(output) diff --git a/src/python/pants/backend/adhoc/adhoc_tool_test.py b/src/python/pants/backend/adhoc/adhoc_tool_test.py index 21d6d35c809..be15362f817 100644 --- a/src/python/pants/backend/adhoc/adhoc_tool_test.py +++ b/src/python/pants/backend/adhoc/adhoc_tool_test.py @@ -141,29 +141,42 @@ def test_adhoc_tool_with_workdir(rule_runner: RuleRunner) -> None: ) -def test_adhoc_tool_capture_stdout_err(rule_runner: RuleRunner) -> None: +@pytest.mark.parametrize( + ("write_dir", "workdir", "root_output_directory", "expected_dir"), + [ + # various relative paths: + ("", None, None, "src/"), + ("dir/", None, None, "src/dir/"), + ("../", None, None, ""), + # absolute path + ("/", None, None, ""), + # interaction with workdir and root_output_directory: + ("", "/", None, ""), + ("dir/", None, ".", "dir/"), + ("3/", "1/2", "1", "2/3/"), + ], +) +def test_adhoc_tool_capture_stdout_err( + rule_runner: RuleRunner, + write_dir: str, + workdir: None | str, + root_output_directory: None | str, + expected_dir: str, +) -> None: rule_runner.write_files( { - "src/fruitcake.py": dedent( - """\ - import sys - print("fruitcake") - print("inconceivable", file=sys.stderr) - """ - ), "src/BUILD": dedent( - """\ - python_source( - source="fruitcake.py", - name="fruitcake", - ) + f"""\ + system_binary(name="bash", binary_name="bash") adhoc_tool( name="run_fruitcake", - runnable=":fruitcake", - stdout="stdout", - stderr="stderr", - root_output_directory=".", + runnable=":bash", + args=["-c", "echo fruitcake; echo inconceivable >&2"], + stdout="{write_dir}stdout", + stderr="{write_dir}stderr", + workdir={workdir!r}, + root_output_directory={root_output_directory!r}, ) """ ), @@ -174,8 +187,8 @@ def test_adhoc_tool_capture_stdout_err(rule_runner: RuleRunner) -> None: rule_runner, Address("src", target_name="run_fruitcake"), expected_contents={ - "stderr": "inconceivable\n", - "stdout": "fruitcake\n", + f"{expected_dir}stderr": "inconceivable\n", + f"{expected_dir}stdout": "fruitcake\n", }, ) diff --git a/src/python/pants/backend/adhoc/target_types.py b/src/python/pants/backend/adhoc/target_types.py index 142e2f6285e..4fddd7d5138 100644 --- a/src/python/pants/backend/adhoc/target_types.py +++ b/src/python/pants/backend/adhoc/target_types.py @@ -154,8 +154,9 @@ class AdhocToolStdoutFilenameField(StringField): default = None help = help_text( lambda: f""" - A filename to capture the contents of `stdout` to, relative to the value of - `{AdhocToolWorkdirField.alias}`. + A filename to capture the contents of `stdout` to. Relative paths are + relative to the value of `{AdhocToolWorkdirField.alias}`, absolute paths + start at the build root. """ ) @@ -165,8 +166,9 @@ class AdhocToolStderrFilenameField(StringField): default = None help = help_text( lambda: f""" - A filename to capture the contents of `stderr` to, relative to the value of - `{AdhocToolWorkdirField.alias}` + A filename to capture the contents of `stderr` to. Relative paths are + relative to the value of `{AdhocToolWorkdirField.alias}`, absolute paths + start at the build root. """ ) diff --git a/src/python/pants/backend/shell/util_rules/shell_command.py b/src/python/pants/backend/shell/util_rules/shell_command.py index 5d0084ab3ac..1fd05a556fd 100644 --- a/src/python/pants/backend/shell/util_rules/shell_command.py +++ b/src/python/pants/backend/shell/util_rules/shell_command.py @@ -157,6 +157,8 @@ async def _prepare_process_request_from_target( immutable_input_digests=FrozenDict.frozen(merged_extras.immutable_input_digests), log_on_process_errors=_LOG_ON_PROCESS_ERRORS, log_output=shell_command[ShellCommandLogOutputField].value, + capture_stdout_file=None, + capture_stderr_file=None, ) diff --git a/src/python/pants/core/util_rules/adhoc_process_support.py b/src/python/pants/core/util_rules/adhoc_process_support.py index d9c1cc1f0fc..2b0fa13be32 100644 --- a/src/python/pants/core/util_rules/adhoc_process_support.py +++ b/src/python/pants/core/util_rules/adhoc_process_support.py @@ -61,6 +61,8 @@ class AdhocProcessRequest: supplied_env_var_values: FrozenDict[str, str] | None log_on_process_errors: FrozenDict[int, str] | None log_output: bool + capture_stdout_file: str | None + capture_stderr_file: str | None @dataclass(frozen=True) @@ -410,7 +412,25 @@ async def run_adhoc_process( request.root_output_directory, working_directory ) - adjusted = await Get(Digest, RemovePrefix(result.output_digest, root_output_directory)) + extras = ( + (request.capture_stdout_file, result.stdout), + (request.capture_stderr_file, result.stderr), + ) + extra_contents = {i: j for i, j in extras if i} + + output_digest = result.output_digest + + if extra_contents: + extra_digest = await Get( + Digest, + CreateDigest( + FileContent(_parse_relative_file(name, working_directory), content) + for name, content in extra_contents.items() + ), + ) + output_digest = await Get(Digest, MergeDigests((output_digest, extra_digest))) + + adjusted = await Get(Digest, RemovePrefix(output_digest, root_output_directory)) return AdhocProcessResult(result, adjusted) @@ -508,5 +528,15 @@ def _parse_relative_directory(workdir_in: str, relative_to: Union[Address, str]) return workdir_in +def _parse_relative_file(file_in: str, relative_to: str) -> str: + """Convert the `capture_std..._file` fields into something that can be understood by + `Process`.""" + + if file_in.startswith("/"): + return file_in[1:] + + return os.path.join(relative_to, file_in) + + def rules(): return collect_rules()