Run the redraw gate in CI, and take the chart profile off host.json - #79
Merged
Merged
Conversation
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
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.
Closes #75, though not the way that issue describes it. The first half of it is already done and has been for a while:
chart --manifestwritesgraphs.sha256, and both committed manifests verify. What was missing is the half the issue itself says matters, which is that nothing runs the check.CI now redraws every chart in every published results directory from the committed
output.jsonand compares SHA-256 against the committed manifest. That is M8's exit gate as a job rather than as a sentence, and it is the only check here that can see a change to the chart code quietly moving a pixel. It goes in thegeneratedjob, besidedocs --check, because it is the same kind of claim: the file in the tree is the file the generator would write.The issue worried that byte for byte reproducibility across platforms might not be free. It is, so far. Both directories were drawn on macOS and both verify on this machine, and the CI runner is Linux, so the first run of this job is also the first cross platform check. If it fails, that is worth knowing now rather than on the day somebody disputes a bar.
Getting there found a footgun.
chart --dir X --check X/graphs.sha256with no--profiledraws 146 unstamped charts and reports that all 146 of them do not match, because the stamp along the bottom is part of the picture. The output says the charts are not reproducible when what happened is that an argument was left off, which is the worst possible thing for a determinism check to say. A results directory names its own profile inhost.json, so that is where it comes from now when--profileis absent, and the CI job needs no per directory argument.--profilestill wins where it is given, and the golden series, which has no directory, still draws unstamped.A test asserts that every published directory is named for the profile its
host.jsonrecords. Those two disagreeing would put one name on the directory and another on the charts inside it, and the manifest would stop verifying for anybody who passed the name they could see.