From fe7c95e506049019f8f307cd73dc9fb4067142b4 Mon Sep 17 00:00:00 2001 From: Claudio Matsuoka Date: Wed, 2 Aug 2023 17:57:14 -0300 Subject: [PATCH] sequencer: only load project vars in adopting part Project variables exist only in the part scope and should be ignored in other parts' states. In the existing implementation these values are persisted in each part, which leads to double assignment errors when a step is executed for parts in a order that's different from the previous execution (see example in LP #1831135). There are different ways to address this issue, the current fix tries to be minimally disruptive by just ignoring persisted values when reloading the state. We can refactor this code later to have a cleaner implementation. Signed-off-by: Claudio Matsuoka --- craft_parts/infos.py | 5 + craft_parts/sequencer.py | 9 +- tests/integration/lifecycle/test_craftctl.py | 113 ++++++++++++++++++- tests/unit/test_infos.py | 1 + 4 files changed, 126 insertions(+), 2 deletions(-) diff --git a/craft_parts/infos.py b/craft_parts/infos.py index aba73359a..ef2523bd1 100644 --- a/craft_parts/infos.py +++ b/craft_parts/infos.py @@ -173,6 +173,11 @@ def project_name(self) -> Optional[str]: """Return the name of the project using craft-parts.""" return self._project_name + @property + def project_vars_part_name(self) -> Optional[str]: + """Return the name of the part that can set project vars.""" + return self._project_vars_part_name + @property def project_options(self) -> Dict[str, Any]: """Obtain a project-wide options dictionary.""" diff --git a/craft_parts/sequencer.py b/craft_parts/sequencer.py index ab6c75d81..66a396d9c 100644 --- a/craft_parts/sequencer.py +++ b/craft_parts/sequencer.py @@ -200,9 +200,16 @@ def _add_step_actions( current_step, action_type=ActionType.SKIP, reason="already ran", - project_vars=self._sm.project_vars(part, current_step), + project_vars=self._get_project_vars(part, current_step), ) + def _get_project_vars( + self, part: Part, step: Step + ) -> Optional[Dict[str, ProjectVar]]: + if part.name == self._project_info.project_vars_part_name: + return self._sm.project_vars(part, step) + return None + def _process_dependencies(self, part: Part, step: Step) -> None: prerequisite_step = steps.dependency_prerequisite_step(step) if not prerequisite_step: diff --git a/tests/integration/lifecycle/test_craftctl.py b/tests/integration/lifecycle/test_craftctl.py index 58f517b29..7649de5f5 100644 --- a/tests/integration/lifecycle/test_craftctl.py +++ b/tests/integration/lifecycle/test_craftctl.py @@ -23,7 +23,7 @@ import yaml import craft_parts -from craft_parts import Action, Step, errors +from craft_parts import Action, ActionType, Step, errors from tests import TESTS_DIR @@ -356,6 +356,117 @@ def test_craftctl_set_error(new_dir, capfd, mocker): assert "'myvar' not in project variables" in captured.err +def test_craftctl_set_only_once(new_dir, capfd, mocker): # see LP #1831135 + parts_yaml = textwrap.dedent( + """\ + parts: + part1: + plugin: nil + override-pull: | + craftctl default + craftctl set version=xx + + part2: + plugin: nil + after: [part1] + """ + ) + parts = yaml.safe_load(parts_yaml) + + lf = craft_parts.LifecycleManager( + parts, + application_name="test_set", + cache_dir=new_dir, + project_vars_part_name="part1", + project_vars={"version": ""}, + ) + + assert lf.project_info.get_project_var("version", raw_read=True) == "" + + actions = lf.plan(Step.BUILD) + with lf.action_executor() as ctx: + ctx.execute(actions) + + assert lf.project_info.get_project_var("version") == "xx" + + # change something in part1 to make it dirty + parts["parts"]["part1"]["override-pull"] += "\necho foo" + + # now build only part2 so that pull order will be reversed + lf = craft_parts.LifecycleManager( + parts, + application_name="test_set", + cache_dir=new_dir, + project_vars_part_name="part1", + project_vars={"version": ""}, + ) + + assert lf.project_info.get_project_var("version", raw_read=True) == "" + + # execution of actions must succeed + actions = lf.plan(Step.BUILD, part_names=["part2"]) + with lf.action_executor() as ctx: + ctx.execute(actions) + + assert lf.project_info.get_project_var("version") == "xx" + + +def test_craftctl_update_project_vars(new_dir, capfd, mocker): + parts_yaml = textwrap.dedent( + """\ + parts: + part1: + plugin: nil + override-pull: | + craftctl default + craftctl set version=xx + + part2: + plugin: nil + after: [part1] + """ + ) + parts = yaml.safe_load(parts_yaml) + + lf = craft_parts.LifecycleManager( + parts, + application_name="test_set", + cache_dir=new_dir, + project_vars_part_name="part1", + project_vars={"version": ""}, + ) + + assert lf.project_info.get_project_var("version", raw_read=True) == "" + + actions = lf.plan(Step.BUILD) + with lf.action_executor() as ctx: + ctx.execute(actions) + + assert lf.project_info.get_project_var("version") == "xx" + + # re-execute the lifecycle with no changes + lf = craft_parts.LifecycleManager( + parts, + application_name="test_set", + cache_dir=new_dir, + project_vars_part_name="part1", + project_vars={"version": ""}, + ) + + assert lf.project_info.get_project_var("version", raw_read=True) == "" + + actions = lf.plan(Step.BUILD) + + # all actions should be skipped + for action in actions: + assert action.action_type == ActionType.SKIP + + with lf.action_executor() as ctx: + ctx.execute(actions) + + assert lf.project_info.get_project_var("version") == "xx" + + def test_craftctl_get_error(new_dir, capfd, mocker): parts_yaml = textwrap.dedent( """\ diff --git a/tests/unit/test_infos.py b/tests/unit/test_infos.py index c48697b9d..59246f247 100644 --- a/tests/unit/test_infos.py +++ b/tests/unit/test_infos.py @@ -69,6 +69,7 @@ def test_project_info(mocker, new_dir, tc_arch, tc_target_arch, tc_triplet, tc_c "project_vars_part_name": "adopt", "project_vars": {"a": ProjectVar(value="b")}, } + assert x.project_vars_part_name == "adopt" assert x.global_environment == {} assert x.parts_dir == new_dir / "parts"