diff --git a/scripts/repo_shape.py b/scripts/repo_shape.py index f35ee8bf..66b1b592 100644 --- a/scripts/repo_shape.py +++ b/scripts/repo_shape.py @@ -1094,9 +1094,34 @@ def _block_value(text: str, line: int) -> Any: raise +#: U+FEFF, the BYTE-ORDER MARK, which a Windows editor may start a file a +#: person saved with (#855): `parse_yaml` takes it off. +BYTE_ORDER_MARK = "\ufeff" + + def parse_yaml(text: str) -> Any: - """Parse the supported YAML subset. Raises `YamlError` on anything else.""" - recs = _tokenise(text) + """Parse the supported YAML subset. Raises `YamlError` on anything else. + + A BYTE-ORDER MARK AT THE START IS NO PART OF THE TEXT (#855), and every + one there is taken off. YAML 1.2.2 allows a mark at the start of a + document and none inside one (5.2; 9.1.1's `l-document-prefix` is a + mark, then comment lines). Kept, it was the first character of the + first key, so a `project.yaml` that says `schema_version: 1` was + `schema_version is None` to `validate-manifest.py`, and a file whose + first line is a comment was refused at line 1. + + HERE, NOT IN `load_yaml`'S DECODE, because one file read two ways is + worse than either answer. The readers of a pin at a commit + (`shape-doctor.py`'s `pin_at`, `update-shape.py`'s `_committed_pin`, + `adopt-project.py`'s `_pinned_shape_paths`) and `pin_values_in` hand + this the text of no file `load_yaml` read: with the mark taken off + there alone, `pin_at` read a pin every validator reads as broken, and + the doctor's NEXT put back the pin from before it. Every mark goes, as + `shape_advisory.py` takes them off, so the two agree. `load_yaml` still + decodes as `utf-8`, so the byte its refusal names is counted from the + file's first byte, the mark's three included. The tools write no mark. + """ + recs = _tokenise(text.lstrip(BYTE_ORDER_MARK)) if not recs: return None value, end = _parse_nodes(recs, 0, recs[0].indent) diff --git a/templates/assembly-root/scripts/bootstrap.py b/templates/assembly-root/scripts/bootstrap.py index 89e8117d..cec14c25 100755 --- a/templates/assembly-root/scripts/bootstrap.py +++ b/templates/assembly-root/scripts/bootstrap.py @@ -1091,7 +1091,10 @@ def shape_upstream_notice(root: Path) -> None: if tip is None: cache = root / SHAPE_UPSTREAM_CACHE if cache.is_file(): - tip = cache.read_text(encoding="utf-8").strip() + # `utf-8-sig` takes off the byte-order mark a Windows editor may + # start the file with (#855). `strip()` keeps U+FEFF, so a marked + # tip read as no commit and the notice said nothing. + tip = cache.read_text(encoding="utf-8-sig").strip() if not tip or not COMMIT_40.match(tip): return if tip.lower() == pinned: diff --git a/tests/test_bom_reads.py b/tests/test_bom_reads.py new file mode 100644 index 00000000..b60752b6 --- /dev/null +++ b/tests/test_bom_reads.py @@ -0,0 +1,264 @@ +# SPDX-License-Identifier: Apache-2.0 +"""A file a Windows editor saved with a byte-order mark is read as the text +it holds, by every reader of it (#855). + +THE DEFECT, measured on main 9f235da2 by loading the shipped scripts and +calling them, and by running them in a scaffold: + +- `.shape-upstream-tip` holding the bytes EF BB BF and a commit, with LF or + CRLF, made the assembly root's upstream notice say nothing: + `read_text(encoding="utf-8").strip()` kept U+FEFF, which `COMMIT_40` + does not match. +- `repo_shape.load_yaml` read the mark as the first character of the first + key (`commit` was U+FEFF and `commit`), and refused a file whose first + line is a comment, at line 1. So a marked `project.yaml` was `FINDING + manifest-schema-version: schema_version is None, expected 1`, `make + bootstrap` ended `bootstrap FAILED: manifest (exit 1)`, and a marked + `contracts/shape-pin.yaml` left the notice silent. + +THE RULE NOW. The cache is read as `utf-8-sig`. `parse_yaml` takes off +every byte-order mark the text starts with, so `load_yaml` and each reader +that hands the parser a pin's text without reading a file (the doctor's +`pin_at`, `update-shape.py`'s `_committed_pin`, `adopt-project.py`'s +`_pinned_shape_paths`, `pin_values_in`) read the same mapping, as +`shape_advisory.py` already did. A fix in `load_yaml` alone left `pin_at` +reading a committed pin every validator reads as broken, and the doctor's +NEXT put back the pin from before it; the regression row below is that run. + +Every mark is written by this file as bytes (`codecs.BOM_UTF8`), never +typed. +""" + +from __future__ import annotations + +import codecs +from pathlib import Path + +import pytest + +from conftest import REPO, git, load_without_bytecode, run_script +from test_doctor_pin_restore_merges import apply_on, moved_on +from test_shape_doctor import ( # noqa: F401 + SHAPE_PIN, SPEC_PIN, UPSTREAM_TARGET, commit_in, doctor, project, rows_of, + scaffolded_here, standard, +) + +import shape_advisory # noqa: E402 +import shape_materialize # noqa: E402 +from repo_shape import Refusal, load_yaml # noqa: E402 + +MARK = codecs.BOM_UTF8 +#: The mark as the one character the reader reads it as, U+FEFF. +MARK_TEXT = MARK.decode("utf-8") +#: The line a person may put above the file's first key before saving it. +COMMENT = b"# saved by hand\n" +PINNED = "a" * 40 +TIP = "b" * 40 +#: The first words of the notice's one line, naming the cache's commit. +NOTICE = (f" shape: the upstream is at {TIP[:12]} and this project is " + f"pinned at {PINNED[:12]}. ") +BOOTSTRAP = REPO / "templates" / "assembly-root" / "scripts" / "bootstrap.py" +#: The issue's two shapes of a hand-saved file, each holding the same keys. +SHAPES = { + "first-key": f"commit: {PINNED}\nother: 1\n".encode(), + "comment-first": COMMENT + f"commit: {PINNED}\nother: 1\n".encode(), +} + + +@pytest.fixture(scope="module") +def notice(): + """The assembly root's `shape_upstream_notice`, loaded from the template + beside this checkout's `scripts/repo_shape.py`.""" + return load_without_bytecode(BOOTSTRAP, + "bootstrap_bom_reads").shape_upstream_notice + + +@pytest.fixture(scope="module") +def tools(): + """The three tools whose readers hand `parse_yaml` a pin's text.""" + return {name: load_without_bytecode(REPO / f"{name}.py", + f"{name.replace('-', '_')}_bom") + for name in ("update-shape", "shape-doctor", "adopt-project")} + + +def a_root(base: Path, pin: bytes, cache: bytes) -> Path: + """A root holding only `contracts/shape-pin.yaml` and the cache.""" + (base / "contracts").mkdir(parents=True) + (base / SHAPE_PIN).write_bytes(pin) + (base / ".shape-upstream-tip").write_bytes(cache) + return base + + +@pytest.mark.parametrize("cache", [ + pytest.param(f"{TIP}\n".encode(), id="no-mark"), + pytest.param(MARK + f"{TIP}\n".encode(), id="mark"), + pytest.param(MARK + f"{TIP}\r\n".encode(), id="mark-and-crlf"), +]) +def test_the_notice_names_the_tip_a_marked_cache_holds(notice, tmp_path, + capsys, cache): + """THE ISSUE'S FIRST LINE: each says the upstream's tip.""" + notice(a_root(tmp_path, f"commit: {PINNED}\n".encode(), cache)) + assert capsys.readouterr().out.startswith(NOTICE) + + +@pytest.mark.parametrize("marks", [0, 1, 2], ids=["no-mark", "mark", + "two-marks"]) +@pytest.mark.parametrize("shape", list(SHAPES)) +def test_load_yaml_reads_a_marked_file_as_the_keys_it_holds(tmp_path, shape, + marks): + """THE ISSUE'S SECOND LINE, its first two shapes: no first key spelled + with U+FEFF, and no refusal at line 1. Two marks read as one does, as + `shape_advisory.py` reads them.""" + path = tmp_path / "hand-saved.yaml" + path.write_bytes(MARK * marks + SHAPES[shape]) + assert sorted(load_yaml(path)) == ["commit", "other"] + + +@pytest.mark.parametrize("marks", [0, 1, 2], ids=["no-mark", "mark", + "two-marks"]) +def test_the_notice_reads_a_marked_shape_pin(notice, tmp_path, capsys, + marks): + """The second line's third shape: the pin as the notice reads it.""" + notice(a_root(tmp_path, MARK * marks + f"commit: {PINNED}\n".encode(), + f"{TIP}\n".encode())) + assert capsys.readouterr().out.startswith(NOTICE) + + +def test_a_byte_that_is_not_utf_8_is_counted_from_the_files_first_byte( + tmp_path): + """The mark's three bytes are counted: `load_yaml` decodes as `utf-8`, + and the parser, not the decode, takes the mark off. `utf-8-sig` would + count from after the mark and name byte 4 of a file whose 0xff is its + byte 7, where the remedy says to count from the file's start.""" + path = tmp_path / "hand-saved.yaml" + path.write_bytes(MARK + b"k: v\xff\n") + with pytest.raises(Refusal) as refused: + load_yaml(path) + assert refused.value.detail.endswith( + ": not UTF-8 at byte 7 (invalid start byte)"), refused.value.detail + + +@pytest.mark.parametrize("body, read", [ + pytest.param(b'k: "' + MARK + b'x"\n', {"k": MARK_TEXT + "x"}, + id="in-a-quoted-value"), + pytest.param(b"commit: " + MARK + PINNED.encode() + b"\n", + {"commit": MARK_TEXT + PINNED}, id="at-the-start-of-a-value"), + pytest.param(b"k: v\n" + MARK + b"j: w\n", + {"k": "v", MARK_TEXT + "j": "w"}, + id="at-the-start-of-the-second-key"), +]) +def test_a_mark_after_the_start_of_the_text_is_text(tmp_path, body, read): + """ONLY THE START IS TAKEN OFF (RV868's SHOULD-1). YAML 1.2.2 section + 5.2 allows a mark inside a quoted scalar, as text, and none inside a + document, which the reader reads as text. PyYAML 6.0.3 reads all three + as these do. A strip of every mark anywhere would read each of them as + the text without it.""" + path = tmp_path / "hand-saved.yaml" + path.write_bytes(body) + assert load_yaml(path) == read + + +def test_the_notice_says_nothing_of_a_pin_whose_commit_holds_the_mark( + notice, tmp_path, capsys): + """A pin whose `commit:` value starts with the mark names no commit, as + `git rev-parse --verify` refuses one (RV868), so the notice is silent.""" + notice(a_root(tmp_path, b"commit: " + MARK + PINNED.encode() + b"\n", + f"{TIP}\n".encode())) + assert capsys.readouterr().out == "" + + +@pytest.mark.parametrize("marks", [1, 2], ids=["mark", "two-marks"]) +def test_the_advisory_reads_a_marked_manifest_as_load_yaml_does(project, + marks): + """`shape_advisory.py` took every mark off before #855; the two read the + same mapping now.""" + manifest = project / "project.yaml" + manifest.write_bytes(MARK * marks + manifest.read_bytes()) + tree = shape_advisory.Tree(project, None) + assert shape_advisory._read_mapping(tree, "project.yaml") == \ + load_yaml(manifest) + + +def test_a_root_whose_hand_saved_files_carry_the_mark_bootstraps_ok(project): + """Every reader a root runs agrees: the three validators and the notice. + On main this run ended `bootstrap FAILED: manifest (exit 1)`.""" + for name in ("project.yaml", SHAPE_PIN, SPEC_PIN, + "contracts/code-pin.yaml"): + (project / name).write_bytes(MARK + (project / name).read_bytes()) + (project / ".shape-upstream-tip").write_bytes(MARK + f"{TIP}\r\n".encode()) + result = run_script(project / "scripts" / "bootstrap.py", cwd=project, + env={"SHAPE_UPSTREAM_PATH": ""}) + assert result.returncode == 0, result.stdout + result.stderr + assert f" shape: the upstream is at {TIP[:12]} " in result.stdout + assert result.stdout.endswith("\nbootstrap ok\n"), result.stdout + + +# --------------------------------------------------------------------------- +# The readers that hand `parse_yaml` a pin's text, with a committed shape pin +# saved by hand: the mark, a comment line, then the pin the scaffold wrote. +# Each reads what `load_yaml` reads of the same bytes. +# --------------------------------------------------------------------------- + + +@pytest.fixture +def hand_saved(project) -> Path: + """`project` with that shape pin committed on `main`.""" + pin = project / SHAPE_PIN + pin.write_bytes(MARK + COMMENT + pin.read_bytes()) + git("add", "--", SHAPE_PIN, cwd=project) + commit_in(project, "the shape pin, saved by hand") + return project + + +def pinned_paths(root: Path) -> list[str]: + """The `files:` paths of `root`'s shape pin, as `load_yaml` reads it.""" + return [str(row["path"]) for row in load_yaml(root / SHAPE_PIN)["files"]] + + +def test_committed_pin_reads_the_commit_load_yaml_reads(hand_saved, tools): + committed = tools["update-shape"]._committed_pin(hand_saved, SHAPE_PIN) + assert committed == load_yaml(hand_saved / SHAPE_PIN)["commit"] + + +def test_pin_at_reads_the_rows_load_yaml_reads(hand_saved, tools): + doctor_module = tools["shape-doctor"] + rows = doctor_module.pin_at(doctor_module.Context(hand_saved), "HEAD") + assert rows == pinned_paths(hand_saved) + + +def test_pinned_shape_paths_reads_the_rows_load_yaml_reads(hand_saved, + tools): + blob = git("rev-parse", f"HEAD:{SHAPE_PIN}", cwd=hand_saved).stdout + rows = tools["adopt-project"]._pinned_shape_paths(hand_saved, + blob.strip()) + assert rows == pinned_paths(hand_saved) + + +def test_pin_values_in_reads_the_leg_pin_load_yaml_reads(project): + """As `bump-leg.py` hands it the text of a leg pin it is to rewrite.""" + pin = project / SPEC_PIN + pin.write_bytes(MARK + COMMENT + pin.read_bytes()) + read = load_yaml(pin) + assert shape_materialize.pin_values_in(pin.read_text(encoding="utf-8")) \ + == (read["commit"], read["digests"]["tree_sha256"]) + + +def test_the_doctor_puts_back_a_hand_saved_pin_from_head_after_a_re_pin( + standard, hand_saved, tmp_path): + """THE REGRESSION ROW. `update-shape.py apply` re-pins the hand-saved + pin onto a newer standard, keeping the mark and the comment, and the + validators read it. The working pin is then broken. Its NEXT puts back + HEAD's pin, the one every validator reads; with the mark taken off in + `load_yaml` alone it was `checkout '~1'`: the + pin from before the hand-save, naming the commit the re-pin moved it + off.""" + moved = tmp_path / "moved" / "openRepoShape" + moved.parent.mkdir() + git("clone", "-q", str(standard), str(moved), cwd=tmp_path) + target = moved_on(moved, "c2", UPSTREAM_TARGET) + apply_on(moved, hand_saved, target, "shape/re-pin") + pin = hand_saved / SHAPE_PIN + pin.write_bytes(pin.read_bytes() + b"files: [\n") + row = rows_of(doctor(moved, hand_saved, "--json"))["shape-currency"] + assert row["detail"]["restore"] == "uncommitted", row["detail"] + assert f"checkout HEAD -- {SHAPE_PIN}" in row["next"], row["next"]