diff --git a/rockcraft/oci.py b/rockcraft/oci.py index ae88631f1..d81935362 100644 --- a/rockcraft/oci.py +++ b/rockcraft/oci.py @@ -405,23 +405,26 @@ def set_cmd(self, command: list[str] | None = None) -> None: _config_image(image_path, cmd_params, comment="Set default commands") emit.progress(f"CMD set to {command}") - def set_default_path(self, base: str) -> None: - """Set the default PATH on the image (only for bare rocks).""" - if base != "bare": - emit.debug(f"Not setting a PATH on the image as base is {base!r}") + def set_default_path(self) -> None: + """Ensure the OCI image has a sane PATH when PATH is missing or empty.""" + image_path = self.path / self.image_name + + env = _read_image_env_from_skopeo(image_path) + path_value = _get_env_value(env, "PATH") + + if path_value: + emit.debug("PATH already set on the image; not overriding.") return # Follow Pebble's lead here: if PATH is empty, use the standard one. - # This means that containers that bypass the pebble entrypoint will - # have the same behavior as PATH-less pebble services. + # This means that containers that bypass the Pebble entrypoint will + # have the same behavior as PATH-less Pebble services. pebble_path = Pebble.DEFAULT_ENV_PATH - image_path = self.path / self.image_name - - emit.debug(f"Setting bare-based rock PATH to {pebble_path!r}") + emit.debug(f"Setting default PATH on the image to {pebble_path!r}") _config_image( image_path, ["--config.env", f"PATH={pebble_path}"], - comment="Set default PATH for bare-based rock", + comment="Set default PATH", ) def set_pebble_layer( @@ -578,6 +581,28 @@ def _copy_image( ) +def _read_image_env_from_skopeo(image_path: Path) -> list[str]: + """Read the environment from `skopeo inspect` output.""" + output = _process_run(["skopeo", "inspect", f"oci:{image_path}"]).stdout + data: object = json.loads(output) + + if not isinstance(data, dict): + return [] + + env = data.get("Env") + if not isinstance(env, list): + return [] + + return [entry for entry in env if isinstance(entry, str)] + + +def _get_env_value(env: list[str], key: str) -> str | None: + """Return the value for KEY from env entries like ['K=V', ...]. Last wins.""" + prefix = f"{key}=" + values = [e.split("=", 1)[1] for e in env if e.startswith(prefix)] + return values[-1] if values else None + + def _config_image( image_path: Path, params: list[str], comment: str | None = None ) -> None: diff --git a/rockcraft/services/package.py b/rockcraft/services/package.py index f845355de..db3050991 100644 --- a/rockcraft/services/package.py +++ b/rockcraft/services/package.py @@ -152,7 +152,6 @@ def _pack( new_image.set_entrypoint(entrypoint) new_image.set_cmd(cmd) - new_image.set_default_path(project.base) dumped = project.marshal() services = cast(dict[str, typing.Any], dumped.get("services", {})) @@ -172,6 +171,9 @@ def _pack( if project.environment: new_image.set_environment(project.environment) + # Check the final environment after applying project-defined variables. + new_image.set_default_path() + # Set annotations and metadata, both dynamic and the ones based on user-provided properties # Also include the "created" timestamp, just before packing the image emit.progress("Adding metadata") diff --git a/tests/unit/services/test_package.py b/tests/unit/services/test_package.py index 06eb1cd3b..1e3eccc14 100644 --- a/tests/unit/services/test_package.py +++ b/tests/unit/services/test_package.py @@ -15,6 +15,7 @@ # along with this program. If not, see . from pathlib import Path from typing import cast +from unittest.mock import call import pytest from craft_application import ServiceFactory @@ -69,11 +70,11 @@ def test_pack(fake_services: ServiceFactory, default_image_info, mocker): @pytest.mark.parametrize( ("project_keys", "expected_entrypoint", "expected_cmd"), [ - # Most common scenario + # Project environment explicitly clears PATH. ( { "run_user": "_daemon_", - "environment": {"test": "foo"}, + "environment": {"PATH": ""}, "services": {"test": {"override": "replace", "command": "echo foo"}}, }, ["/usr/bin/pebble", "enter"], @@ -208,7 +209,7 @@ def test_inner_pack( image.set_default_user.assert_called_once_with(584792, project.run_user) image.set_entrypoint.assert_called_once_with(expected_entrypoint) image.set_cmd.assert_called_once_with(expected_cmd) - image.set_default_path.assert_called_once_with(project.base) + image.set_default_path.assert_called_once_with() image.set_pebble_layer.assert_called_once_with( services=project.marshal().get("services", {}), checks=project.marshal().get("checks", {}), @@ -220,6 +221,12 @@ def test_inner_pack( ) image.set_environment.assert_called_once_with(project.environment) image.set_annotations.assert_called_once_with(annotations) + + environment_call = call.set_environment(project.environment) + default_path_call = call.set_default_path() + assert image.method_calls.index(environment_call) < image.method_calls.index( + default_path_call + ) image.set_control_data.assert_called_once_with(metadata) image.set_media_type.assert_called_once_with(arch="amd64") image.to_oci_archive.assert_called_once_with( diff --git a/tests/unit/test_oci.py b/tests/unit/test_oci.py index e9568d36a..92f61ceb6 100644 --- a/tests/unit/test_oci.py +++ b/tests/unit/test_oci.py @@ -1012,40 +1012,80 @@ def test_get_manifest(self, new_dir, mock_inject_oci_fields, mock_run, mocker): ] assert mock_loads.called - def test_set_path_bare(self, mock_run): - image = oci.Image("a:b", Path("/c")) + def _mock_skopeo_inspect_env( + self, + mock_run, + mocker, + *, + env: list[str], + ) -> None: + def _run_side_effect(cmd, **kwargs): + result = mocker.MagicMock() - image.set_default_path("bare") + if cmd[:2] == ["skopeo", "inspect"]: + result.stdout = json.dumps({"Env": env}) + return result - expected_path = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" - expected_cmd = [ - "umoci", - "config", - "--image", - "/c/a:b", - "--config.env", - f"PATH={expected_path}", - ] + if cmd[:2] == ["umoci", "config"] and "--image" in cmd: + return result - assert mock_run.mock_calls == [ - call( - [ - *expected_cmd, - "--history.created_by", - " ".join(expected_cmd), - "--history.comment", - "Set default PATH for bare-based rock", - ] - ) - ] + raise AssertionError(f"Unexpected command: {cmd!r}") + + mock_run.side_effect = _run_side_effect @pytest.mark.parametrize( - "base", - ["ubuntu@24.04", "ubuntu@22.04", "ubuntu@20.04"], + ("env", "should_set_default"), + [ + pytest.param(["PATH=/usr/bin"], False, id="path-present"), + pytest.param(["FOO=bar"], True, id="path-missing"), + pytest.param(["PATH="], True, id="path-empty"), + pytest.param( + ["PATH=/usr/bin", "PATH="], + True, + id="last-path-empty", + ), + pytest.param( + ["PATH=", "PATH=/custom/bin"], + False, + id="last-path-present", + ), + ], ) - def test_set_path_non_bare(self, mock_run, base): + def test_set_default_path( + self, + mock_run, + mocker, + env, + should_set_default, + ): image = oci.Image("a:b", Path("/c")) + self._mock_skopeo_inspect_env(mock_run, mocker, env=env) + + image.set_default_path() - image.set_default_path(base) + expected_calls = [ + call(["skopeo", "inspect", "oci:/c/a:b"]), + ] + + if should_set_default: + config_cmd = [ + "umoci", + "config", + "--image", + "/c/a:b", + "--config.env", + f"PATH={Pebble.DEFAULT_ENV_PATH}", + ] + expected_calls.append( + call( + [ + *config_cmd, + "--history.created_by", + " ".join(config_cmd), + "--history.comment", + "Set default PATH", + ] + ) + ) - assert not mock_run.called + assert mock_run.mock_calls == expected_calls