diff --git a/cli/localci/core/patch_pipeline.py b/cli/localci/core/patch_pipeline.py index 7249f29..886b296 100644 --- a/cli/localci/core/patch_pipeline.py +++ b/cli/localci/core/patch_pipeline.py @@ -2,12 +2,15 @@ from __future__ import annotations +import logging from abc import ABC, abstractmethod from dataclasses import dataclass from typing import TYPE_CHECKING from localci.core.config import PATCH_STEP_NAMES, LocalCIConfig +logger = logging.getLogger(__name__) + if TYPE_CHECKING: from localci.core.workflow import MatrixEntry @@ -22,7 +25,15 @@ def name(self) -> str: @abstractmethod def apply(self, ctx: PatchContext) -> None: - """Apply this patch in place to ``ctx.lines``.""" + """Apply this patch in place to ``ctx.lines``. + + Early exits that do not modify ``ctx.lines`` must call + :meth:`_skip` with an actionable reason. + """ + + def _skip(self, reason: str) -> None: + """Log a warning when this step exits without modifying the workflow.""" + logger.warning("Patch step %s skipped: %s", self.name, reason) @dataclass diff --git a/cli/localci/core/patch_steps.py b/cli/localci/core/patch_steps.py index 0346524..c8dd2e3 100644 --- a/cli/localci/core/patch_steps.py +++ b/cli/localci/core/patch_steps.py @@ -41,6 +41,7 @@ def name(self) -> str: def apply(self, ctx: PatchContext) -> None: if not ctx.job_id or not ctx.container_mount_options: + self._skip("job_id and container_mount_options are required") return job_header = re.compile(r"^\s{2}" + re.escape(ctx.job_id) + r"\s*:\s*$") for i, line in enumerate(ctx.lines): @@ -69,8 +70,11 @@ def apply(self, ctx: PatchContext) -> None: j + 1, f'{options_indent}options: "{ctx.container_mount_options}"\n', ) - break - break + return + self._skip(f"job {ctx.job_id!r} has no container block") + return + self._skip(f"job {ctx.job_id!r} not found in workflow") + return class B2SourceCacheStep(PatchStep): @@ -100,7 +104,9 @@ def apply(self, ctx: PatchContext) -> None: f"{ind} fi\n" f"{ind}fi\n" ) - break + return + self._skip("no cp -rL boost-source boost-root line found") + return class RestoreCapyTimestampsStep(PatchStep): @@ -118,24 +124,28 @@ def apply(self, ctx: PatchContext) -> None: already_patched = any( "capy-file-stats" in ctx.lines[j] for j in range(max(0, i - 15), i) ) - if not already_patched: - list_indent = step_match.group(1) - prop_indent = list_indent + " " - body_indent = prop_indent + " " - new_step = [ - f"{list_indent}- name: Restore capy source file timestamps\n", - f"{prop_indent}run: |\n", - f'{body_indent}if [ -n "${{LOCALCI_B2_SOURCE_DIR:-}}" ] && [ -f "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats" ]; then\n', - f"{body_indent} while IFS=' ' read -r saved_mtime fhash relpath; do\n", - f'{body_indent} [ -f "capy-root/$relpath" ] || continue\n', - f"{body_indent} curr=$(sha256sum \"capy-root/$relpath\" 2>/dev/null | cut -d' ' -f1)\n", - f'{body_indent} [ "$curr" = "$fhash" ] && touch -d "@$saved_mtime" "capy-root/$relpath" 2>/dev/null || true\n', - f'{body_indent} done < "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats"\n', - f"{body_indent}fi\n", - ] - for j, new_line in enumerate(new_step): - ctx.lines.insert(i + j, new_line) - break + if already_patched: + self._skip("restore step already present") + return + list_indent = step_match.group(1) + prop_indent = list_indent + " " + body_indent = prop_indent + " " + new_step = [ + f"{list_indent}- name: Restore capy source file timestamps\n", + f"{prop_indent}run: |\n", + f'{body_indent}if [ -n "${{LOCALCI_B2_SOURCE_DIR:-}}" ] && [ -f "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats" ]; then\n', + f"{body_indent} while IFS=' ' read -r saved_mtime fhash relpath; do\n", + f'{body_indent} [ -f "capy-root/$relpath" ] || continue\n', + f"{body_indent} curr=$(sha256sum \"capy-root/$relpath\" 2>/dev/null | cut -d' ' -f1)\n", + f'{body_indent} [ "$curr" = "$fhash" ] && touch -d "@$saved_mtime" "capy-root/$relpath" 2>/dev/null || true\n', + f'{body_indent} done < "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats"\n', + f"{body_indent}fi\n", + ] + for j, new_line in enumerate(new_step): + ctx.lines.insert(i + j, new_line) + return + self._skip("Patch Boost step not found") + return class CapyCopyPreservationStep(PatchStep): @@ -161,7 +171,9 @@ def apply(self, ctx: PatchContext) -> None: f'{ind} done > "${{LOCALCI_B2_SOURCE_DIR}}/.capy-file-stats"\n' f"{ind}fi\n" ) - break + return + self._skip('no cp -r "$workspace_root" capy copy line found') + return class B2BootstrapSkipStep(PatchStep): @@ -183,25 +195,34 @@ def apply(self, ctx: PatchContext) -> None: "Skip b2 bootstrap" in ctx.lines[j] for j in range(max(0, step_start - 15), step_start) ) - if not already_patched: - list_indent, prop_indent, body_indent = _step_insert_indents( - ctx.lines, step_start - ) - new_step = [ - f"{list_indent}- name: Skip b2 bootstrap (b2 binary cached)\n", - f"{prop_indent}run: |\n", - f'{body_indent}if [ -n "${{LOCALCI_B2_SOURCE_DIR:-}}" ] && [ -f "${{LOCALCI_B2_SOURCE_DIR}}/b2" ]; then\n', - f"{body_indent} printf '#!/bin/sh\\necho \"b2 binary cached, skipping bootstrap.\"\\n' > boost-root/bootstrap.sh\n", - f"{body_indent} chmod +x boost-root/bootstrap.sh\n", - f"{body_indent}fi\n", - ] - for j, new_line in enumerate(new_step): - ctx.lines.insert(step_start + j, new_line) - break + if already_patched: + self._skip("skip b2 bootstrap step already present") + return + list_indent, prop_indent, body_indent = _step_insert_indents( + ctx.lines, step_start + ) + new_step = [ + f"{list_indent}- name: Skip b2 bootstrap (b2 binary cached)\n", + f"{prop_indent}run: |\n", + f'{body_indent}if [ -n "${{LOCALCI_B2_SOURCE_DIR:-}}" ] && [ -f "${{LOCALCI_B2_SOURCE_DIR}}/b2" ]; then\n', + f"{body_indent} printf '#!/bin/sh\\necho \"b2 binary cached, skipping bootstrap.\"\\n' > boost-root/bootstrap.sh\n", + f"{body_indent} chmod +x boost-root/bootstrap.sh\n", + f"{body_indent}fi\n", + ] + for j, new_line in enumerate(new_step): + ctx.lines.insert(step_start + j, new_line) + return + self._skip("no b2-workflow uses step found") + return class ImageSubstitutionStep(PatchStep): - """Replace matrix container image with the locally-built image tag.""" + """Replace matrix container image with the locally-built image tag. + + Raises ValueError if the matrix entry name is not found (unexpected workflow + structure). Skips with a warning if the container field is absent from the + entry block (valid but unsupported layout). + """ @property def name(self) -> str: @@ -209,6 +230,7 @@ def name(self) -> str: def apply(self, ctx: PatchContext) -> None: if not ctx.image_tag: + self._skip("image_tag not provided") return name_escaped = re.escape(ctx.entry.name) name_pattern = re.compile(r'name:\s*["\']?' + name_escaped + r'["\']?\s*$') @@ -255,7 +277,9 @@ def apply(self, ctx: PatchContext) -> None: mo = container_pattern.match(ctx.lines[i]) if mo: ctx.lines[i] = f'{mo.group(1)}container: "{ctx.image_tag}"\n' - break + return + self._skip("container field not found in matrix entry block") + return class CodecovSkipStep(PatchStep): @@ -280,7 +304,9 @@ def apply(self, ctx: PatchContext) -> None: f"{indent}{act_check}{rest}; " f'else echo "Skipping Codecov upload (running under act)."; fi\n' ) - break + return + self._skip("no Codecov upload step found") + return PATCH_STEP_REGISTRY: dict[str, type[PatchStep]] = { diff --git a/cli/tests/fixtures/patcher/container_image_no_container.yml b/cli/tests/fixtures/patcher/container_image_no_container.yml new file mode 100644 index 0000000..2877fcd --- /dev/null +++ b/cli/tests/fixtures/patcher/container_image_no_container.yml @@ -0,0 +1,13 @@ +name: Container Image No Container Field +on: + push: +jobs: + build: + strategy: + matrix: + include: + - name: "GCC 15: C++20" + runs-on: ubuntu-latest + runs-on: ${{ matrix.runs-on }} + steps: + - run: echo build diff --git a/cli/tests/fixtures/patcher/container_mount_no_container_block.yml b/cli/tests/fixtures/patcher/container_mount_no_container_block.yml new file mode 100644 index 0000000..4d00cbb --- /dev/null +++ b/cli/tests/fixtures/patcher/container_mount_no_container_block.yml @@ -0,0 +1,8 @@ +name: Container Mount No Container Block +on: + push: +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo build diff --git a/cli/tests/test_patch_pipeline.py b/cli/tests/test_patch_pipeline.py index 50de1d3..f45d0ed 100644 --- a/cli/tests/test_patch_pipeline.py +++ b/cli/tests/test_patch_pipeline.py @@ -2,13 +2,15 @@ from __future__ import annotations +import logging from pathlib import Path import pytest from localci.cli.run.patcher import _write_patched_workflow from localci.core.config import LocalCIConfig, PatchesConfig -from localci.core.patch_pipeline import PatchPipeline +from localci.core.patch_pipeline import PatchContext, PatchPipeline +from localci.core.patch_steps import ContainerMountsStep from localci.core.workflow import ( BuildSystem, BuildVariant, @@ -192,3 +194,23 @@ def test_patches_config_rejects_enabled_step_missing_from_order() -> None: "image_substitution", ], ) + + +def test_patch_step_skip_emits_warning(sample_entry: MatrixEntry, caplog) -> None: + """Enabled patch step with incomplete context logs a skip warning.""" + ctx = PatchContext( + lines=["jobs:\n", " build:\n", " runs-on: ubuntu-latest\n"], + entry=sample_entry, + config=LocalCIConfig(), + job_id=None, + container_mount_options=None, + ) + with caplog.at_level(logging.WARNING, logger="localci.core.patch_pipeline"): + ContainerMountsStep().apply(ctx) + + assert any( + r.levelno == logging.WARNING + and "container_mounts" in r.message + and "job_id and container_mount_options are required" in r.message + for r in caplog.records + ) diff --git a/cli/tests/test_patcher_patches.py b/cli/tests/test_patcher_patches.py index eda0ec6..8b126ec 100644 --- a/cli/tests/test_patcher_patches.py +++ b/cli/tests/test_patcher_patches.py @@ -5,12 +5,14 @@ from __future__ import annotations +import logging from pathlib import Path import pytest import yaml from localci.cli.run import _write_patched_workflow +from localci.core.config import LocalCIConfig, PatchesConfig from localci.core.workflow import ( BuildSystem, BuildVariant, @@ -23,6 +25,75 @@ ) FIXTURES_DIR = Path(__file__).parent / "fixtures" / "patcher" +PATCH_LOGGER = "localci.core.patch_pipeline" + + +def _assert_skip_warning(caplog, step_name: str, reason_fragment: str) -> None: + assert any( + r.levelno == logging.WARNING + and step_name in r.message + and reason_fragment in r.message + for r in caplog.records + ), ( + f"expected WARNING for {step_name!r} containing {reason_fragment!r}, " + f"got: {[(r.levelname, r.message) for r in caplog.records]}" + ) + + +def _container_mounts_only_config() -> LocalCIConfig: + return LocalCIConfig( + patches=PatchesConfig( + container_mounts=True, + b2_source_cache=False, + restore_capy_timestamps=False, + capy_copy_preservation=False, + b2_bootstrap_skip=False, + image_substitution=False, + codecov_skip=False, + ) + ) + + +def _restore_capy_only_config() -> LocalCIConfig: + return LocalCIConfig( + patches=PatchesConfig( + container_mounts=False, + b2_source_cache=False, + restore_capy_timestamps=True, + capy_copy_preservation=False, + b2_bootstrap_skip=False, + image_substitution=False, + codecov_skip=False, + ) + ) + + +def _image_substitution_only_config() -> LocalCIConfig: + return LocalCIConfig( + patches=PatchesConfig( + container_mounts=False, + b2_source_cache=False, + restore_capy_timestamps=False, + capy_copy_preservation=False, + b2_bootstrap_skip=False, + image_substitution=True, + codecov_skip=False, + ) + ) + + +def _b2_source_cache_only_config() -> LocalCIConfig: + return LocalCIConfig( + patches=PatchesConfig( + container_mounts=False, + b2_source_cache=True, + restore_capy_timestamps=False, + capy_copy_preservation=False, + b2_bootstrap_skip=False, + image_substitution=False, + codecov_skip=False, + ) + ) def _make_entry(name: str = "GCC 15: C++20") -> MatrixEntry: @@ -104,12 +175,33 @@ def test_negative_raises_when_matrix_entry_name_missing(self, patcher_paths): image_tag="localci/missing:latest", ) - def test_negative_skips_when_image_tag_not_provided(self, patcher_paths): + def test_negative_skips_when_image_tag_not_provided(self, patcher_paths, caplog): workflow = FIXTURES_DIR / "container_image.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert 'container: "ubuntu:25.04"' in content assert "localci/" not in content + _assert_skip_warning(caplog, "image_substitution", "image_tag not provided") + + def test_negative_skips_when_matrix_entry_has_no_container_field( + self, patcher_paths, caplog + ): + workflow = FIXTURES_DIR / "container_image_no_container.yml" + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths( + workflow, + _make_entry("GCC 15: C++20"), + image_tag="localci/gcc-15:custom", + config=_image_substitution_only_config(), + ) + content = _assert_valid_yaml(patched) + assert "localci/gcc-15:custom" not in content + _assert_skip_warning( + caplog, + "image_substitution", + "container field not found in matrix entry block", + ) # --------------------------------------------------------------------------- @@ -142,22 +234,68 @@ def test_positive_injects_mount_options( if has_existing_options: assert "--privileged" in content - def test_negative_skips_without_job_id(self, patcher_paths): + def test_negative_skips_without_job_id(self, patcher_paths, caplog): workflow = FIXTURES_DIR / "container_mount.yml" original = workflow.read_text(encoding="utf-8") - patched = patcher_paths( - workflow, - container_mount_options="-v /host/boost:/cache", + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths( + workflow, + container_mount_options="-v /host/boost:/cache", + ) + content = _assert_valid_yaml(patched) + assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, + "container_mounts", + "job_id and container_mount_options are required", ) + + def test_negative_skips_without_mount_options(self, patcher_paths, caplog): + workflow = FIXTURES_DIR / "container_mount.yml" + original = workflow.read_text(encoding="utf-8") + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow, job_id="build") content = _assert_valid_yaml(patched) assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, + "container_mounts", + "job_id and container_mount_options are required", + ) - def test_negative_skips_without_mount_options(self, patcher_paths): + def test_negative_skips_when_job_has_no_container_block( + self, patcher_paths, caplog + ): + workflow = FIXTURES_DIR / "container_mount_no_container_block.yml" + original = workflow.read_text(encoding="utf-8") + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths( + workflow, + job_id="build", + container_mount_options="-v /host/boost:/cache", + config=_container_mounts_only_config(), + ) + content = _assert_valid_yaml(patched) + assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, "container_mounts", "job 'build' has no container block" + ) + + def test_negative_skips_when_job_not_found(self, patcher_paths, caplog): workflow = FIXTURES_DIR / "container_mount.yml" original = workflow.read_text(encoding="utf-8") - patched = patcher_paths(workflow, job_id="build") + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths( + workflow, + job_id="missing_job", + container_mount_options="-v /host/boost:/cache", + config=_container_mounts_only_config(), + ) content = _assert_valid_yaml(patched) assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, "container_mounts", "job 'missing_job' not found in workflow" + ) # --------------------------------------------------------------------------- @@ -177,15 +315,19 @@ def test_positive_replaces_cp_with_cache_logic(self, patcher_paths): assert "cp -a boost-root/." in content def test_negative_leaves_workflow_unchanged_without_boost_copy_line( - self, patcher_paths + self, patcher_paths, caplog ): workflow = FIXTURES_DIR / "container_image.yml" original = workflow.read_text(encoding="utf-8") - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow, config=_b2_source_cache_only_config()) content = _assert_valid_yaml(patched) assert "LOCALCI_B2_SOURCE_DIR" not in content assert "cp -rL boost-source boost-root" not in content assert content == original.replace("\r\n", "\n") + _assert_skip_warning( + caplog, "b2_source_cache", "no cp -rL boost-source boost-root line found" + ) # --------------------------------------------------------------------------- @@ -205,11 +347,27 @@ def test_positive_injects_restore_step_before_patch_boost(self, patcher_paths): "Patch Boost" ) - def test_negative_skips_duplicate_on_already_patched_workflow(self, patcher_paths): + def test_negative_skips_duplicate_on_already_patched_workflow( + self, patcher_paths, caplog + ): workflow = FIXTURES_DIR / "already_patched.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert content.count("Restore capy source file timestamps") == 1 + _assert_skip_warning( + caplog, "restore_capy_timestamps", "restore step already present" + ) + + def test_negative_skips_when_patch_boost_step_absent(self, patcher_paths, caplog): + workflow = FIXTURES_DIR / "container_image.yml" + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow, config=_restore_capy_only_config()) + content = _assert_valid_yaml(patched) + assert "Restore capy source file timestamps" not in content + _assert_skip_warning( + caplog, "restore_capy_timestamps", "Patch Boost step not found" + ) # --------------------------------------------------------------------------- @@ -228,11 +386,21 @@ def test_positive_replaces_cp_r_with_cp_rp_and_stats(self, patcher_paths): assert "sha256sum" in content assert 'cp -r "$workspace_root"' not in content - def test_negative_no_cp_rp_injected_when_capy_copy_line_absent(self, patcher_paths): + def test_negative_no_cp_rp_injected_when_capy_copy_line_absent( + self, patcher_paths, caplog + ): + # Full default pipeline on boost_cache.yml skips several steps; we assert + # only the capy_copy_preservation warning for this fixture. workflow = FIXTURES_DIR / "boost_cache.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert 'cp -rp "$workspace_root"' not in content + _assert_skip_warning( + caplog, + "capy_copy_preservation", + 'no cp -r "$workspace_root" capy copy line found', + ) # --------------------------------------------------------------------------- @@ -250,11 +418,15 @@ def test_positive_injects_skip_step_before_b2_workflow(self, patcher_paths): assert "bootstrap.sh" in content assert content.index("Skip b2 bootstrap") < content.index("b2-workflow") - def test_negative_skips_when_b2_workflow_step_absent(self, patcher_paths): + def test_negative_skips_when_b2_workflow_step_absent(self, patcher_paths, caplog): workflow = FIXTURES_DIR / "boost_cache.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert "Skip b2 bootstrap (b2 binary cached)" not in content + _assert_skip_warning( + caplog, "b2_bootstrap_skip", "no b2-workflow uses step found" + ) # --------------------------------------------------------------------------- @@ -272,13 +444,17 @@ def test_positive_wraps_upload_with_act_guard(self, patcher_paths): assert "Skipping Codecov upload (running under act)" in content assert "https://codecov.io/bash" in content - def test_negative_leaves_workflow_without_codecov_unchanged(self, patcher_paths): + def test_negative_leaves_workflow_without_codecov_unchanged( + self, patcher_paths, caplog + ): workflow = FIXTURES_DIR / "boost_cache.yml" - patched = patcher_paths(workflow) + with caplog.at_level(logging.WARNING, logger=PATCH_LOGGER): + patched = patcher_paths(workflow) content = _assert_valid_yaml(patched) assert "codecov.io" not in content assert "ACT" not in content assert "LOCALCI_B2_SOURCE_DIR" in content + _assert_skip_warning(caplog, "codecov_skip", "no Codecov upload step found") # ---------------------------------------------------------------------------