fix(quarto-citeproc): stop false CSL lockfile mismatches#380
Merged
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Member
Author
|
Retrying, but I think those CI errors are from main too |
csl_validate_manifest was generated by build.rs, which baked an "expected"
lockfile string at build-script run time and compared it against the live
tests/csl_conformance.lock at test-run time. If Cargo ever failed to rerun
build.rs when it should have, the baked value went stale relative to disk,
producing a false "Lockfile mismatch".
Investigation ruled out both directory-rerun-if-changed theories that were
suspected as the cause: add/remove detection was confirmed to work correctly
on the cargo version this repo pins (1.97.0-nightly, well past cargo#8973),
and the baked value never depended on fixture content in the first place, so
neither theory actually explains the failure. Rather than patch a mechanism
that was never proven broken, the check now reads test-data/csl-suite/ and
enabled_tests.txt live at test-run time via env!("CARGO_MANIFEST_DIR") - a
compile-time path constant, not a baked computed value - eliminating the
time-of-check/time-of-use gap structurally instead of trying to make
rerun-if-changed perfect.
build.rs keeps only per-fixture test codegen, which this doesn't touch.
Duplicate/nonexistent-entry detection moves from a soft cargo:warning to a
hard test failure. UPDATE_CSL_LOCKFILE=1 auto-update is preserved byte-for-
byte since scripts/csl-test-helper.py depends on it in three places.
The staleness bug itself isn't regression-testable - Cargo's build-script
rerun timing can't be forced deterministically - so it's fixed architecturally
rather than by a reproducing test. Added unit tests for the extracted
comparison/formatting logic instead (red before implementation, green after).
csl_validate_manifest byte-compares tests/csl_conformance.lock against a generated LF-only string. With core.autocrlf=true and no .gitattributes, Windows checkouts get CRLF for that file, producing a false "Lockfile mismatch" unrelated to any real enabled_tests.txt/fixture divergence. Pins tests/csl_conformance.lock and tests/enabled_tests.txt to eol=lf, same pattern crates/pampa/.gitattributes already uses for its fixtures. eol=lf overrides core.autocrlf for these paths in both directions, so regenerating the lockfile from Windows stays safe too.
cderv
force-pushed
the
fix/csl-lockfile-runtime-check
branch
from
217f521 to
57b305e
Compare
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
2 participants
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.
When csl_validate_manifest ran after a rebase, it reported "Lockfile mismatch: tests/csl_conformance.lock" even though enabled_tests.txt and test-data/csl-suite/ were unchanged and regenerating the lockfile produced a byte-identical file.
Root Cause
Two independent bugs, both making the check flaky rather than authoritative:
csl_validate_manifestwas generated bybuild.rs, which baked an "expected" lockfile string into the compiled test binary at build-script run time and compared it against the live file at test-run time. If Cargo didn't rerunbuild.rswhen it should have, the baked value went stale relative to disk - a time-of-check/time-of-use gap.core.autocrlf=truesetting,tests/csl_conformance.lockgets checked out with CRLF line endings, while the generator that produces the "expected" string always emits LF-only. The raw byte compare then fails on line endings alone, with no real content divergence -crates/quarto-citeprochad no.gitattributespinning the file, unlikecrates/pampa, which already pins its fixtures the same way.Fix
csl_validate_manifestis now a hand-written runtime test intests/integration/csl_conformance.rsthat readstest-data/csl-suite/andenabled_tests.txtlive at test-run time viaenv!("CARGO_MANIFEST_DIR")- a compile-time path constant, not a baked computed value - and compares against the lockfile in the same instant.build.rskeeps only per-fixture test codegen. Duplicate/nonexistent-entry detection moves from a softcargo:warningto a hard test failure.UPDATE_CSL_LOCKFILE=1auto-update behavior is unchanged.crates/quarto-citeproc/.gitattributesnow pinstests/csl_conformance.lockandtests/enabled_tests.txttoeol=lf, matching the existingcrates/pampa/.gitattributesconvention. This overridescore.autocrlffor these two paths in both directions, so regenerating the lockfile from Windows stays safe too.