fix(cmd/morphic)!: stop a failed -o write masking exit 1 - #371
Open
OmarAlJarrah wants to merge 1 commit into
Open
fix(cmd/morphic)!: stop a failed -o write masking exit 1#371OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
Conversation
compile let whichever failure came last decide its exit code, so a run whose diagnostics reached the --fail-on threshold returned 1 when -o could be written and 2 when it could not. Whether a destination can be written is a property of the destination, not of the spec: -o publishes by rename, and both /dev/null and a read-only directory holding a writable file refuse the temp file that needs. The same spec with the same diagnostics therefore reported a different code depending on where -o pointed, and a caller reading the code alone could not tell a spec that failed the gate from a destination it could not write. The diagnostics' verdict now wins. A run that reached the threshold exits 1 wherever -o pointed, and 2 is left to mean a run that failed for a reason outside the spec. The write error still prints to stderr either way. Writing through to a destination that cannot be published by rename, so that -o /dev/null works at all, is deliberately not done here: it would honour what a name points at rather than replacing the name, which is the trade replaceFile already declines for symlinks and hard links; opening a reader-less FIFO for writing blocks indefinitely; and it reaches into the exit-code taxonomy that is still being settled. It would also not fix this bug, which is not specific to those destinations — a read-only directory holding a writable regular file masks the exit code identically. BREAKING CHANGE: morphic compile over a spec that reaches the --fail-on threshold now exits 1, not 2, when the -o destination cannot be written. A caller that read 2 as "no output was produced" must read stderr for the write error instead.
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.
Summary
morphic compilelet whichever failure came last decide its exit code. A run whose diagnosticsreached the
--fail-onthreshold returned1when-ocould be written and2when it couldnot, so the same spec with the same diagnostics reported a different code depending only on where
-opointed:A caller reading the code alone could not tell a spec that failed the gate from a destination it
could not write.
The diagnostics' verdict now wins. A run that reached the threshold exits
1wherever-opointed;
2is left to mean a run that failed for a reason outside the spec. The write error stillprints to stderr either way, so nothing is hidden from a human. README's exit-code paragraph now
says which wins, and
replaceFile's doc comment records why the write itself is left alone.After:
Why the exit code and not the write
The other direction on the table was to recognise destinations that cannot be published by rename
and write through to them directly, making
-o /dev/nullwork. That is deliberately not done here,for three reasons:
read-only directory holding a writable regular file masks the exit code identically (captured
above), and that case is pinned as deliberate by
TestWriteParsed_ReadOnlyDirFails. Recognisingnon-regular destinations cannot reach it. Fixing the precedence fixes every shape at once,
including the ones nobody has enumerated yet — a full disk, a name too long for a temp suffix.
main: a FIFO in a writabledirectory does not fail today — the temp file is created beside it and the rename replaces the
FIFO with a regular file, the same class as the symlink and hard-link cases
replaceFiledocuments and three tests pin ("each is a way to reach the destination's bytes other than
through its own name"). Writing through to it is a reversal of that contract, not a bug fix.
open(fifo, O_WRONLY)on a reader-lessFIFO does not return.
-o some.fifowould hang instead of failing. Doing it safely needsO_NONBLOCKplus anEAGAINretry loop — a bounded-write problem, not an exit-code one.The issue also notes that direction interacts with the exit-code taxonomy #58 is still deciding
(spec problems become diagnostics at
1;2stays for I/O and programmer errors). This change leaves both codes' documented meanings exactly as written and only decideswhich wins when both fire, so it settles nothing on that thread's behalf.
-o /dev/nulltherefore still fails. That limitation is now recorded inreplaceFile's doccomment (where a reader asking "why can't I write there?" lands) and in README's CLI section
(where a user asking the same question lands), rather than only here.
Test plan
TestRunParse_WriteFailureDoesNotMaskDiagnostics— four flat rows over two destination shapes(a missing parent directory; a writable file inside a read-only directory) crossed with two
specs (one reaching the threshold, one clean), asserting
1and2respectively.Every row also asserts stderr carries
create output, so a row cannot pass because the run diedearlier. The fixture spec is chosen so it still lowers to a non-nil document: a spec that lowers
to nil returns
1before the write is attempted and would exercise nothing./dev/nullis covered by the captured runs above rather than by a test, on purpose: under a root/devthe temp file would be created and the rename would replace/dev/nullitself.Mutation check. Reverting
compileSpectoreturn 2and re-running reddens exactly the twothreshold rows, on both destination shapes:
The two clean-spec rows stay green, which is correct — they were always
2.Existing pins still pass unchanged:
TestRunParse_OutputCreateError(clean spec, unwritabledestination,
2) andTestWriteParsed_ReadOnlyDirFails.Full gate green:
gofmt,go vet ./...,golangci-lint run(0 issues),go build ./...,./scripts/check-coverage.sh(100% of 4946 statements).Breaking
morphic compileover a spec that reaches the--fail-onthreshold now exits1, not2, whenthe
-odestination cannot be written. A caller that read2as "no output was produced" mustread stderr for the write error instead.
2remains the code when the spec had nothing to reportat the threshold and the write failed.
Closes #308