diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9f4a2ab1..cdb00663 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -105,6 +105,25 @@ jobs: git show "$BASE_SHA:CHANGELOG.md" > "$RUNNER_TEMP/base-CHANGELOG.md" python3 scripts/changelog_frozen.py \ "$RUNNER_TEMP/base-CHANGELOG.md" CHANGELOG.md + # blooop/devlaunch#591. Two branches bumped to 0.36.0 independently and git + # merged it silently, because the same number on both sides is the identical + # edit and there is nothing to conflict on. The second merge then published + # nothing. This is the last point where the two versions still differ: by the + # time the merge is on `main` its version equals its first parent's, so + # `publish.yml` has nothing to notice. + # + # Same `BASE_SHA` the step above fetched, which is the base as of the pull + # request rather than the moving tip of `main` -- for #591 that was the commit + # before the rival release landed, where the version was still 0.35.0. + - name: A version that is already published is not one to bump to + if: github.event_name == 'pull_request' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + git show "$BASE_SHA:rust/Cargo.toml" > "$RUNNER_TEMP/base-Cargo.toml" + python3 scripts/version_untaken.py \ + "$RUNNER_TEMP/base-Cargo.toml" rust/Cargo.toml # Flagged `python` since #294, so it is reported as what it is — the # harness, the doc guards and `scripts/` — rather than as "the project's # coverage". The shipped crates are the `rust` flag, uploaded by the diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index cf0252e9..ed57d465 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -49,18 +49,46 @@ jobs: fi echo "version=$version" >> "$GITHUB_OUTPUT" - # Already released: a re-run of this workflow over the same commit, or a - # later push that did not touch the version. Nothing to do, and saying - # so beats a failed upload. + git show HEAD^:rust/Cargo.toml > /tmp/previous-cargo.toml 2>/dev/null || : > /tmp/previous-cargo.toml + previous=$(read_version /tmp/previous-cargo.toml) + + # Already tagged. Existence is asked of the ref alone, never of the commit + # behind it: this is a depth-2 clone, so a tag pointing anywhere but here + # may have no commit object to resolve, and a failed resolve must not read + # as "free to publish". if git rev-parse -q --verify "refs/tags/v$version" >/dev/null; then + tagged_at=$(git rev-parse -q --verify "refs/tags/v$version^{commit}" || true) + if [ -n "$tagged_at" ] && [ "$tagged_at" = "$(git rev-parse HEAD)" ]; then + # The tag is on this very commit: a re-run of the workflow over the + # release it already published. `gh release create --target + # $GITHUB_SHA` is what puts it here, and idempotence is the point. + echo "v$version was published from this commit; nothing to publish" + echo "publish=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + if [ "$version" != "$previous" ]; then + # This push moved the version *onto* a number somebody else already + # released. Two branches cut the same one, which git merges with no + # conflict because the same version line on both sides is one edit + # (blooop/devlaunch#591). Refusing to publish is right either way; the + # difference is that this says why, instead of reporting the same + # "nothing to publish" an ordinary push gets -- which is how #591 sat + # merged and released nowhere with nothing red. + # + # Narrow on purpose. It fires only when the version changed against the + # first parent, and #591's own merge did not: both sides were already + # 0.36.0, so there was no bump left to see by then. That case is caught + # a step earlier, by `scripts/version_untaken.py` on the pull request. + echo "::error::this push moves the version to $version, but v$version is already tagged at ${tagged_at:-another commit}. Two branches cut the same release; bump to an unused version and re-file the CHANGELOG entry under it." + exit 1 + fi + # A later push that did not touch the version. Nothing to do, and saying + # so beats a failed upload. echo "v$version is already tagged; nothing to publish" echo "publish=false" >> "$GITHUB_OUTPUT" exit 0 fi - git show HEAD^:rust/Cargo.toml > /tmp/previous-cargo.toml 2>/dev/null || : > /tmp/previous-cargo.toml - previous=$(read_version /tmp/previous-cargo.toml) - if [ "$version" = "$previous" ]; then echo "version is still ${version:-unset}; nothing to publish" echo "publish=false" >> "$GITHUB_OUTPUT" diff --git a/scripts/version_untaken.py b/scripts/version_untaken.py new file mode 100755 index 00000000..4b83f413 --- /dev/null +++ b/scripts/version_untaken.py @@ -0,0 +1,128 @@ +#!/usr/bin/env python3 +"""A branch may not propose a version that has already been published. + +blooop/devlaunch#591. Two branches bumped to 0.36.0 independently: #593 cut it and +published, and #591 -- open across that release -- carried a `release: 0.36.0` +commit of its own. Git merged the collision *silently*, because picking the same +number means both sides made the identical edit: one `version = "0.36.0"` line in +`rust/Cargo.toml` with nothing to resolve, and one byte-identical +`## [0.36.0]` heading in CHANGELOG.md with the differing bodies merged in +underneath as separate additions. Choosing 0.37.0 instead would have conflicted +loudly in `Cargo.toml` and been caught at the merge. + +The cost is that the second merge publishes **nothing**. `publish.yml` sees the +version already tagged and reports `nothing to publish`, which is also what it +says for every ordinary push, so the fix sat merged and unreleased with no check +red and no line in any log to act on. + +**Why this runs at pull-request time and not at publish time.** By the time the +merge reaches `main` the collision is no longer visible: the merge commit's +version equals its first parent's -- both are 0.36.0 -- so "did this push change +the version" is false and there is nothing for `publish.yml` to notice. The +branch is the last place the two numbers still differ. + +**Why comparing against the base commit is enough.** `pull_request.base.sha` is +the base as of the pull request, not the moving tip of `main`: for #591 it was +`908a24e`, the commit before #593 landed, where the version was still 0.35.0. So +the comparison sees 0.35.0 -> 0.36.0, a proposed bump, and asks the one question +worth asking about it -- is `v0.36.0` already out there? + +Usage: + version_untaken.py + +Exits 0 when the branch proposes no new version, or proposes one that has never +been tagged. Exits 1 when it proposes a version that is already published, and 1 +(never 0) when either side cannot be read -- a guard that cannot read its input +has checked nothing. +""" + +import re +import subprocess +import sys +from pathlib import Path + +# `[workspace.package] version` is the first `version = "..."` line in the file; +# the crates inherit it with `version.workspace = true`. `publish.yml` and +# `conda-publish.yml` both read it exactly this way -- keep the three in step. +VERSION = re.compile(r'^version = "(.*)"', re.MULTILINE) + + +class Unreadable(Exception): + """The manifest is not one this guard can take a version out of.""" + + +def version_in(path: Path, where: str) -> str: + try: + text = path.read_text(encoding="utf-8") + except OSError as problem: + raise Unreadable(f"{where}: cannot be read ({problem})") from problem + found = VERSION.search(text) + if not found: + raise Unreadable(f"{where}: no 'version = \"...\"' line") + return found.group(1) + + +def is_tagged(version: str, run=subprocess.run) -> bool | None: + """Whether `v` already names a commit. + + `None` is "could not tell" -- no git, or a git that failed for a reason of its + own -- and the caller refuses on it. A missing tag is a confident `False` and + the ordinary answer for a release being cut. + """ + try: + done = run( + ["git", "rev-parse", "-q", "--verify", f"refs/tags/v{version}^{{commit}}"], + capture_output=True, + text=True, + check=False, + ) + except OSError: + return None + if done.returncode == 0: + return True + # `rev-parse -q --verify` exits 1 for "no such ref" and reserves other codes + # for being unable to answer, which is not the same thing and must not read as + # "free to publish". + return False if done.returncode == 1 else None + + +def main(argv: list[str]) -> int: + if len(argv) != 3: + print(f"usage: {Path(argv[0]).name} ", file=sys.stderr) + return 2 + try: + base = version_in(Path(argv[1]), "base rust/Cargo.toml") + head = version_in(Path(argv[2]), "head rust/Cargo.toml") + except Unreadable as problem: + print(f"the version could not be checked, so it is not passing: {problem}", file=sys.stderr) + return 1 + + if base == head: + print(f"this branch proposes no new version (still {head})") + return 0 + + taken = is_tagged(head) + if taken is None: + print( + f"could not ask git whether v{head} is already tagged, so this is not passing", + file=sys.stderr, + ) + return 1 + if taken: + print( + f"this branch bumps {base} -> {head}, and v{head} is already tagged.\n\n" + f"Someone else cut {head} while this branch was open, so merging it would\n" + f"publish nothing: `publish.yml` finds the tag, reports 'nothing to publish',\n" + f"and the work lands on main released nowhere. Git will not catch it for you --\n" + f"both sides wrote the same version line, so there is nothing to conflict on.\n\n" + f"Bump to the next unused version instead, and check that the CHANGELOG entry\n" + f"is under that heading rather than under {head}'s.\n", + file=sys.stderr, + ) + return 1 + print(f"this branch bumps {base} -> {head}, which has never been tagged") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/test/test_publish_decision.py b/test/test_publish_decision.py new file mode 100644 index 00000000..4678b097 --- /dev/null +++ b/test/test_publish_decision.py @@ -0,0 +1,156 @@ +"""The four answers `publish.yml` can give, exercised as the shell it actually is. + +The step decides whether a push to `main` is a release, and it now has four arms +rather than two. Three of them decline to publish and one of those is an *error*, +which is the whole point of the change: blooop/devlaunch#591 merged a version bump +onto a number another branch had already released, and the workflow reported +`nothing to publish` -- the same sentence an ordinary push gets -- so the work sat +on main released nowhere with no check red. + +The script is extracted from the workflow and run, rather than reimplemented here. +A rewritten copy would be a second hand-maintained version of the logic and would +pass while the real one rotted; running the real text means the test fails if the +step is edited into disagreeing with it. + +What this does **not** claim: that the error arm would have caught #591. It would +not have, and `test_the_case_that_needs_the_pull_request_guard` pins that instead. +By the time #591 reached main its merge carried the same version as its first +parent, so there was no bump left to see. `scripts/version_untaken.py` is the guard +for that, and it runs on the pull request. +""" + +import os +import subprocess +from pathlib import Path + +import pytest + +ROOT = Path(__file__).parent.parent +PUBLISH = ROOT / ".github" / "workflows" / "publish.yml" +STEP = " - name: Decide whether there is a release to publish\n" + + +def decide_script() -> str: + """The `run:` body of the decision step, dedented out of the workflow.""" + text = PUBLISH.read_text(encoding="utf-8") + assert STEP in text, "the decision step is not named what this test looks for" + body = text.split(STEP, 1)[1].split("run: |\n", 1)[1] + lines = [] + for line in body.splitlines(): + if line.strip() and not line.startswith(" " * 10): + break + lines.append(line[10:]) + return "\n".join(lines) + + +def git(*args: str, cwd: Path) -> None: + done = subprocess.run(("git", *args), cwd=cwd, capture_output=True, text=True, check=False) + assert done.returncode == 0, f"git {' '.join(args)} failed: {done.stderr}" + + +@pytest.fixture(name="repo") +def a_repository_to_decide_about(tmp_path: Path) -> Path: + work = tmp_path / "repo" + (work / "rust").mkdir(parents=True) + git("init", "-q", "-b", "main", ".", cwd=work) + git("config", "user.email", "publish@example.invalid", cwd=work) + git("config", "user.name", "Publish Fixture", cwd=work) + return work + + +def commit(repo: Path, version: str) -> None: + (repo / "rust" / "Cargo.toml").write_text( + f'[workspace.package]\nversion = "{version}"\n', encoding="utf-8" + ) + git("add", "-A", cwd=repo) + git("commit", "-qm", f"v{version}", cwd=repo) + + +def decide(repo: Path, tmp_path: Path) -> subprocess.CompletedProcess: + output = tmp_path / "github-output" + output.write_text("", encoding="utf-8") + script = tmp_path / "decide.sh" + script.write_text(decide_script(), encoding="utf-8") + return subprocess.run( + ("bash", str(script)), + cwd=repo, + capture_output=True, + text=True, + check=False, + # The runner's own PATH, not a guessed one: the script needs `git` and + # `sed`, and hardcoding where they live is a test that passes here and + # fails on a machine that puts them somewhere else. + env={"PATH": os.environ["PATH"], "GITHUB_OUTPUT": str(output)}, + ) + + +def test_a_bump_to_a_version_nobody_has_taken_publishes(repo, tmp_path): + commit(repo, "0.36.0") + commit(repo, "0.37.0") + + done = decide(repo, tmp_path) + + assert done.returncode == 0, done.stderr + assert "publishing" in done.stdout + + +def test_a_re_run_over_the_commit_that_published_declines_quietly(repo, tmp_path): + """Idempotence, and the ordinary reason the tag is found at all. + + `gh release create --target $GITHUB_SHA` puts the tag on the commit that + published, so a re-run sees it on HEAD and must do nothing rather than fail. + """ + commit(repo, "0.36.0") + commit(repo, "0.37.0") + git("tag", "v0.37.0", cwd=repo) + + done = decide(repo, tmp_path) + + assert done.returncode == 0, done.stderr + assert "published from this commit" in done.stdout + + +def test_a_later_push_that_did_not_touch_the_version_declines_quietly(repo, tmp_path): + commit(repo, "0.36.0") + commit(repo, "0.37.0") + git("tag", "v0.37.0", cwd=repo) + (repo / "unrelated.txt").write_text("a change that is not a release\n", encoding="utf-8") + git("add", "-A", cwd=repo) + git("commit", "-qm", "later", cwd=repo) + + done = decide(repo, tmp_path) + + assert done.returncode == 0, done.stderr + assert "already tagged; nothing to publish" in done.stdout + + +def test_a_bump_onto_a_version_already_tagged_is_an_error_not_a_shrug(repo, tmp_path): + """The arm that is new: declining, but loudly enough to act on.""" + commit(repo, "0.36.0") + git("tag", "v0.37.0", cwd=repo) + commit(repo, "0.37.0") + + done = decide(repo, tmp_path) + + assert done.returncode == 1 + assert "::error::" in done.stdout + assert "already tagged" in done.stdout + + +def test_the_case_that_needs_the_pull_request_guard(repo, tmp_path): + """#591's shape: the merge carries the version its first parent already had. + + Nothing here can see a bump, so this step correctly says nothing to publish and + correctly does not error. That silence is exactly why the collision has to be + refused on the branch instead, which is `scripts/version_untaken.py`. + """ + commit(repo, "0.36.0") + git("tag", "v0.36.0", cwd=repo) + (repo / "unrelated.txt").write_text("the merge's other side\n", encoding="utf-8") + git("add", "-A", cwd=repo) + git("commit", "-qm", "a merge carrying the same version", cwd=repo) + + done = decide(repo, tmp_path) + + assert done.returncode == 0, done.stderr + assert "::error::" not in done.stdout diff --git a/test/test_version_untaken.py b/test/test_version_untaken.py new file mode 100644 index 00000000..aa8a034b --- /dev/null +++ b/test/test_version_untaken.py @@ -0,0 +1,144 @@ +"""The guard that refuses a bump onto a version somebody already published. + +blooop/devlaunch#591. Two branches bumped to 0.36.0 independently: #593 cut it and +published, and #591 -- open across that release -- carried its own +`release: 0.36.0`. Git merged the collision without a conflict, because picking +the same number means both sides made the identical edit, and the second merge +published nothing at all: `publish.yml` found the tag, said `nothing to publish` +(what it says for every ordinary push too), and the fix landed on main released +nowhere with no check red. + +Each test builds its own repository and its own tag rather than reading the real +history back. The first draft did read it -- #591's actual two manifests by SHA -- +and it was wrong twice over: CI checks out shallow, so those commits are not +there, and the guard's oracle is `git rev-parse refs/tags/v` in whatever +directory it runs in, so it would have been asking the checkout about tags the +checkout may not have fetched either. What is worth holding is the shape and the +numbers, and a repository built here has both under the test's own control. +""" + +import subprocess +import sys +from pathlib import Path + +import pytest + +ROOT = Path(__file__).parent.parent +GUARD = (ROOT / "scripts" / "version_untaken.py").resolve() +CI = ROOT / ".github" / "workflows" / "ci.yml" + + +def run(*args: str, cwd: Path | None = None) -> subprocess.CompletedProcess: + return subprocess.run(args, cwd=cwd, capture_output=True, text=True, check=False) + + +def git(*args: str, cwd: Path) -> None: + done = run("git", *args, cwd=cwd) + assert done.returncode == 0, f"git {' '.join(args)} failed: {done.stderr}" + + +def manifest(version: str) -> str: + return f'[workspace.package]\nversion = "{version}"\nedition = "2024"\n' + + +# Named through the decorator so the fixture function and the parameter that +# receives it are not one name in one module, which pylint reads as shadowing. +@pytest.fixture(name="released") +def a_repository_with_a_release_already_out(tmp_path: Path) -> Path: + """A repository where v0.36.0 has been cut and tagged, as #593 left main.""" + work = tmp_path / "repo" + work.mkdir() + git("init", "-q", "-b", "main", ".", cwd=work) + git("config", "user.email", "version@example.invalid", cwd=work) + git("config", "user.name", "Version Fixture", cwd=work) + (work / "seed.txt").write_text("the release\n", encoding="utf-8") + git("add", "-A", cwd=work) + git("commit", "-qm", "cut 0.36.0", cwd=work) + git("tag", "v0.36.0", cwd=work) + return work + + +def check(base: str, head: str, tmp_path: Path, cwd: Path) -> subprocess.CompletedProcess: + """Run the guard in `cwd`, which is the repository whose tags it will consult.""" + (tmp_path / "base.toml").write_text(base, encoding="utf-8") + (tmp_path / "head.toml").write_text(head, encoding="utf-8") + return run( + sys.executable, + str(GUARD), + str(tmp_path / "base.toml"), + str(tmp_path / "head.toml"), + cwd=cwd, + ) + + +def test_the_collision_that_happened_is_refused(released, tmp_path): + """#591's numbers: 0.35.0 at its base, 0.36.0 at its head, v0.36.0 already out. + + `pull_request.base.sha` is the base as of the pull request rather than the + moving tip of main -- for #591 it was 908a24e, before the rival release, where + the version was still 0.35.0. That is why the comparison sees a bump at all. + """ + done = check(manifest("0.35.0"), manifest("0.36.0"), tmp_path, released) + + assert done.returncode == 1 + assert "v0.36.0 is already tagged" in done.stderr + assert "publish nothing" in done.stderr + + +def test_a_merge_commit_cannot_see_the_collision_its_branch_could(released, tmp_path): + """Why the guard runs on the branch and not after the merge. + + Once #591 merged, the merge commit's version and its first parent's were the + same string -- both 0.36.0 -- so every "did this push bump the version" test + downstream answers no. There is nothing left to catch by then, which is the + whole argument for checking earlier, and the reason `publish.yml`'s own error + arm is not a second answer to this case. + """ + done = check(manifest("0.36.0"), manifest("0.36.0"), tmp_path, released) + + assert done.returncode == 0, done.stderr + assert "proposes no new version" in done.stdout + + +def test_an_ordinary_branch_that_never_touches_the_version_passes(released, tmp_path): + """The common case, and the one a false positive here would block outright.""" + done = check(manifest("0.36.0"), manifest("0.36.0"), tmp_path, released) + + assert done.returncode == 0, done.stderr + + +def test_a_release_cut_to_a_version_nobody_has_taken_passes(released, tmp_path): + """The ritual this guard must not break.""" + done = check(manifest("0.36.0"), manifest("0.37.0"), tmp_path, released) + + assert done.returncode == 0, done.stderr + assert "never been tagged" in done.stdout + + +def test_a_manifest_with_no_version_fails_rather_than_passes(released, tmp_path): + """A guard that cannot read its input has checked nothing.""" + done = check("[workspace.package]\n", manifest("0.37.0"), tmp_path, released) + + assert done.returncode == 1 + assert "not passing" in done.stderr + + +def test_a_manifest_that_is_not_there_fails_rather_than_passes(released, tmp_path): + done = run( + sys.executable, + str(GUARD), + str(tmp_path / "absent.toml"), + str(tmp_path / "also.toml"), + cwd=released, + ) + + assert done.returncode == 1 + assert "not passing" in done.stderr + + +def test_ci_runs_the_guard_on_pull_requests(): + """It is worth a step only if a step actually runs it.""" + ci = CI.read_text(encoding="utf-8") + + assert "scripts/version_untaken.py" in ci + assert "A version that is already published is not one to bump to" in ci