ci: adopt the org reusable workflows, keep split-library-check - #212
Merged
Merged
Conversation
Replaces the hand-rolled R-CMD-check, test-coverage and pkgdown workflows with
thin callers on PredictiveEcology/actions, and adds the two pieces of shared
infrastructure this repo never picked up.
R-CMD-check 223 -> 132 lines
test-coverage 80 -> 30
pkgdown 48 -> 20
test-downstream 0 -> 31 (new)
revdeps 0 -> 49 (new)
Supersedes #206, which is stale: four of its five non-docs files are already
byte-identical on development, it carries 54 gitignored docs/ files, and its
caller had a single `uses:` job that would have deleted split-library-check.
split-library-check is preserved verbatim (only actions/checkout v5 -> v7).
It is the only CI coverage of the .libPaths() layout that produced the 11 CRAN
failures in 2.1.0, and it cannot move into the shared workflow: the caller env
does reach dependency resolution, so R_LIBS_USER could ride in as an
extra-config leg, but the "Verify the split-library layout" guard has no input
to hang on. Without that guard the leg silently decays into an ordinary
ubuntu/release run.
Customisations carried across:
* _R_CHECK_THINGS_IN_OTHER_DIRS_ via extra-env
* oldrel-2 on both OSes and oldrel-3 on Ubuntu via extra-config; windows
oldrel-3 stays deliberately absent (dep-install storms past GHA's 6h cap)
* the runLong leg, as a second ubuntu/release entry carrying
R_REQUIRE_RUN_LONG_CI. A duplicate by construction -- there is no way to
add env to a default leg -- and the right trade, since the gated tests
should not slow the leg every other job waits on.
* NOT_CRAN=true for coverage, which the shared workflow now sets itself
Behaviour change, documented in the file: the shared workflows have no
`[skip-ci]` guard. The hyphenated spelling is not one of GitHub's native skip
keywords, which is why it needed a manual `if:`. The guard survives on
split-library-check; the shared matrix responds to GitHub's own `[skip ci]`.
test-downstream covers SpaDES.core and SpaDES.project -- the only two PE
packages that declare a Require dependency. revdeps is workflow_dispatch only,
per the shared action's warning that revdep checks are too heavy for standard
runners.
Pins are @main throughout, stated in each file: a tag cannot change underneath
us but is exactly what broke SpaDES (install-spatial-deps@v0.1 apt-installing a
package name retired before noble).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NNLtKFXrNPN2XvuAjsy4Sy
Follows PredictiveEcology/actions#34, which adds both. revdeps.yaml drops from 49 lines to 24: the job harness it was carrying (checkout, geospatial system libraries, R, dependency install) is the same one quickPlot had written, so it belongs upstream rather than here. Adds the weekly schedule alongside workflow_dispatch. The [skip-ci] caveat in R-CMD-check.yaml is gone -- the shared workflows honour it now, so a thin caller no longer loses it. split-library-check, the one job we still own, moves from commits[0] to head_commit to match the shared guard: the tip of the push rather than the oldest commit in it. Depends on PredictiveEcology/actions#34 being merged; revdeps is workflow_dispatch/schedule only, so nothing here runs it before then. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NNLtKFXrNPN2XvuAjsy4Sy
A caller with no workflow_dispatch inputs gives a Run-workflow button with no options, so every manual run is stuck on the shared defaults. cranonly is the one worth varying: false additionally walks children and grandchildren via revdepcheck.extras, which is what picks up the PE downstreams that live on r-universe rather than CRAN. `inputs` is null on the scheduled run, hence the event_name guard -- the fallback repeats the same defaults the dispatch form shows rather than passing empty strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NNLtKFXrNPN2XvuAjsy4Sy
The coverage job moved from macOS to the shared workflow's ubuntu-latest,
and this test failed there on its first ever CI execution. It had never
run in CI before: the macOS runner has no dpkg-query so it always skipped,
and R CMD check skips it via skip_on_cran() because NOT_CRAN is unset.
Coverage is the only job that sets NOT_CRAN, so ubuntu coverage is the
first place both conditions hold.
The failure signature is `SUDO INVOKED: -s id` -- identical to what the
negative control produces. That is the problem: nothing in the test
verified that the protective run differed from the negative control in
the way it claims to. `pkgRoot` is normalizePath("."), the root found by
walking up from tests/testthat, which under covr is an instrumented copy.
If load_all does not resolve there, .onLoad never runs, PKG_SYSREQS is
never forced off, and the run is an unlabelled second negative control --
failing exactly as a real escalation regression would.
So the child now reports whether Require actually loaded and what
PKG_SYSREQS/PKG_SYSREQS_SUDO it ended up with, and the test asserts both
before asserting on the trap. A genuine regression and a broken fixture
now produce different, named failures.
This does not yet say which of the two the CI failure is -- that is what
the next run will show. Locally on Linux all 30 assertions pass, so the
contract holds outside covr.
Rides along with the CI migration because that migration is what surfaced
it, and #212 cannot go green without knowing which it is.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NNLtKFXrNPN2XvuAjsy4Sy
The shared trap log was unlink()ed between the negative control and the
protective run. That looks equivalent to isolation but is not: pak does
its work in background subprocesses, so a straggler from the negative
control can recreate the log AFTER the unlink, and its `sudo -s id` is
then attributed to the protective run.
That matches every observation from the CI failure:
* the trap fired in the protective run with the negative control's
exact signature, `SUDO INVOKED: -s id`;
* yet the child reported PKG_SYSREQS="false", so Require's .onLoad had
correctly configured that child -- the assertion added in ed371a3
passed;
* and it is timing-dependent, which is why it passes on an 8-core local
box and failed on a loaded GHA runner. This test had never run in CI
before, so nothing had exercised the race.
The mechanism it is meant to catch is real and unchanged -- pkgdepends'
default_sysreqs() calls can_sudo_without_pw(), which runs
processx::run("sudo", c("-s", "id")) -- but that default is only computed
when the sysreqs config has no explicit value, which is precisely what
PKG_SYSREQS=false supplies.
Separate trap directories per scenario make the misattribution
structurally impossible rather than merely unlikely.
Locally all 30 assertions still pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NNLtKFXrNPN2XvuAjsy4Sy
…ed tree
Root cause of the CI failure, and it is the test, not the package.
The protective run did:
pkgRoot <- normalizePath(".") # the testthat directory
pkgload::load_all(pkgRoot) # walks up to find a package root
From a source checkout that walks up to the real package and .onLoad runs.
Under covr and R CMD check the tests run from an INSTALLED tree, so it
walks up to an installed package directory instead. That directory has a
DESCRIPTION but keeps its code in R/Require.rdb rather than R/*.R, so the
namespace ends up registered without .onLoad having executed.
The result is a run that looks protective and is not:
* "Require" %in% loadedNamespaces() is TRUE, so the load appeared to
succeed;
* PKG_SYSREQS was never forced to "false";
* pak therefore computed its own default, which is
default_sysreqs() -> can_sudo_without_pw() -> processx::run("sudo",
c("-s", "id"));
* the trap recorded that probe and the protective assertion failed with
exactly the signature a real privilege-escalation regression produces.
The assertion added in ed371a3 is what separated the two: it showed the
child reporting PKG_SYSREQS empty while loading "successfully". Without it
this would have read as the 2.0.0 regression returning.
Now the child looks for a genuine source root -- DESCRIPTION *and* R/*.R,
which an installed tree fails -- and load_all()s that when found, falling
back to the installed package otherwise. Preferring source keeps the
working copy under test rather than a possibly stale installed copy.
The report added here (loaded, how, onLoadRan, PKG_SYSREQS,
PKG_SYSREQS_SUDO, getOption, namespace path, pkgRoot) is cat()ed rather
than left in expect_*() info because CI truncates testthat.Rout.fail from
the front; printing it immediately before the assertions keeps it next to
any failure.
Require's mitigation was never at fault: .sysreqsUserOptedIn() correctly
returns FALSE when neither PKG_SYSREQS nor options(pkg.sysreqs) is set, and
.onLoad then forces both env vars off. That contract holds -- it simply was
not being exercised.
Locally 30/30 via load_all, and via the installed package when no source
tree is present.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NNLtKFXrNPN2XvuAjsy4Sy
eliotmcintire
added a commit
that referenced
this pull request
Sep 3, 2026
Merges development (PRs #212 and #213) into the release branch and notes the browser() -> stop() change in NEWS under 2.1.1. cran-comments.md goes from 99 lines to 47. It said everything it needed to but buried the point under a long explanation of the .libPaths() mechanism, which belongs in NEWS rather than a submission comment. What a reviewer needs is unchanged and still explicit: what 2.1.1 fixes, why it follows 2.1.0 by five days, both notes quoted verbatim with the platforms that produced them, and the revdep result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NNLtKFXrNPN2XvuAjsy4Sy
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
1 participant
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.
Replaces #206. Thin callers on
PredictiveEcology/actionsfor the threehand-rolled workflows, plus the two pieces of shared infrastructure this repo
never picked up.
R-CMD-checktest-coveragepkgdowntest-downstreamrevdepsWhy not #206
Four of its five non-docs files (
README.md,codecov.yml,CRAN-SUBMISSION,and the
isTRUE()one-liner inR/Require-helpers.R) are already byte-identicalon
development. It also carries 54docs/files that.gitignoreexcludes,and it conflicts with #208. Most importantly its caller had a single
uses:job,so merging it would have deleted
split-library-check.split-library-check is preserved verbatim
Only change:
actions/checkoutv5 → v7, which is the migration's stated point(off the deprecated Node 20 runtime).
It cannot fold into the shared workflow. Caller env is applied before
setup-r-deps, soR_LIBS_USERcould ride in as anextra-configleg — butthe "Verify the split-library layout reproduces the CRAN shape" guard has no
input to hang on, and without it the leg silently decays into an ordinary
ubuntu/release run. It stays a plain
runs-on:job in the caller file.This is the only CI coverage of the
.libPaths()layout behind the 11 CRANfailures in 2.1.0.
Customisations carried across
_R_CHECK_THINGS_IN_OTHER_DIRS_viaextra-envoldrel-2(both OSes) andoldrel-3(Ubuntu) viaextra-config; windowsoldrel-3stays deliberately absent — dep-install storms ran past GHA's 6hcap (fix(pak): never file-copy pak; ensure it's fresh in project lib #155)
R_REQUIRE_RUN_LONG_CI. A duplicate by construction (no way to add env to adefault leg), and the right trade: those gated tests shouldn't slow the leg
everything else waits on.
NOT_CRAN=truefor coverage — the shared workflow sets it itself nowCoverage also moves macOS → ubuntu (the shared default) and stops cancelling
in-progress push runs, which the CI standards call out as a source of spurious
red.
The shared workflows have no
[skip-ci]guard. Note the hyphen —[skip-ci]is not one of GitHub's native skip keywords (
[skip ci],[ci skip],[no ci],[skip actions],[actions skip]), which is why it needed a manualif:here in the first place. The guard survives onsplit-library-check; theshared matrix responds to GitHub's own
[skip ci]spelling. Documented in thefile. If we want the hyphenated form back it should be an opt-in input upstream,
not a local fork.
New workflows
test-downstream— SpaDES.core and SpaDES.project, the only two PE packagesdeclaring a
Requiredependency. The shared workflow strips theirRemotes:pin and asserts the installed package has no
RemoteSha; without that a PR runpasses having tested none of the proposed change.
revdeps—workflow_dispatchonly, per the shared action's own warningthat revdep checks are too heavy for standard runners. Relevant right now:
SpaDES.core is back on CRAN at 3.2.1 after its 2026-07-13 archiving, so
Require again has a live reverse dependency and the stored "no reverse
dependencies" conclusion in
revdep/is stale.Pins
@mainthroughout, stated in each file. A tag cannot change underneath us butis exactly what broke SpaDES —
install-spatial-deps@v0.1apt-installing apackage name retired before noble.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NNLtKFXrNPN2XvuAjsy4Sy