fix: two files shipped in 0.3.0 that cannot compile from a tarball - #62
Merged
Conversation
`ytsaurus-skiff/tests/wire.rs` and `ytsaurus-job/tests/skiff_reader_tests.rs`
`include_str!` reference vectors from `tests/skiff-go-interop/`, which is
outside their crates. Those macros resolve at compile time, so both files
compile in this repository and cannot compile from the published `.crate` — the
fixtures are not in the tarball and no consumer can put them there. Verified
against the actual artifacts downloaded from crates.io: of the nine published,
`ytsaurus-skiff` and `ytsaurus-job` are the two whose `cargo test --no-run`
fails.
Consumers are unaffected: cargo does not build a dependency's tests. It bites
whoever unpacks the crate and runs its suite, and vendoring or packaging
workflows that do the same.
Four files of this kind were found and excluded before 0.3.0 went out. These two
were missed, and the reason is worth recording: the search was a line-based
grep for
include_str!("../../../tests/…
which does not match
hex_fixture(include_str!(
"../../../tests/…"
))
— the same macro with a newline in it. The audit that caught the other four did
not catch these either.
So `scripts/check-package-includes.sh` replaces the grep. It asks cargo which
files actually ship, parses each with a multi-line-aware regex, resolves every
include target and fails if one escapes its crate root. It runs in CI, and it
was checked to fail: re-including `tests/wire.rs` makes it exit 1 naming the
file and the three fixtures it reaches.
0.3.0 is published and cannot be replaced. This lands the fix so it is correct
from here, and so nothing else ships with it.
📊 Criterion benchmarks: main vs PR🔍 Main 📊 25 benchmark(s) compared · ℹ️ Time is lower-is-better. Both revisions ran on the same GitHub-hosted VM; ±20% is an advisory marker, not a merge gate.
|
`ytsaurus-skiff` and `ytsaurus-job` needed the fix in the commit before this one, and the workspace version is one version — "they move as one" is stated in the README, in AGENTS.md and in every changelog here, and giving two crates a private version to avoid a patch bump would start exactly the kind of drift between documentation and reality that 0.3.0 spent its whole diff correcting. So all nine go to 0.3.1. Seven of them record it as what it is: no change beyond the version. The two that were broken say what was broken, that a consumer depending on the crate was never affected, and why the check that was supposed to catch it did not. AGENTS.md's shipped section now carries the same account, next to the note about 0.2.6 never having been released.
It was a bash entrypoint wrapping a Python heredoc — two languages to read for one job, and the Python half was a string as far as any tool was concerned: not linted by ruff, not formatted, no syntax check until CI ran it. Now it is `scripts/check_package_includes.py`, plain stdlib, invoked directly. ruff lints and formats it with the rest, which is what `pyproject.toml` exists for. It is still checked to fail: re-including `tests/skiff_reader_tests.rs` makes it exit 1 naming the file and the fixture it reaches. The count of Python files was stated as four in three places and is now five.
Automation here was bash, Python, JavaScript and Rust, with the largest pieces of it inlined into workflow YAML where no linter, formatter or type checker could see them. The rule is now written down in AGENTS.md Fixed decisions: Python for anything that computes, parses or asserts; bash for glue only, with no source of another language embedded in it and none of it inlined into a workflow; Rust only where genuinely required; Go only in the interop tests. **JavaScript is gone.** `scripts/compare-criterion.mjs` was the one file outside every mechanism this repository has — no lint, no format, no test, and Node inherited unpinned from the runner image. Its port is byte-identical, and that is verified rather than asserted: a fixture covering an improvement, a regression past the threshold, one under it, a new benchmark, a removed one and the rounding boundary renders identically under both, and all three error paths produce the same message and exit code. Two pieces of JavaScript arithmetic are load-bearing and are reproduced rather than tidied: `toFixed` strips the sign before rounding, so a tie goes away from zero and `-6.25` prints `-6.3` where Python's banker's rounding gives `-6.2`; and `localeCompare` is ICU collation, not codepoint order. The *rounded* value is what gets classified against the 20% threshold, so either would have silently changed a published report. The `actions/github-script` block went with it — 27 lines of JavaScript that found the previous benchmark comment and edited it. `gh` is authenticated from GH_TOKEN exactly as that action's client was. The matching stays on the report's HTML marker and deliberately not on the comment's author, which is what `gh pr comment --edit-last` does: a second bot on the same pull request would silently steal the edit. Verified against the live API, read-only — it finds the existing marked comments on #61 and #62, so it will edit them rather than post duplicates. Two assertions came out of ci.yml. The worker-graph check and the static-linkage check were 38 lines of shell between them, and the shell version of the second one had the failure mode this repository keeps meeting: a glob matching nothing walks its loop zero times and reports success. Both are now scripts that were checked to fail — the static one against stubbed `file`/`ldd` in all three states (no binaries, static, dynamic), the graph one by adding a crate that is present to the forbidden list. The five Python heredocs inside the cluster-e2e shell scripts are real files now. Two of them were `python3 -c "` with double quotes, so the shell expanded `$` inside what was meant to be Python source — no live bug, one edit away from one. Type checking is `mypy --strict`, not `ty`. ty is astral's, like ruff and uv, and would keep the toolchain to one vendor, but it is preview and a checker whose diagnostics move between releases is a poor thing to gate CI on; on the same two files mypy found a real gap that ty did not. The reasoning is in pyproject.toml so the next person does not have to re-derive it. Four helpers written before the rule needed annotating to pass; each was checked for semantic inertness, and the committed fixtures two of them generate are byte-identical afterwards. `scripts/check_package_includes.py`, added yesterday, had the vacuity bug itself: a package whose `cargo package --list` failed was skipped with a warning and counted as clean. In the script written to stop a defect that reached crates.io twice. It is fatal now, and that was verified. Also corrected: AGENTS.md said the cluster end-to-end run was not in CI, and it has been on every push to main since e5b5198.
ruff lints, uv runs, and now ty type-checks — all three astral, one binary each and one place to look when a version moves. mypy --strict was in place first and was equally clean; the switch is for that consistency, not because either found something the other missed. Verified on the same injected return-type error: both report it. The two untyped third-party imports are suppressed per import line, with `# ty: ignore[unresolved-import]` — `yt`, the official YTsaurus Python client, and `yt_yson_bindings`, a compiled C extension over upstream's library/cpp/skiff. Neither ships type information and neither will. Turning `unresolved-import` off in pyproject.toml would have been one line and would also have hidden a typo in a first-party import; six narrow comments cannot. Four `# type: ignore[call-arg]` comments went away with mypy. They existed because mypy does not run class decorators and so saw `@yt_dataclass` types as having no constructor at all; ty resolves the decorator and needs nothing. The comment that explained them now says which construct to look at if the checker is ever changed again, rather than describing a checker that is no longer here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: two files shipped in 0.3.0 that cannot compile from a tarball
ytsaurus-skiff/tests/wire.rsandytsaurus-job/tests/skiff_reader_tests.rsinclude_str!reference vectors fromtests/skiff-go-interop/, which isoutside their crates. Those macros resolve at compile time, so both files
compile in this repository and cannot compile from the published
.crate— thefixtures are not in the tarball and no consumer can put them there. Verified
against the actual artifacts downloaded from crates.io: of the nine published,
ytsaurus-skiffandytsaurus-jobare the two whosecargo test --no-runfails.
Consumers are unaffected: cargo does not build a dependency's tests. It bites
whoever unpacks the crate and runs its suite, and vendoring or packaging
workflows that do the same.
Four files of this kind were found and excluded before 0.3.0 went out. These two
were missed, and the reason is worth recording: the search was a line-based
grep for
which does not match
— the same macro with a newline in it. The audit that caught the other four did
not catch these either.
So
scripts/check-package-includes.shreplaces the grep. It asks cargo whichfiles actually ship, parses each with a multi-line-aware regex, resolves every
include target and fails if one escapes its crate root. It runs in CI, and it
was checked to fail: re-including
tests/wire.rsmakes it exit 1 naming thefile and the three fixtures it reaches.
0.3.0 is published and cannot be replaced. This lands the fix so it is correct
from here, and so nothing else ships with it.