Repository navigation
Should we fail on R CMD check NOTES in check-full? #381
Description
Activity
I think the major blocker to doing this would be the package size NOTE. But perhaps we could use
_R_CHECK_PKG_SIZES_=falseor 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: r-lib/rcmdcheck#12 to solve this very issue.
It is mostly straightforward, the slightly challenging part is non-deterministic output.Reacted by Jim HesterOTOH, 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?
- NOTE about the package size, for which we can set
Or maybe the best is a combination of both: if
NOT_CRAN=truethen 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
warningsfor 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-packageaction 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_CRANis 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_CRANwould just make rcmdcheck add the env vars frominst/check.envfor the check. There is also another env var to force turn it on/off.So nothing changes, unless people add
check.envto their package, and also setNOT_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.envin 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.
Alternatively, we can put it in
tests/and then it is not installed?Or in
tools?Good idea, maybe
tools/is slighter closer to our usage here?Reacted by Gábor CsárdiOK, 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 r-lib/rcmdcheck#173 first, because, as you say, switching to notes will be too annoying. Agreed that we should change in the examples.
Reacted by Gábor CsárdiFor the record, r-lib/rcmdcheck#173 is still blocking this.
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.