Run R CMD check in CI on three platforms - #67
Merged
Merged
Conversation
Phases approved at the plan gate, with two decisions taken there: a three-platform matrix at R release (the GDAL axis is what varies for a package calling sf::gdal_utils), and clearing the pre-existing non-ASCII WARNING in this PR rather than landing a gate relaxed to error-on "error". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019MapqNGo4kVXGBuQNHqVdH
0 errors, 1 warning, 1 note, 243 s, against the tree as committed and with the action's own defaults rather than devtools' -- so the first CI run holds no surprises and Phase 2 fixes what is there rather than what was predicted. Both planning predictions were wrong, and both are recorded as wrong rather than quietly re-pointed. The non-ASCII WARNING names one file, not three: R/fly_footprint.R and R/fly_georef.R already carry eleven — escapes, and the probe that named them parsed and deparsed, which renders — back as a literal em dash -- it measured its own un-escaping and reported the fix as the defect. What the check runs is tools:::.check_package_ASCII_code(dir, FALSE), line-based over the raw file, and that is the probe to use for this class. The predicted utils/stats undeclared-import NOTE does not exist -- they are base packages and need no Imports entry -- so the DESCRIPTION edit it implied is struck from Phase 2 rather than made for no reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019MapqNGo4kVXGBuQNHqVdH
.github/workflows/ held only pkgdown, so a green PR check meant the docs site built -- not that the package checks, and not that the suite runs anywhere but the author's machine. Three platforms at R release. The platform axis is what earns the runner time: fly reaches GDAL through sf::gdal_utils() for warp, translate and nearblack, and fly_mask() asks for -alg floodfill, which GDAL's own nearblack.rst marks versionadded:: 3.8 -- and the three runners take GDAL from three different sources. devel and oldrel-1 are absent by decision: fly is not going to CRAN and oldrel-1 is R 4.4, far above the declared Depends: R (>= 4.1). Committed BEFORE the fix that clears the one pre-existing WARNING, so the PR's first run goes red on it and the next goes green. A gate nobody has seen reject anything is indistinguishable from one that cannot. Also here: the plan review and three code-review rounds, with what each found. Round 1 found a defect inside a fix -- a concurrency group carrying no matrix value, which would have made the three runners cancel each other and read as "cancelled" rather than "failure" -- so the loop ended on enumeration rather than on a quiet round. Six of twenty-six claims across three populations were wrong, including a GDAL floor of 3.7 taken from this repo's own conventions where upstream says 3.8: a guard that would have passed on a build with no -alg flag at all. Filed as soul#255. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019MapqNGo4kVXGBuQNHqVdH
R/fly_mask.R carried a raw em dash in a warning message, which is the whole of the WARNING the previous commit's CI run went red on across all three platforms. — is the remedy R CMD check names and the form R/fly_footprint.R and R/fly_georef.R already use eleven times between them. The rendered message is byte-identical, verified against the parsed string value rather than a deparse() -- deparse() re-escapes non-ASCII, so grepping one for a literal em dash reports FALSE after the fix and reads as a change that did not happen. test-fly_mask.R greps "flood into the image" from this same string, ahead of the dash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019MapqNGo4kVXGBuQNHqVdH
A gate that reddens on a third party's outage teaches people to ignore it, which is fly#52's own complaint in a new form. Seven blocks in test-fly_georef.R fetched thumbnails with no guard at all, four asserting result$success directly on a fresh download. Measured by routing HTTP at a dead local port -- so DNS still resolves and skip_if_offline() passes, isolating the fetch guard -- with the pre-change file pulled from git rather than reconstructed: FAIL 11 before, FAIL 0 / SKIP 9 after. All seven now use the pattern the two #49-era blocks below them already did. The same edit moves fourteen shared fixed tempdir() names to withr::local_tempdir(). unlink() fails SILENTLY on Windows with a handle open, and fly_fetch() then serves the stale file as a cache hit. Two defects the first CI run found, neither reachable from this machine, which is the argument for the three-platform matrix made by the matrix rather than by anyone's reasoning: * test-fly_footprint.R's anti-vacuity premise asserts the pre-fly#26 recomputed ring close still fails somewhere in a 720-bearing sweep. That is a property of the BLAS: 342 of 720 bearings differ here, 0 of 720 on all three runners, where asserting it took the check red. Split into its own block that skips when the defect is unreachable and asserts when it is not, with the skip printed by the new skip-report step rather than being silent. * Windows yields 2 bands for a masked grayscale frame against 1 with masking off, contradicting the unconditional "output band counts do not change" in CLAUDE.md and inst/notes/border-masking.md -- the stated reason masking could default to on without moving stac_airphoto_bc. Filed as fly#68. The observed Windows value is PINNED rather than skipped, so nothing goes unasserted and the test reddens if that platform moves in either direction, including the direction where #68 is fixed. Also: a GDAL >= 3.8 premise in test-fly_mask.R, because fly_mask() passes -alg floodfill and swallows the failure -- an old GDAL writes unmasked output and passes the examples. The floor is read from GDAL's own nearblack.rst; this repo's conventions say 3.7, which would have passed on a build with no -alg at all (soul#255). A pre-release suffix is stripped before parsing, since package_version() errors on "3.9.0beta1". A premise on the mtime block, which compared NA to NA when nothing was written. And the README badge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019MapqNGo4kVXGBuQNHqVdH
Three platforms green: run 35681274310, Status: OK on each, slowest Windows at 7m39s. The skip report is non-empty and identical everywhere -- two skips, skip_on_ci() and the BLAS premise -- and carries no skip_if_offline(), so the network tests genuinely ran rather than quietly standing down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019MapqNGo4kVXGBuQNHqVdH
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 #52.
.github/workflows/held onlypkgdown.yaml, so a green PR check meant the docs sitebuilt.
R CMD checkhad never run anywhere but the author's machine, and neither had thetest suite.
What landed
R CMD checkon ubuntu, macOS and Windows at R release, on every push tomainandevery PR, gating at the r-lib action's default
error-on: "warning".Three platforms, and the platform axis is the one that earns the runner time.
flyreaches GDAL through
sf::gdal_utils()for warp, translate andnearblack, andfly_mask()asks for-alg floodfill— which GDAL's ownnearblack.rstmarksversionadded:: 3.8. The three runners take GDAL from three different sources, andtest-fly_mask.Rsynthesizes its own images withterra, so it exercises that calloffline on all three. R
develandoldrel-1are deliberately absent:flyis not goingto CRAN, and
oldrel-1is R 4.4, far above the declaredDepends: R (>= 4.1).The gate sits at "warning" because the one pre-existing WARNING is cleared here.
R/fly_mask.Rcarried a raw em dash in a warning message; it is now—, the remedyR CMD checknames and the formR/fly_footprint.RandR/fly_georef.Ralready useeleven times between them. The rendered message is byte-identical, verified against the
parsed string value rather than a
deparse(). The alternative waserror-on: "error",which passes with a WARNING present — that is what makes it tempting and what makes it
weak, since it would also have passed over the next one. gq#51 took that route and is
still open.
A "Report skipped tests" step. The action already cats the raw testthat output, so
this step's job is narrower than a first draft claimed: the skip block on its own rather
than buried in a collapsed dump, and a failure when the output cannot be read at all,
where the action's version ends in
|| true. All four of its branches were driven againstfixture trees, including the one that must fail.
The blocker a review found, and the measurement that settled it
A concurrent plan review found that the workflow's own comment asserted a safety property
the package did not have. Seven blocks in
test-fly_georef.Rfetched thumbnails from theBC Data Catalogue with no guard at all, four of them asserting
result$successdirectly on a fresh download.
Measured by routing HTTP at a dead local port — so DNS still resolves and
skip_if_offline()passes, isolating the fetch guard as the thing under test — with thepre-change file pulled from git rather than reconstructed:
test-fly_georef.RSo a hiccup at the catalogue would have reddened three runners on an unrelated diff, which
is how a gate teaches people to ignore it — #52's own complaint in a new form. All seven
now carry
skip_if_offline()+skip_if_not(all(fetched$success)), the pattern the two#49-era blocks below them already used.
The same edit moves fourteen shared fixed
tempdir()names towithr::local_tempdir():unlink()fails silently on Windows when a handle is open, andfly_fetch()thenserves the stale file as a cache hit.
Measured, not assumed
rcmdcheckwith the action's exact inputs and the two env vars the action forces —_R_CHECK_CRAN_INCOMING_=false,_R_CHECK_FORCE_SUGGESTS_=false— gives 0 errors,0 warnings, 0 notes in 207 s. Suite: FAIL 0, PASS 2144. A bare local
rcmdcheck()without those env vars is measuring a different check, and reports a CRAN-incoming NOTE
this workflow will never see.
The commits are ordered workflow-first on purpose. The WARNING fix comes second, so
this PR's first run goes red on the real pre-existing WARNING and the next goes green. A
gate nobody has seen reject anything is indistinguishable from one that cannot.
Six claims made along the way were wrong
Recorded as wrong in
planning/, not quietly re-pointed:each file and ran
deparse()over the parse tree — which renders—back as aliteral em dash, so it measured its own un-escaping and reported the existing fix as the
defect. The probe that agrees with the check is
tools:::.check_package_ASCII_code(dir, FALSE).utils/statsundeclared-import NOTE does not exist — they are basepackages. The
DESCRIPTIONedit it implied was struck rather than made for no reason.concurrencygroup carried no matrix value, so all three runners shared onegroup and
cancel-in-progresswould have made them cancel each other — and acancelled run reads as
cancelled, notfailure, so two platforms would simply neverhave reported. That defect sat inside a fix for an earlier review finding.
error-onsemantics.>= 3.7, the figure this repo's own conventions carry.Upstream says 3.8, so the guard would have passed on a build with no
-algflag atall — failing toward pass on precisely the release it exists to catch. Filed as
NewGraphEnvironment/soul#255.
package_version()errors on a pre-release GDAL string ("3.9.0beta1","3.11.0-dev"), so the premise would have reddened a correct package on such a runner.The suffix is now stripped and the strip's own output asserted.
Six of twenty-six claims, across three populations enumerated by count. None was reachable
by reading the files again. The review loop ended on that enumeration rather than on a
quiet round, which is the rule once a defect has been found inside a fix.
Adjacent, filed rather than folded in
.Rbuildignorehas no^\.git$, so a build from a worktree ships an absolutedeveloper path. Reviewed and argued for during this work; it was scoped out at the plan
gate, so it is filed rather than slipped in. The issue carries the reviewer's point that
CI is structurally incapable of catching it, and that the assertion the conventions
prescribe has a placement problem of its own.
that reaches every repo.
test-fly_fetch.Ris vacuous under an outage (FAIL 0, SKIP 0,PASS 13 measured). Pre-existing, out of scope, written down so it is not rediscovered as
a surprise.
What the first CI run found
Run 35680424973, on
the workflow-only commit, went red on the non-ASCII WARNING across all three platforms —
the gate demonstrated on a runner rather than asserted locally.
It also found two pre-existing defects, neither reachable from this machine. That is
the argument for the three-platform matrix, made by the matrix on its first run rather than
by anyone's reasoning about it:
test-fly_footprint.R's anti-vacuity premise is a property of the BLAS. It assertsthe pre-Bearing rotation for diagonal flight lines — film half (resolved: the mapping is per-roll, not a constant) #26 recomputed ring close still fails somewhere in a 720-bearing sweep, so the
test cannot quietly become decoration. Measured: 342 of 720 bearings differ on this
machine, 0 of 720 on all three runners — where asserting it took the whole check red.
Split into its own block that skips when the defect is unreachable and asserts when it is
not, with the skip printed by the new skip-report step rather than being silent.
2 bands against 1 with masking off; ubuntu and macOS both give 1.
CLAUDE.md's KeyDecision that "output band counts do not change" — the stated reason masking could
default to on without moving
stac_airphoto_bc— was measured on one platform andwritten down unconditionally. Filed as Masking adds a band on Windows for grayscale, so "output band counts do not change" is platform-conditional #68. The observed Windows value is pinned
rather than skipped, so nothing goes unasserted and the test reddens if that platform
moves in either direction, including the direction where Masking adds a band on Windows for grayscale, so "output band counts do not change" is platform-conditional #68 is fixed.