Conversation
The OSv edits to the vendored OpenZFS tree are applied by a $(shell ...) that
sends git apply stderr to /dev/null. Two properties of that line compound
badly.
git apply is not all-or-nothing across several patch files in one invocation:
it can modify the files named by the earlier patches and then reject a later
one, leaving a partially patched tree. git apply does report this correctly
with a non-zero status, and the "&& touch" means the stamp file is correctly
not written, but the exit status of a $(shell ...) is invisible to make, so
nothing stops the build, and with the diagnostic discarded nothing reports it
either. make simply proceeds to compile whatever the failed apply left
behind: an OpenZFS built with only some of the OSv platform edits. That
builds cleanly and then fails at run time, with nothing in the build output
pointing back at the cause.
Because the stamp is absent, the next make retries the apply against the
now-dirty tree, where it fails differently ("patch does not apply"), so the
tree stays broken until the submodule is reset by hand.
Keep the diagnostic on stderr and require an explicit sentinel from the
recipe, so the apply and the stamp both have to succeed before the build goes
on; otherwise fail with the message and how to restore the submodule. The
apply is still skipped when the stamp exists, when the submodule is not
checked out, and now also when the patch directory is empty, which git apply
would otherwise reject.
master carries a single patch today, so the partial-apply window is narrow;
the point of the guard is that it makes extending the series safe and turns a
silent wrong build into a build failure that says what happened.
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
The OSv edits to the vendored OpenZFS tree are applied from a
$(shell ...)nearthe top of the Makefile:
Two properties of that line compound into a silent wrong build.
git applyis not all-or-nothing across several patch files in oneinvocation. It can modify the files named by the earlier patches and then
reject a later one, leaving a partially patched tree.
A
$(shell ...)exit status is invisible to make.git applydoes reportthe failure correctly with a non-zero status, and because of the
&&the stampfile is correctly not written, so neither of those is the defect. The defect is
that nothing acts on the failure: make cannot see the status, and
2>/dev/nulldiscards the only other evidence. make proceeds to compile whatever the failed
apply left behind, which is an OpenZFS built with only some of the OSv platform
edits. It compiles cleanly and fails at run time, with nothing in the build
output pointing back at the cause.
Since the stamp is absent, the next
makeretries the apply against thenow-dirty tree, where it fails a second way ("patch does not apply"), so the tree
stays broken until the submodule is reset by hand.
The change
Keep
git apply's diagnostic instead of discarding it, and require an explicitsentinel from the recipe so that the apply and the stamp both have to succeed
before the build continues. Otherwise
$(error ...)with the captured messageand the command that restores the submodule.
The apply is still skipped when the stamp exists and when the submodule is not
checked out. It is additionally skipped when the patch directory is empty, which
git applywould otherwise reject with "No valid patches in input" and whichwould have turned into a spurious build failure.
Verified behaviour
Exercised against the real submodule at the pinned tag, and against a scratch
repository for the cases that need a deliberately broken patch:
In the failing case before this change,
makereturned 0 and went on to thecompile step with the submodule left modified. After it:
conf_zfs=bsd(the default) does not reach this code and is unaffected.Motivation
master carries a single patch file today, so the partial-apply window is narrow
right now. The value of the guard is that it makes extending the series safe, and
that it converts a silent wrong build into a build failure that says what
happened. It is a small guard on a step whose failure is otherwise invisible.
One file.