diff --git a/buildrunner/docker/runner.py b/buildrunner/docker/runner.py index 4d03758c..3ef678c7 100644 --- a/buildrunner/docker/runner.py +++ b/buildrunner/docker/runner.py @@ -589,13 +589,11 @@ def save_caches( f"It will not be saved again to `{local_cache_archive_file}`" ) - def run(self, cmd, console=None, stream=True, log=None, workdir=None): + def _build_exec_cmdv(self, cmd): """ - Run the given command in the container. + Validate the container/shell are ready and build the argv to exec, + wrapping string commands in the container's shell. """ - # Unused variable - _ = workdir - if isinstance(cmd, str): cmdv = [self.shell, "-xc", cmd] elif ( @@ -607,14 +605,24 @@ def run(self, cmd, console=None, stream=True, log=None, workdir=None): cmdv = cmd else: raise TypeError(f"Unhandled command type: {type(cmd)}:{cmd}") - # if console is None: - # raise Exception('No console!') if not self.container: raise BuildRunnerContainerError("Container has not been started") if not self.shell: raise BuildRunnerContainerError( "Cannot call run if container cmd not shell" ) + return cmdv + + def run(self, cmd, console=None, stream=True, log=None, workdir=None): + """ + Run the given command in the container. + """ + # Unused variable + _ = workdir + + cmdv = self._build_exec_cmdv(cmd) + # if console is None: + # raise Exception('No console!') self._run_log(log, f"Executing: {cmdv}") @@ -672,6 +680,40 @@ def run(self, cmd, console=None, stream=True, log=None, workdir=None): return inspect_res["ExitCode"] raise BuildRunnerContainerError("Error running cmd: no exit code") + def run_and_capture(self, cmd, log=None): + """ + Run the given command in the container and return its exit code and + captured stdout as a string, instead of streaming output to a + console/log or requiring the caller to write output to a file. + """ + cmdv = self._build_exec_cmdv(cmd) + + self._run_log(log, f"Executing: {cmdv}") + + create_res = self.docker_client.exec_create( + self.container["Id"], + cmdv, + tty=False, + ) + # demux separates stdout from the `-x` trace output (which goes to + # stderr), so stdout only contains the command's actual output. + stdout, stderr = self.docker_client.exec_start( + create_res, + stream=False, + demux=True, + ) + if stderr: + self._run_log(log, stderr) + + inspect_res = self.docker_client.exec_inspect(create_res) + if "ExitCode" in inspect_res: + if inspect_res["ExitCode"] is None: + raise BuildRunnerContainerError( + f"Error running cmd ({cmd}): exit code is None" + ) + return inspect_res["ExitCode"], stdout.decode("utf-8") if stdout else "" + raise BuildRunnerContainerError("Error running cmd: no exit code") + def run_script( self, script, diff --git a/buildrunner/steprunner/tasks/run.py b/buildrunner/steprunner/tasks/run.py index b8998adb..3fe95f27 100644 --- a/buildrunner/steprunner/tasks/run.py +++ b/buildrunner/steprunner/tasks/run.py @@ -192,24 +192,13 @@ def _retrieve_artifacts(self, console=None): # pylint: disable=too-many-locals for pattern, properties in self.step.artifacts.items(): # query files for each artifacts pattern, capturing the output # for parsing - stat_output_file = f"{str(uuid.uuid4())}.out" - stat_output_file_local = os.path.join( - self.step_runner.results_dir, - stat_output_file, - ) - exit_code = artifact_lister.run( - f'stat -c "%n{FILE_INFO_DELIMITER}%F" {pattern} >/stepresults/{stat_output_file}', - console=console, - stream=True, + exit_code, output = artifact_lister.run_and_capture( + f'stat -c "%n{FILE_INFO_DELIMITER}%F" {pattern}', log=self.step_runner.log, ) # if the command was successful we found something if exit_code == 0: - with open( - stat_output_file_local, "r", encoding="utf-8" - ) as output_fd: - output = output_fd.read() artifact_files = [af.strip() for af in output.split("\n")] for art_info in artifact_files: if not art_info or FILE_INFO_DELIMITER not in art_info: @@ -263,10 +252,6 @@ def _retrieve_artifacts(self, console=None): # pylint: disable=too-many-locals properties, ) - # remove the stat output file - if os.path.exists(stat_output_file_local): - os.remove(stat_output_file_local) - finally: if artifact_lister: # make sure the current user/group ids of our @@ -297,19 +282,11 @@ def _archive_dir( if properties and properties.get("format", None) == "uncompressed": # recursively find all files in dir and add # each one, passing any properties - find_output_file = f"{str(uuid.uuid4())}.out" - find_output_file_local = os.path.join( - self.step_runner.results_dir, - find_output_file, - ) - find_exit_code = artifact_lister.run( - f"find {artifact_file} -type f >/stepresults/{find_output_file}", - stream=False, + find_exit_code, output = artifact_lister.run_and_capture( + f"find {artifact_file} -type f", log=self.step_runner.log, ) if find_exit_code == 0: - with open(find_output_file_local, "r", encoding="utf-8") as output_fd: - output = output_fd.read() for _file in [_f.strip() for _f in output.split("\n")]: if not _file: continue @@ -348,10 +325,6 @@ def _archive_dir( properties, ) - # remove the find output file - if os.path.exists(find_output_file_local): - os.remove(find_output_file_local) - else: filename = os.path.basename(artifact_file) diff --git a/examples/artifacts/buildrunner.yaml b/examples/artifacts/buildrunner.yaml new file mode 100644 index 00000000..fedf9a32 --- /dev/null +++ b/examples/artifacts/buildrunner.yaml @@ -0,0 +1,26 @@ +# Description: This example demonstrates how to archive artifacts from a step +# using the artifacts key, including a plain file and an uncompressed +# directory, and how to access artifacts from a previous step in a +# subsequent step using the depends keyword. +version: 2.0 +steps: + make-artifacts: + run: + image: alpine:latest + cmds: + - echo "Hello from make-artifacts" > hello.txt + - mkdir -p outdir + - echo "Hello from outdir" > outdir/nested.txt + artifacts: + hello.txt: + my-property: some-value + outdir: + format: uncompressed + use-artifacts: + depends: + - make-artifacts + run: + image: alpine:latest + cmds: + - cat /artifacts/make-artifacts/hello.txt + - cat /artifacts/make-artifacts/outdir/nested.txt diff --git a/pyproject.toml b/pyproject.toml index b244ea82..21f53297 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "uv_build" [project] name = "buildrunner" -version = "3.26" +version = "3.27" description = "Docker-based build tool" readme = "README.rst" requires-python = ">=3.9" diff --git a/tests/test_docker_runner_run_and_capture.py b/tests/test_docker_runner_run_and_capture.py new file mode 100644 index 00000000..c30d141c --- /dev/null +++ b/tests/test_docker_runner_run_and_capture.py @@ -0,0 +1,149 @@ +""" +Copyright 2026 Adobe +All Rights Reserved. + +NOTICE: Adobe permits you to use, modify, and distribute this file in accordance +with the terms of the Adobe license agreement accompanying it. +""" + +from unittest import mock + +import pytest + +from buildrunner.docker import BuildRunnerContainerError +from buildrunner.docker.runner import DockerRunner + + +@pytest.fixture(name="runner") +def fixture_runner(): + """ + A DockerRunner with __init__ skipped, since it otherwise requires a live + docker daemon connection. Only the attributes used by run_and_capture() + are populated. + """ + runner = object.__new__(DockerRunner) + runner.container = {"Id": "container123"} + runner.shell = "/bin/sh" + runner.run_log_debug = False + runner.docker_client = mock.MagicMock() + runner.docker_client.exec_create.return_value = {"Id": "exec123"} + return runner + + +def test_run_and_capture_returns_exit_code_and_decoded_stdout(runner): + """ + Regression test for XENG-10989: artifact-listing commands must be able + to get their output back directly from the exec call, without writing + to a file that a container-root process creates and a host process must + later remove. + """ + runner.docker_client.exec_start.return_value = (b"line1\nline2\n", b"") + runner.docker_client.exec_inspect.return_value = {"ExitCode": 0} + + exit_code, output = runner.run_and_capture("stat -c '%n' /some/path") + + assert exit_code == 0 + assert output == "line1\nline2\n" + + # must not stream (would return a generator, not the full buffer) and + # must demux so stdout doesn't contain the `-x` shell trace + _, kwargs = runner.docker_client.exec_start.call_args + assert kwargs["stream"] is False + assert kwargs["demux"] is True + + +def test_run_and_capture_no_stdout_returns_empty_string(runner): + runner.docker_client.exec_start.return_value = (None, None) + runner.docker_client.exec_inspect.return_value = {"ExitCode": 1} + + exit_code, output = runner.run_and_capture("false") + + assert exit_code == 1 + assert output == "" + + +def test_run_and_capture_passes_iterable_command_through(runner): + """ + A non-string (list/tuple) command must be handed to exec verbatim rather + than being wrapped in the shell. run() relies on this for its cp/tar/chown + commands, and both now share _build_exec_cmdv. + """ + runner.docker_client.exec_start.return_value = (b"", b"") + runner.docker_client.exec_inspect.return_value = {"ExitCode": 0} + + runner.run_and_capture(["find", "/some/dir", "-type", "f"]) + + exec_args, _ = runner.docker_client.exec_create.call_args + # second positional arg to exec_create is the command vector, passed + # through unwrapped (not [shell, "-xc", ...]) + assert exec_args[1] == ["find", "/some/dir", "-type", "f"] + + +def test_run_and_capture_raises_when_exit_code_is_none(runner): + runner.docker_client.exec_start.return_value = (b"", b"") + runner.docker_client.exec_inspect.return_value = {"ExitCode": None} + + with pytest.raises(BuildRunnerContainerError): + runner.run_and_capture("stat /some/path") + + +def test_run_and_capture_logs_stderr(runner): + log = mock.MagicMock() + log.clean_output.side_effect = lambda output: ( + output if isinstance(output, str) else output.decode("utf-8", errors="replace") + ) + runner.docker_client.exec_start.return_value = (b"out", b"+ stat ...\n") + runner.docker_client.exec_inspect.return_value = {"ExitCode": 0} + + runner.run_and_capture("stat -c '%n' /some/path", log=log) + + assert log.info.called + + +def test_run_and_capture_requires_started_container(): + runner = object.__new__(DockerRunner) + runner.container = None + runner.shell = "/bin/sh" + + with pytest.raises(BuildRunnerContainerError): + runner.run_and_capture("stat /some/path") + + +def test_run_and_capture_raises_on_missing_exit_code(runner): + runner.docker_client.exec_start.return_value = (b"", b"") + runner.docker_client.exec_inspect.return_value = {} + + with pytest.raises(BuildRunnerContainerError): + runner.run_and_capture("stat /some/path") + + +def test_run_still_delegates_to_build_exec_cmdv_and_returns_exit_code(runner): + """ + run() was refactored to share command building with run_and_capture via + _build_exec_cmdv; verify it still wraps the command and returns the exit + code unchanged. + """ + runner.docker_client.exec_start.return_value = b"some output\n" + runner.docker_client.exec_inspect.return_value = {"ExitCode": 7} + + exit_code = runner.run("echo hi") + + assert exit_code == 7 + exec_args, _ = runner.docker_client.exec_create.call_args + assert exec_args[1] == ["/bin/sh", "-xc", "echo hi"] + + +def test_build_exec_cmdv_wraps_string_in_shell(runner): + assert runner._build_exec_cmdv("stat /x") == ["/bin/sh", "-xc", "stat /x"] + + +def test_build_exec_cmdv_raises_type_error_on_unknown_command(runner): + with pytest.raises(TypeError): + runner._build_exec_cmdv(12345) + + +def test_build_exec_cmdv_raises_when_shell_not_set(runner): + runner.shell = None + + with pytest.raises(BuildRunnerContainerError): + runner._build_exec_cmdv(["find", "/x"]) diff --git a/tests/test_run_task_artifacts.py b/tests/test_run_task_artifacts.py new file mode 100644 index 00000000..427e7adf --- /dev/null +++ b/tests/test_run_task_artifacts.py @@ -0,0 +1,161 @@ +""" +Copyright 2026 Adobe +All Rights Reserved. + +NOTICE: Adobe permits you to use, modify, and distribute this file in accordance +with the terms of the Adobe license agreement accompanying it. +""" + +from unittest import mock + +import pytest + +from buildrunner import BuildRunnerConfig +from buildrunner.steprunner.tasks.run import RunBuildStepRunnerTask + + +@pytest.fixture(name="initialize_config", autouse=True) +def fixture_initialize_config(tmp_path): + buildrunner_path = tmp_path / "buildrunner.yaml" + buildrunner_path.write_text("steps: {'step1': {}}") + BuildRunnerConfig.initialize_instance( + build_id="123", + vcs=None, + build_dir=str(tmp_path), + global_config_file=None, + run_config_file=str(buildrunner_path), + build_time=0, + build_number=1, + push=False, + steps_to_run=None, + log_generated_files=False, + global_config_overrides={}, + platform=None, + ) + + +@pytest.fixture(name="task") +def fixture_task(): + """ + A RunBuildStepRunnerTask with __init__ skipped, since it otherwise + requires a live docker daemon connection. Only the attributes used by + _retrieve_artifacts()/_archive_dir() are populated. + """ + task = object.__new__(RunBuildStepRunnerTask) + task._docker_client = mock.MagicMock() + task.step = mock.MagicMock() + task.step_runner = mock.MagicMock() + task.step_runner.results_dir = "/local/results" + task.step_runner.network_name = None + task._get_source_container = mock.MagicMock(return_value="source-container-id") + return task + + +def _mock_docker_runner_class(run_and_capture_result, run_exit_code=0): + mock_class = mock.MagicMock() + lister = mock_class.return_value + lister.container = {"Id": "lister123"} + lister.run.return_value = run_exit_code + lister.run_and_capture.return_value = run_and_capture_result + return mock_class + + +def test_retrieve_artifacts_file_uses_run_and_capture_without_temp_file(task): + """ + Regression test for XENG-10989: the artifact-listing `stat` command must + get its output back directly from run_and_capture, rather than writing + to a temp file under results_dir that a container-root process creates + and a host process must later remove (which can fail with + PermissionError under parallel builds). + """ + task.step.artifacts = {"some/*.txt": None} + mock_class = _mock_docker_runner_class( + (0, "some/file.txt~!~regular file\n"), + ) + + with mock.patch("buildrunner.steprunner.tasks.run.DockerRunner", mock_class): + with mock.patch.object( + RunBuildStepRunnerTask, "_archive_file" + ) as mock_archive_file: + task._retrieve_artifacts() + + lister = mock_class.return_value + + # the stat command must not redirect its output into /stepresults + stat_cmd = lister.run_and_capture.call_args_list[0].args[0] + assert ">" not in stat_cmd + assert "/stepresults/" not in stat_cmd + + # the parsed output correctly identified the file and archived it + assert mock_archive_file.called + archive_args = mock_archive_file.call_args.args + assert archive_args[4] == "some/file.txt" # artifact_file + + # no run() call should be removing a temp .out file + for call in lister.run.call_args_list: + cmd = call.args[0] + if isinstance(cmd, str): + assert ".out" not in cmd + + +def test_retrieve_artifacts_directory_dispatches_to_archive_dir(task): + task.step.artifacts = {"some/dir": None} + mock_class = _mock_docker_runner_class( + (0, "some/dir~!~directory\n"), + ) + + with mock.patch("buildrunner.steprunner.tasks.run.DockerRunner", mock_class): + with mock.patch.object( + RunBuildStepRunnerTask, "_archive_dir" + ) as mock_archive_dir: + task._retrieve_artifacts() + + assert mock_archive_dir.called + _, _, artifact_file = mock_archive_dir.call_args.args + assert artifact_file == "some/dir" + + +def test_retrieve_artifacts_no_matches_archives_nothing(task): + task.step.artifacts = {"some/*.txt": None} + mock_class = _mock_docker_runner_class((1, "")) + + with mock.patch("buildrunner.steprunner.tasks.run.DockerRunner", mock_class): + with mock.patch.object( + RunBuildStepRunnerTask, "_archive_file" + ) as mock_archive_file: + task._retrieve_artifacts() + + assert not mock_archive_file.called + + +def test_archive_dir_uncompressed_uses_run_and_capture_without_temp_file(task): + """ + Regression test for XENG-10989 (second call site): the recursive `find` + listing used for uncompressed directory artifacts must also come back + directly from run_and_capture instead of a temp file under results_dir. + """ + artifact_lister = mock.MagicMock() + artifact_lister.run_and_capture.return_value = ( + 0, + "some/dir/a.txt\nsome/dir/b.txt\n", + ) + artifact_lister.run.return_value = 0 + + with mock.patch.object( + RunBuildStepRunnerTask, "_archive_file" + ) as mock_archive_file: + task._archive_dir( + artifact_lister, + {"format": "uncompressed"}, + "some/dir", + ) + + find_cmd = artifact_lister.run_and_capture.call_args.args[0] + assert ">" not in find_cmd + assert "/stepresults/" not in find_cmd + assert mock_archive_file.call_count == 2 + + for call in artifact_lister.run.call_args_list: + cmd = call.args[0] + if isinstance(cmd, str): + assert ".out" not in cmd diff --git a/uv.lock b/uv.lock index 855e7d09..c1dc3f73 100644 --- a/uv.lock +++ b/uv.lock @@ -105,7 +105,7 @@ wheels = [ [[package]] name = "buildrunner" -version = "3.26" +version = "3.27" source = { editable = "." } dependencies = [ { name = "bcrypt" },