actions
actions copied to clipboard
r-lib
Should we fail on R CMD check NOTES in check-full?
Warning seems like the right default for most package developers, but I keep getting surprised that GHA doesn't fail on NOTES when working on tidyverse packages.
I think the major blocker to doing this would be the package size NOTE. But perhaps we could use _R_CHECK_PKG_SIZES_=false or increase _R_CHECK_PKG_SIZES_THRESHOLD_.
Are there other common NOTES that don't stop a package submission we would have to ignore?
I think most of the notes we hit are only for CRAN submission, two others I can remember off the top of my head are:
❯ checking Rd cross-references ... NOTE
Package unavailable to check Rd xrefs: ‘dplyr’
❯ checking for GNU extensions in Makefiles ... NOTE
GNU make is a SystemRequirements.
(both from haven)
The first is generally easy to fix, but I know the tidymodels folks don't always add when they already have very heavy suggests.
It looks like we can turn off the first with _R_CHECK_RD_XREFS_ and the second the GNU make one with _R_CHECK_CRAN_INCOMING_NOTE_GNU_MAKE_, though the latter is not documented in R-ints, so must be relatively new.
Now that we have a check action, maybe we could have an argument that's like "ignore common unimportant notes" or something? Or some setting that's part way between "warning" and "note"?
Maybe "note" could mean "notes that generally interfere with cran submission" and "all" could mean "all notes"?
Or we could make it more clear that this is an opinionated choice that we've made and call "significant" notes "tidy"
Those sound like good suggestions to me. One thing I worry about is if we will run into a time when a note we want to ignore doesn't have a envvar. Also some of the envvars were added only in later versions of R.
It might be worth talking with @gaborcsardi about support for some sort of ignore list in rcmdcheck, so we can ignore them from the package rather than relying on the envvars to disable the checks.
Yes, I am going to add this: https://github.com/r-lib/rcmdcheck/issues/12 to solve this very issue. It is mostly straightforward, the slightly challenging part is non-deterministic output.
OTOH, maybe we could just suppress particular NOTEs using env vars? What are the ones that we want to suppress? I could think of two:
- NOTE about the package size, for which we can set
_R_CHECK_PKG_SIZES_THRESHOLD_to a higher value, - NOTE about marked UTF-8 (etc.) string in data, this can be suppressed with
_R_CHECK_PACKAGE_DATASETS_SUPPRESS_NOTES_=trueI think.
Any others?
Or maybe the best is a combination of both: if NOT_CRAN=true then we set a list of env vars for the check, and the env vars can be configured in a config file. This way we can suppress NOTEs via a config file.
@gaborcsardi see above 😉 _R_CHECK_RD_XREFS_ and _R_CHECK_CRAN_INCOMING_NOTE_GNU_MAKE_.
I think @jimhester's worry is that we'll eventually hit a note that we can't easily suppress, which is where something more general from rcmdcheck would come in handy. We could also just switch back to warnings for that repo.
@gaborcsardi see above
:)
I think @jimhester's worry is that we'll eventually hit a note that we can't easily suppress,
That's true, but CRAN is quite meticulously adding env vars for the new NOTEs, probably because some of them don't want to see them.
So I think for now I'll just add a custom env var, can can suppress notes and checks on a per package basis.
If it turns out that we need an ignore file, we can add support for it later.
But then maybe rcmdcheck doesn't need to do anything? We could just set those env vars in the check-r-package action for (say) error-on: cran-notes?
Yeah, that's not too bad, either. But if it is in rcmdcheck, then it is used locally as well.
Oh yeah, aligning the local and GHA results is a good motivation for doing this. In that case, are you sure NOT_CRAN is the right env var? It feels like it might be too aggressive? (I can't make up my mind either way so I think we should briefly discuss it).
NOT_CRAN would just make rcmdcheck add the env vars from inst/check.env for the check. There is also another env var to force turn it on/off.
So nothing changes, unless people add check.env to their package, and also set NOT_CRAN=true.
Does the file have to be included in the built package? Couldn't rcmdcheck read it before building?
Sure, but then it does not work on places where you use a package .tar.gz, eg. on R-hub.
@jimhester But I don't mind that much, if you prefer, I can also put it in .check.env in the package root, and people will have to ignore it then. Or support both.
That makes sense, I just don't really love shipping this file to users, but it is not a huge deal either way.
OK, so this is now up to us flipping the switch, probably in the example workflows? Changing the default in the action would probably break many builds.
I still think we https://github.com/r-lib/rcmdcheck/issues/173 first, because, as you say, switching to notes will be too annoying. Agreed that we should change in the examples.
For the record, https://github.com/r-lib/rcmdcheck/issues/173 is still blocking this.