diff --git a/.github/workflows/analyzers.yml b/.github/workflows/analyzers.yml index 8cec429a5..ddecb0f8e 100644 --- a/.github/workflows/analyzers.yml +++ b/.github/workflows/analyzers.yml @@ -13,20 +13,39 @@ defaults: shell: bash jobs: - tooling: - name: Tooling + script-tests: + name: Analyzer script tests runs-on: ubuntu-24.04 timeout-minutes: 10 steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 with: persist-credentials: false - - name: Check analyzer tooling + - name: Run analyzer script tests run: make lint-test + scs-lint: + name: SCST code style + needs: script-tests + runs-on: ubuntu-24.04 + timeout-minutes: 10 + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + - uses: astral-sh/setup-uv@1c37ad07a6a961277cf70c0d37d6f313000f5884 # v10.3.0 + with: + version: "0.13.1" + python-version: "3.12" + enable-cache: false + - name: Test SCST style rules + run: make scs-lint-test + - name: Run make scs_lint + run: make scs_lint + native: - name: ${{ matrix.analyzer }} with KUnit - needs: tooling + name: ${{ matrix.analyzer }} (includes KUnit sources) + needs: script-tests runs-on: ubuntu-24.04 timeout-minutes: 45 strategy: @@ -35,11 +54,12 @@ jobs: analyzer: - sparse - smatch + - checkpatch # Supported by the runner; enable after reviewing native coverage. # - coccicheck # - cppcheck steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 with: persist-credentials: false - name: Build pinned analyzer environment @@ -51,8 +71,6 @@ jobs: ANALYZER: ${{ matrix.analyzer }} run: | source scripts/ci/kernel.env - # Temporary rollout mode: only completed source findings are advisory. - # Tool, build and coverage failures still fail this job. docker run --rm --init --network=none --cap-drop=ALL \ --security-opt=no-new-privileges --user "$(id -u):$(id -g)" \ --env HOME=/tmp --env GITHUB_STEP_SUMMARY=/scst/.cache/analyzer-summary \ @@ -67,7 +85,7 @@ jobs: fi - name: Save complete analyzer evidence if: always() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v7 with: name: ${{ matrix.analyzer }}-evidence retention-days: 7 diff --git a/.github/workflows/checkpatch_push.yml b/.github/workflows/checkpatch_push.yml deleted file mode 100644 index 7fe56f4e7..000000000 --- a/.github/workflows/checkpatch_push.yml +++ /dev/null @@ -1,32 +0,0 @@ -name: Checkpatch upon push - -on: - push: - branches: - - master - -jobs: - checkpatch: - name: ${{matrix.commit.message}} - runs-on: ubuntu-latest - strategy: - fail-fast: false - matrix: - commit: ${{github.event.commits}} - steps: - - name: Checkout code - uses: actions/checkout@main - with: - ref: ${{matrix.commit.id}} - fetch-depth: 2 - - - name: Download checkpatch.pl - run: | - curl -fsSL --retry 5 https://raw.githubusercontent.com/torvalds/linux/master/scripts/checkpatch.pl -o checkpatch.pl - curl -fsSL --retry 5 https://raw.githubusercontent.com/torvalds/linux/master/scripts/spelling.txt -o spelling.txt - curl -fsSL --retry 5 https://raw.githubusercontent.com/torvalds/linux/master/scripts/const_structs.checkpatch -o const_structs.checkpatch - chmod +x checkpatch.pl - - - name: Run checkpatch.pl - run: | - git format-patch -1 --stdout | CHECKPATCH_PATH=. ./scripts/checkpatch diff --git a/doc/CI.md b/doc/CI.md index 464d5b45b..161cc9568 100644 --- a/doc/CI.md +++ b/doc/CI.md @@ -5,11 +5,11 @@ Workflows live in [`.github/workflows`](../.github/workflows). PR checks target | Workflow | Purpose | Events | | --- | --- | --- | -| Checkpatch ([PR](../.github/workflows/checkpatch_pull.yml), [push](../.github/workflows/checkpatch_push.yml)) | Patch and commit style | PR, push | +| [PR Checkpatch](../.github/workflows/checkpatch_pull.yml) | Individual patch and commit style | PR | | [CI](../.github/workflows/ci.yml) | Clang build and DEB packaging | PR, push, manual | | [Packages](../.github/workflows/rpm.yml) | Ubuntu DEBs, RPMs and a published package snapshot | Push to master only | | [Regression](../.github/workflows/run_regression_tests.yaml) | Module and patched-kernel builds across the kernel matrix | PR, push | -| [Static analyzers](../.github/workflows/analyzers.yml) | Sparse/Smatch with KUnit sources, tooling checks and full reports | Push to master, manual | +| [Static analyzers](../.github/workflows/analyzers.yml) | Sparse, Smatch, whole-file Checkpatch and SCST code style, including KUnit sources; script tests | Push to master, manual | | [KUnit](../.github/workflows/kunit.yml) | Kernel unit tests in QEMU | PR, push, manual | | [Ginkgo](../.github/workflows/ginkgo.yml) | Local and TCP iSCSI component tests in QEMU | PR, push, manual | | [Coverity](../.github/workflows/coverity.yml) | Static analysis | Push, upstream repository only | @@ -21,7 +21,7 @@ Edit that config to change path rules; it is validated before selection. Unknown paths and changes to the config or selector run every check. Known unrelated changes skip expensive work; regression selection happens before creating its matrix jobs. Some documentation is a package or kernel-patch -input and still needs validation. Checkpatch always runs. Packages runs only +input and still needs validation. PR Checkpatch always runs. Packages runs only after a push to master; the separate Clang build still checks DEB packaging on PRs. Push and manual runs retain their full checks. Selection errors fail the workflow; skip reasons appear in the job summary. @@ -53,9 +53,10 @@ kernel diagnostics and incomplete reports fail the run. Package artifacts are attached to the Packages workflow; upstream master builds also update the `master-rpm` prerelease snapshot. -Analyzer jobs retain full reports in CI artifacts. See the -[analyzer guide](../scripts/README.md#analyzer-ci) for reproduction and the -temporary nonblocking policy for source findings. +Analyzer jobs fail on remaining findings and print them in job logs; container +jobs also retain reports as artifacts. The +[analyzer guide](../scripts/README.md#analyzer-ci) covers reproduction and +the distinction between findings and incomplete analysis. ## Local reproduction diff --git a/scripts/README.md b/scripts/README.md index ceeb084c6..e71e1cdd6 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -271,15 +271,17 @@ returns 2 (Make maps both to 2). Empty input is a skip. ## Analyzer CI The [workflow](../.github/workflows/analyzers.yml) runs on pushes to `master` -and manual dispatch. Separate jobs run `make sparse` and `make smatch` with -`KUNIT_TEST=y`. Reports in `.cache/lint/` are kept for seven days. Use the -commands above to run either analyzer locally. The runner and image also +and manual dispatch. Separate jobs run `make sparse`, `make smatch` and +`make checkpatch`, including KUnit sources. `Analyzer script tests` runs +`make lint-test`; it does not execute SCST tests. A parallel `SCST code style` +job runs `make scs_lint` without a kernel build. The container jobs retain +reports from `.cache/lint/` for seven days. The runner and image also support Coccinelle and Cppcheck; their matrix entries remain commented out. -CI temporarily reports completed source findings without blocking while fixes -land. Build/tool failures, incomplete analysis and missing KUnit coverage -still fail. This policy lives in `ci/run-analyzer.py`; local Make targets -always fail on findings. +CI fails on findings remaining after policy exceptions, just like local Make +targets. The native-analyzer runner returns 1 for completed analysis with +findings and 2 for build/tool failures, incomplete analysis or missing KUnit +coverage. PRs retain the separate patch/commit Checkpatch workflow. ## Dependencies and tooling tests diff --git a/scripts/ci/run-analyzer.py b/scripts/ci/run-analyzer.py index db325762c..0363481f5 100644 --- a/scripts/ci/run-analyzer.py +++ b/scripts/ci/run-analyzer.py @@ -17,12 +17,23 @@ ROOT = Path(__file__).resolve().parents[2] sys.path.insert(0, str(ROOT / "scripts")) from analyzer_policy import source_policy # noqa: E402 from lint_common import copy_output, entrypoint, run # noqa: E402 +from style.checkpatch import checker_path # noqa: E402 -TOOLS = {"sparse": "sparse", "smatch": "smatch", "coccicheck": "spatch", "cppcheck": "cppcheck"} +TOOLS = {"sparse": "sparse", "smatch": "smatch", "coccicheck": "spatch", "cppcheck": "cppcheck", + "checkpatch": "checkpatch.pl"} def complete_native_report(report): tool = report.get("tool") + if tool == "checkpatch": + checked = report.get("checked_files") + completed = report.get("completed_files") + return (report.get("complete") is True and not report.get("failures") and + report.get("native_status") == report.get("exit_code") and + isinstance(checked, list) and bool(checked) and + checked == report.get("selected_files") and + isinstance(completed, list) and + sorted(str((ROOT / path).resolve()) for path in checked) == sorted(completed)) if tool in {"sparse", "smatch"}: return (bool(report.get("translation_units")) and report.get("make_status") == 0 and report.get("errors") == 0 and not report.get("unchecked_files") and @@ -50,7 +61,7 @@ def digest(path): def job_status(make_status, report): - """Only a completed scan with source findings may become advisory.""" + """Return 0 for clean scans, 1 for findings and 2 for incomplete analysis.""" status = report.get("exit_code") if (report.get("tool") not in TOOLS or type(status) is not int or status not in (0, 1) or @@ -73,8 +84,7 @@ def job_status(make_status, report): "plus userspace where selected by policy.", file=sys.stderr) return 2 - # Temporary rollout policy: completed source findings are advisory. - return 0 + return status def main(): @@ -82,12 +92,14 @@ def main(): parser.add_argument("tool", choices=tuple(TOOLS)) args = parser.parse_args() kernel = Path(os.environ.get("SCST_LINT_KDIR") or os.environ.get("KDIR", "")) - if not (kernel / "Module.symvers").is_file(): - raise ValueError("Set KDIR to a prepared kernel build tree with Module.symvers") - config = (kernel / ".config").read_text() - if not any(line in {"CONFIG_KUNIT=y", "CONFIG_KUNIT=m"} for line in config.splitlines()): - raise ValueError("The CI analysis kernel must enable CONFIG_KUNIT") - executable = shutil.which(TOOLS[args.tool]) + config_path = kernel / ".config" + config = config_path.read_text() if config_path.is_file() else "" + if args.tool != "checkpatch": + if not (kernel / "Module.symvers").is_file(): + raise ValueError("Set KDIR to a prepared kernel build tree with Module.symvers") + if not any(line in {"CONFIG_KUNIT=y", "CONFIG_KUNIT=m"} for line in config.splitlines()): + raise ValueError("The CI analysis kernel must enable CONFIG_KUNIT") + executable = str(checker_path()) if args.tool == "checkpatch" else shutil.which(TOOLS[args.tool]) if not executable: raise ValueError(f"{args.tool} is not installed") directory = ROOT / ".cache/lint" @@ -95,12 +107,18 @@ def main(): provenance = Path("/usr/local/share/scst-ci") / (args.tool + "-source.txt") environment = { "commit": subprocess.check_output(["git", "rev-parse", "HEAD"], cwd=ROOT, text=True).strip(), - "kernel": str(kernel.resolve()), "kernel_config_sha256": digest(kernel / ".config"), + "kernel": str(kernel.resolve()), + "kernel_config_sha256": digest(config_path) if config else None, "tool": executable, "tool_sha256": digest(Path(executable)), "version": subprocess.check_output([executable, "--version"], text=True).strip(), "source_provenance": provenance.read_text() if provenance.exists() else "unavailable", "kunit_test": "y", } + if args.tool == "checkpatch": + environment["version"] = next((line for line in environment["version"].splitlines() + if line.startswith("Version:")), environment["version"]) + environment["data_sha256"] = {name: digest(Path(executable).with_name(name)) + for name in ("spelling.txt", "const_structs.checkpatch")} environment_text = json.dumps(environment, indent=2) + "\n" (directory / f"{args.tool}.environment.json").write_text(environment_text) print(environment_text, flush=True) @@ -135,9 +153,9 @@ def main(): print(f"\nFull report: {log}", flush=True) copy_output(log, sys.stdout.buffer) status = job_status(result.returncode, report) - if status == 0 and report["exit_code"] == 1: - print(f"::warning::{args.tool}: {report['remaining']} source finding(s); " - "temporary report mode, analysis completed.") + if status == 1: + print(f"::error::{args.tool}: {report['remaining']} source finding(s); " + "analysis completed.") summary = (f"{args.tool}: outcome={report.get('outcome', 'missing')}, " f"remaining={report.get('remaining', 'unknown')}, " f"make exit={result.returncode}, job exit={status}\n") diff --git a/scripts/tests/test_analyzer_ci.py b/scripts/tests/test_analyzer_ci.py index 273e8a712..a36907c94 100644 --- a/scripts/tests/test_analyzer_ci.py +++ b/scripts/tests/test_analyzer_ci.py @@ -25,8 +25,8 @@ class AnalyzerCiTests(unittest.TestCase): # Exercise real GNU Make's mapping of recipe failures to status 2. # The fixture driver below produces reports without compiling SCST. (self.root / "Makefile").write_text( - "SHELL := /bin/sh\n.PHONY: sparse smatch coccicheck cppcheck\n" - "sparse smatch coccicheck cppcheck:\n" + "SHELL := /bin/sh\n.PHONY: sparse smatch coccicheck cppcheck checkpatch\n" + "sparse smatch coccicheck cppcheck checkpatch:\n" f"\t@{shlex.quote(sys.executable)} -B scripts/sparse_smatch/smp.py $@\n" "\t@exit $(FIXTURE_MAKE_TAIL_EXIT)\n") for name in ("git", "sparse", "smatch", "spatch", "cppcheck"): @@ -37,11 +37,18 @@ class AnalyzerCiTests(unittest.TestCase): kernel.mkdir() (kernel / "Module.symvers").touch() (kernel / ".config").write_text("CONFIG_KUNIT=y\n") + checker = kernel / "scripts/checkpatch.pl" + checker.parent.mkdir() + checker.write_text(f"#!{sys.executable}\nprint('Version: fixture-version')\n") + checker.chmod(0o755) + for name in ("spelling.txt", "const_structs.checkpatch"): + checker.with_name(name).touch() self.env = dict(os.environ, KDIR=str(kernel), SCST_LINT_KDIR=str(kernel), PATH=str(self.bin), MAKEFLAGS="", MFLAGS="", MAKEOVERRIDES="", GNUMAKEFLAGS="", FIXTURE_STATUS="0", FIXTURE_MAKE_TAIL_EXIT="0", FIXTURE_OUTCOME="clean", FIXTURE_COVERAGE="full") self.env.pop("GITHUB_STEP_SUMMARY", None) + self.env.pop("CHECKPATCH_PATH", None) driver = self.root / "scripts/sparse_smatch/smp.py" driver.write_text('''import json, os, sys from pathlib import Path @@ -59,7 +66,7 @@ if os.environ.get('FIXTURE_NO_REPORT'): checked = [p + '/fixture.c' for p in ('scst', 'iscsi-scst', 'qla2x00t-32gbit/qla2x00-target', 'srpt', 'fcst', 'scst_local')] checked += ['scst/src/kunit/fixture.c'] -if tool in ('coccicheck', 'cppcheck'): +if tool in ('coccicheck', 'cppcheck', 'checkpatch'): checked += ['usr/fileio/fixture.c', 'iscsi-scst/usr/fixture.c'] if os.environ['FIXTURE_COVERAGE'] != 'full': checked = checked[:1] @@ -72,9 +79,14 @@ report['rules'] = [dict(complete=True, native_status=0, failures=[], handled=che report['kernel_make_status'] = report['userspace_make_status'] = 0 report['profiles'] = [dict(profile=profile, complete=True, native_status=0, failures=[], translation_units=1, checked_files=checked) for profile in ('kernel', 'userspace')] +report.update(complete=True, native_status=status, failures=[], selected_files=checked, + completed_files=[str((root / path).resolve()) for path in checked]) if os.environ.get('FIXTURE_NATIVE_INCOMPLETE'): report['rules'][0]['complete'] = False report['profiles'][0]['complete'] = False + report['complete'] = False +if os.environ.get('FIXTURE_MISSING_COMPLETION'): + report['completed_files'].pop() if os.environ.get('FIXTURE_STALE'): report['run_id'] = 'previous run' (directory / (tool + '.json')).write_text(json.dumps(report)) @@ -88,19 +100,20 @@ sys.exit(status) tool], cwd=self.root, env=self.env, capture_output=True, text=True, timeout=10) - def test_only_completed_findings_can_be_advisory(self): - for tool in ("sparse", "smatch", "coccicheck", "cppcheck"): + def test_findings_fail_separately_from_incomplete_analysis(self): + for tool in ("sparse", "smatch", "coccicheck", "cppcheck", "checkpatch"): for status, outcome, expected in ( - (0, "clean", 0), (1, "findings", 0), (2, "incomplete", 2), + (0, "clean", 0), (1, "findings", 1), (2, "incomplete", 2), (1, "incomplete", 2), (0, "skipped", 2), (0, "findings", 2)): with self.subTest(tool=tool, status=status, outcome=outcome): self.env.update(FIXTURE_STATUS=str(status), FIXTURE_OUTCOME=outcome) result = self.run_ci(tool) self.assertEqual(result.returncode, expected, result.stdout + result.stderr) self.assertEqual(result.stdout.count("raw diagnostic fixture"), 1) - self.assertEqual("::warning::" in result.stdout, - outcome == "findings" and expected == 0) + self.assertEqual("::error::" in result.stdout, expected == 1) + self.assertNotIn("::warning::", result.stdout) self.assertIn(f"make exit={2 if status else 0}", result.stdout) + self.assertIn(f"job exit={expected}", result.stdout) def test_make_failure_after_clean_analysis_cannot_pass(self): self.env["FIXTURE_MAKE_TAIL_EXIT"] = "7" @@ -108,13 +121,30 @@ sys.exit(status) self.assertEqual(result.returncode, 2, result.stdout + result.stderr) self.assertIn("make exit=2, job exit=2", result.stdout) - def test_basic_analyzers_cannot_downgrade_incomplete_native_reports(self): + def test_basic_analyzers_cannot_report_incomplete_scans_as_findings(self): self.env.update(FIXTURE_STATUS="1", FIXTURE_OUTCOME="findings", FIXTURE_NATIVE_INCOMPLETE="1") - for tool in ("coccicheck", "cppcheck"): + for tool in ("coccicheck", "cppcheck", "checkpatch"): with self.subTest(tool=tool): result = self.run_ci(tool) self.assertEqual(result.returncode, 2, result.stdout + result.stderr) - self.assertNotIn("::warning::", result.stdout) + self.assertNotIn("::error::", result.stdout) + + def test_checkpatch_requires_every_native_file_completion(self): + self.env.update(FIXTURE_STATUS="1", FIXTURE_OUTCOME="findings", + FIXTURE_MISSING_COMPLETION="1") + result = self.run_ci("checkpatch") + self.assertEqual(result.returncode, 2, result.stdout + result.stderr) + self.assertNotIn("::error::", result.stdout) + + def test_checkpatch_needs_tool_data_but_not_a_prepared_kunit_kernel(self): + (self.root / "kernel/.config").unlink() + (self.root / "kernel/Module.symvers").unlink() + result = self.run_ci("checkpatch") + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + (self.root / "kernel/scripts/spelling.txt").unlink() + result = self.run_ci("checkpatch") + self.assertEqual(result.returncode, 2, result.stdout + result.stderr) + self.assertIn("missing Checkpatch tool/data", result.stderr) def test_make_failure_without_a_new_report_cannot_reuse_previous_results(self): self.assertEqual(self.run_ci().returncode, 0)