diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a2ce0232..5f8a2ee1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -204,7 +204,7 @@ jobs: fi changed_files="${RUNNER_TEMP}/ci-changed-files" - if ! git diff --name-only "${BASE_SHA}" "${HEAD_SHA}" > "${changed_files}"; then + if ! git diff --name-only --no-renames -z "${BASE_SHA}" "${HEAD_SHA}" > "${changed_files}"; then run_everything "The changed files could not be determined, so the safe fallback is to run the full matrix." exit 0 fi @@ -213,7 +213,7 @@ jobs: classification="${RUNNER_TEMP}/ci-classification" if ! python3 scripts/nvx.py classify-ci-changes \ --event-name "${EVENT_NAME}" \ - --changed-files "${changed_files}" \ + --changed-files "${changed_files}" --null \ --summary "${GITHUB_STEP_SUMMARY}" > "${classification}"; then run_everything "The changes could not be classified, so the safe fallback is to run the full matrix." exit 0 diff --git a/doc/ci.md b/doc/ci.md index 5828fe5a..5c9dc7a0 100644 --- a/doc/ci.md +++ b/doc/ci.md @@ -412,7 +412,10 @@ job or a shard needs runners to run it in parallel; otherwise it only queues. `openvmm-changes` lists the files that changed between the pull request's base and head, or between a push's previous and new commits, and -`nvx.py classify-ci-changes` selects the suites from their paths: +`nvx.py classify-ci-changes` selects the suites from their paths. The detector +uses `git diff --name-only --no-renames -z` and the classifier's `--null` option +so both sides of a rename and unquoted filenames, including embedded newlines, +participate in the decision: - `run-workloads` is false only when every changed file is documentation, under `doc/` or ending in `.md`. It enables the guest artifacts, the OpenVMM diff --git a/doc/usage.md b/doc/usage.md index 2f3646a2..a6d9cf4f 100644 --- a/doc/usage.md +++ b/doc/usage.md @@ -86,10 +86,13 @@ Installs the GNU tar and zstd tools used by GitHub Actions cross-OS caches. python3 scripts/nvx.py classify-ci-changes --event-name {pull_request,push} --changed-files PATH + [--null] [--summary PATH] ``` -Reads one changed repository path per line and prints the change detector's +Reads one changed repository path per line by default, or NUL-delimited paths +with `--null`, and prints +the change detector's `run-tests`, `run-workloads`, `run-openvmm-unit-tests`, and `run-openvmm-vmm-tests` outputs as `name=true` or `name=false` lines. Only documentation changes skip the workloads, and only test inputs run the NVX diff --git a/scripts/nvx.py b/scripts/nvx.py index 3950e0c7..c7f0622f 100644 --- a/scripts/nvx.py +++ b/scripts/nvx.py @@ -351,10 +351,11 @@ def command_setup_cross_os_cache(_: argparse.Namespace) -> None: def command_classify_ci_changes(args: argparse.Namespace) -> None: - changed_files = args.changed_files.read_text(encoding="utf-8").splitlines() + contents = args.changed_files.read_bytes().decode("utf-8") + changed_files = contents.split("\0") if args.null else contents.splitlines() changes = classify_ci_changes(args.event_name, changed_files) if args.summary is not None: - with args.summary.open("a", encoding="utf-8") as summary: + with args.summary.open("a", encoding="utf-8", newline="") as summary: summary.write(describe_ci_changes(args.event_name, changed_files, changes)) for name, selected in changes.outputs().items(): print(f"{name}={str(selected).lower()}") @@ -1027,6 +1028,11 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: required=True, help="file listing one changed repository path per line", ) + classify_changes.add_argument( + "--null", + action="store_true", + help="read NUL-delimited paths, as emitted by git diff --name-only -z", + ) classify_changes.add_argument( "--summary", type=Path, diff --git a/scripts/test_nvx_tools.py b/scripts/test_nvx_tools.py index ab9c3da0..460f3e8b 100644 --- a/scripts/test_nvx_tools.py +++ b/scripts/test_nvx_tools.py @@ -3354,6 +3354,132 @@ def test_classify_ci_changes_command_prints_every_output(self): self.assertTrue(text.startswith("before\n### ")) self.assertIn("### OpenVMM VMM tests: run\n", text) + def test_classify_ci_changes_command_reads_null_delimited_paths(self): + with tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + changed_files = root / "changed-files" + changed_files.write_bytes( + "doc/notes.md\0kernel/patches/é\r\n.patch\0".encode() + ) + args = nvx.parse_args( + [ + "classify-ci-changes", + "--event-name", + "pull_request", + "--changed-files", + os.fspath(changed_files), + "--null", + "--summary", + os.fspath(root / "summary"), + ] + ) + stdout = io.StringIO() + with contextlib.redirect_stdout(stdout): + args.handler(args) + self.assertEqual( + stdout.getvalue().splitlines(), + [ + "run-tests=true", + "run-workloads=true", + "run-openvmm-unit-tests=false", + "run-openvmm-vmm-tests=true", + ], + ) + summary = (root / "summary").read_bytes().decode("utf-8") + self.assertIn("kernel/patches/é\r\n.patch", summary) + + @unittest.skipUnless(os.name == "posix", "requires bash and POSIX filenames") + def test_ci_change_detector_classifies_real_git_paths(self): + workflow = ( + BuildConstants.REPO_ROOT / ".github" / "workflows" / "ci.yml" + ).read_text(encoding="utf-8") + step = _workflow_step(workflow, "openvmm-changes", "Detect relevant changes") + lines = step.splitlines() + start = lines.index(" run: |") + 1 + script = "\n".join(line[10:] for line in lines[start:]) + classifier = BuildConstants.REPO_ROOT / "scripts" / "nvx.py" + shim = ( + f"python3() {{ {shlex.quote(sys.executable)} " + f'{shlex.quote(os.fspath(classifier))} "${{@:2}}"; }}\n' + ) + cases = ( + ("rename", "doc/input.patch", True), + ("delete", None, True), + ("unicode", "kernel/patches/é.patch", True), + ("newline", "kernel/patches/line\nbreak.patch", True), + ("documentation", "doc/notes.md", False), + ) + for name, destination, test_input in cases: + with self.subTest(name=name), tempfile.TemporaryDirectory() as temporary: + root = Path(temporary) + + def git(*arguments: str, directory: Path = root) -> str: + result = subprocess.run( + ["git", *arguments], + cwd=directory, + capture_output=True, + text=True, + check=True, + ) + return result.stdout.strip() + + git("init", "--quiet") + git("config", "user.name", "Reproduction") + git("config", "user.email", "repro@example.invalid") + git("config", "commit.gpgsign", "false") + source = root / "kernel" / "patches" / "input.patch" + source.parent.mkdir(parents=True) + source.write_text("suite input\n", encoding="utf-8") + git("add", ".") + git("commit", "--quiet", "-m", "baseline") + base = git("rev-parse", "HEAD") + if name == "delete": + source.unlink() + else: + assert destination is not None + target = root / destination + target.parent.mkdir(parents=True, exist_ok=True) + if name == "rename": + source.rename(target) + else: + target.write_text("new input\n", encoding="utf-8") + git("add", "--all") + git("commit", "--quiet", "-m", "changed paths") + for event in ("pull_request", "push"): + with self.subTest(event=event): + output = root / f"output-{event}" + environment = os.environ.copy() + environment.update( + EVENT_NAME=event, + BASE_SHA=base, + HEAD_SHA=git("rev-parse", "HEAD"), + GITHUB_OUTPUT=os.fspath(output), + GITHUB_STEP_SUMMARY=os.fspath(root / "summary"), + RUNNER_TEMP=temporary, + ) + result = subprocess.run( + ["bash", "-c", shim + script], + cwd=root, + env=environment, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr) + actual = dict( + line.split("=", 1) + for line in output.read_text(encoding="utf-8").splitlines() + ) + expected = { + "run-tests": str(test_input).lower(), + "run-workloads": str(test_input).lower(), + "run-openvmm-unit-tests": str( + test_input and event == "push" + ).lower(), + "run-openvmm-vmm-tests": str(test_input).lower(), + } + self.assertEqual(actual, expected) + @unittest.skipUnless(os.name == "posix", "requires bash") def test_ci_change_detector_runs_the_full_matrix_unless_classified(self): workflow = ( @@ -3368,7 +3494,7 @@ def test_ci_change_detector_runs_the_full_matrix_unless_classified(self): case "$1" in cat-file) return "${GIT_CAT_FILE_STATUS}" ;; diff) - printf '%s' "${CHANGED_FILES}" + printf '%b' "${CHANGED_FILES}" return "${GIT_DIFF_STATUS}" ;; esac @@ -3425,7 +3551,7 @@ def test_ci_change_detector_runs_the_full_matrix_unless_classified(self): "real classifier", { "CLASSIFIER": sys.executable, - "CHANGED_FILES": "scripts/nvx_tools/microvm_tests.py\n", + "CHANGED_FILES": "scripts/nvx_tools/microvm_tests.py\\0", }, classified, ), @@ -3444,7 +3570,7 @@ def test_ci_change_detector_runs_the_full_matrix_unless_classified(self): "RUNNER_TEMP": temporary, "GIT_CAT_FILE_STATUS": "0", "GIT_DIFF_STATUS": "0", - "CHANGED_FILES": "openvmm\n", + "CHANGED_FILES": "openvmm\\0", "CLASSIFICATION": complete, "CLASSIFIER_STATUS": "0", }