fix(verbs): measure and render refuse an absent build input too - #389
Merged
Conversation
check has refused a build whose depfile names a file that is not on disk since #354, but that fix was scoped to runner.py. The same part was still measured and still drawn: measure printed the volume of a bare plate at exit 0, render wrote four PNGs of it. The two verbs did not come along because they are guarded on a different signal. Both key on the engine's stderr markers, and an import() of an absent target emits none -- it renders as nothing, so the mesh is well-formed and there was nothing for those guards to see. All three verbs now refuse on one shared answer rather than each deriving its own, which is #308's rule applied to the evidence and not only to the wording. That shared answer includes the narrowing, and the narrowing is the half that matters. A file named only from a % subtree reaches engine_inputs.missing exactly as a real dependency does, so refusing on that field alone -- the smallest fix as #355 states it -- exits 4 on a correct part. Measured: `cube(...); %import("gone.stl");` and `cube(...); import("gone.stl");` produce identical missing entries, both measure 7200.0, and only the second is a fault. Two guard sites in measure, not one, because it has two build paths. The --out FILE form must refuse before its rename or it overwrites the caller's file with the hollow artifact and then declines to measure it (round-2 review of #306). render refuses before any view moves, so a previous run's PNGs are untouched. SPEC-report 6.1 widened: the sibling-payload paragraph was deliberately scoped to what #354 implemented, and closing this is what makes the wider statement true. Closes #355
CI's mesh-only job runs apt 2021.01 with no display, which has no EGL offscreen path, so every render exits 4 with "cannot render PNG without a display" -- an environment fault with nothing to do with this issue. The first draft read only the exit code. That failed the three rows that expect 0, and, worse, PASSED the four refusing rows for the wrong reason: they exit 4 there whether or not the guard exists. Both verbs now assert the refusal names the absent input, and render skips where the engine cannot draw at all. The refusing rows still run without a display, because the refusal lands before any view is rendered -- which is the guard's placement, asserted from the other side.
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.
checkhas refused a build whose depfile names a file that is not on disk since #354,but that fix was scoped to
runner.py. The same part was still measured and stilldrawn.
Reproduced on both pinned engines:
checkmeasurerenderimport("missing.stl")volume: 7200.07200.0is a bare 40x30x6 plate — the import contributed nothing, which is the wholepoint. An agent measuring that number writes it into a contract that then passes forever.
The two verbs did not come along because they guard on a different signal: both key on
the engine's stderr markers, and an
import()of an absent target emits none. It rendersas nothing, the mesh is well-formed, and those guards had nothing to see.
The issue's smallest fix would have refused a correct part
#355 says to refuse when
stl_deps[-1].missingis non-empty. That is not safe, andthe repo already knows why: OpenSCAD evaluates a
%subtree, so a file named only fromone reaches
engine_inputs.missingexactly as a real dependency does.checkfiltersthrough
_only_in_dropped_subtreesfor precisely this (#354 review, B1).Measured before writing anything:
missingmeasurevolumecube(...); import("gone.stl");gone.stlcube(...); %import("gone.stl");gone.stl%-ed subtree is not part of the partIdentical evidence, opposite verdicts. The naive guard exits
4on the second — the samefault as the one being fixed, with the sign reversed.
So the narrowing moved into a shared
runner.absent_build_inputs()and all three verbsread it. That is #355's own step 3 ("one engine fact is not diagnosed three ways") applied
to the evidence as well as the wording. It costs nothing on the ordinary path:
_only_in_dropped_subtreesreturns immediately when nothing is missing, so the extraengine pass is paid only where a build input is already known absent.
Two guard sites in
measure, not onemeasurehas two build paths. The--out FILEform must refuse inside_build_to_file, before its rename — a refusal asked afterwards replaces the caller'sgood artifact with the hollowed one and then declines to measure it, which is the round-2
review finding on #306. The directory form is guarded at the shared post-build point.
renderrefuses before any view moves, so a previous run's four PNGs are byte-for-byteuntouched — verified, 0 PNGs written on refusal.
After
checkmeasurerenderimport("missing.stl")%import("missing.stl")Tests
15 added, parametrized over the same seven modifier shapes
checkalready uses, so the"all three verbs agree" claim is the thing under test rather than a comment. With the two
verb guards reverted and the tests kept, 9 go red and 6 stay green — and the 6 are
exactly the
%/*rows across both verbs, the false-refusal controls.One more asserts the three verbs share cause, hint and
origin=None, so a reader who metthis through one verb recognises it through another (#308's rule).
just checkclean,1393 passed(baseline 1378).Docs
SPEC-report.md§6.1's sibling-payload paragraph was written to describe only what #354implemented and said so, naming this issue. It now states the wider rule, plus the
requirement that the narrowing be applied identically by every verb.
Closes #355