Do we not run R CMD Check on devel?

[ddsjoberg]

If not, we should!

Clarification

By devel, @ddsjoberg means the devel version of R, that is the unreleased, active development version of R (which is 4.4 as of this writing).

We ought to create a new image for R devel by using the rocker/rstudio:devel image as the base, and installing packages/dependencies directly from CRAN instead of an renv.lock file. Since the dependencies from CRAN will go through a continuous update process, we need to run update.packages() before running R CMD check or other operations. Note that this will be a one-time effort of creating the devel image, as the rocker/rstudio:devel image will always fetch the latest devel version of R. Also if you run the image creation process on a regular basis (assuming one in 2 weeks), you can always keep the devel version up-to-date and not have to worry about updating it ever again.

And then, the workflows within this repo should be updated to include the devel list item in the matrix. Note that this is only needed for R CMD check and not other workflows.

[bms63]

@pharmaverse/admiral Hi team. This is a pretty easy update to the CI workflows. If you want to try a little CI magik then this issue is for you!!!

https://github.com/pharmaverse/admiralci/blob/main/.github/workflows/r-cmd-check.yml

[dgrassellyb]

Hey everyone !
I started on this issue, we now have a docker image "devel" on admiralci (I will also setup a cronjob to keep it updated) - you can see the logs here :
https://github.com/pharmaverse/admiral/actions/runs/6638710927/job/18035623837
The devel check is failing, more precisely on occds.Rmd here :

adae <- adae %>%
  derive_var_trtemfl(
    trt_start_date = TRTSDT,
    trt_end_date = TRTEDT,
    end_window = 30
  )

But I suppose it's a behavior we will have quite often, since devel will contains dependencies updated from CRAN directly I expect that we will have often this kind of errors related to updated dependencies ?
Should we convert this devel checks into warnings then ?

[bms63]

Thanks @dgrassellyb!! I think switching it to warnings make sense.

The occds.Rmd issue looks like something we might need to fix to be ready for 4.4....but didn't really look very closely @pharmaverse/admiral

[cicdguy]

I wouldn't recommend switching to warnings for the R-devel checks because it could go unnoticed for a while as people might ignore them as the builds will "succeed" all the time.

[ddsjoberg]

Agree with @cicdguy . Best to stay ahead of any breaking changes in R-devel before they are an emergency 🚨 🚨 🚨

[bms63]

Okay so leave as Error but have it not be a Required check? Or should we always fix errors in r-devel - do they (CRAN) sometimes revert their updates or fix issues that might cause r-devel to error for us? Just don't want to fix something that is reverted later.

[cicdguy]

I'd say to not have it as a required check given that r-devel and packages therein will always be a moving target and ever changing. Not sure about what CRAN does though, I'll defer to @ddsjoberg to address that.

[zdz2101]

Okay so leave as Error but have it not be a Required check? Or should we always fix errors in r-devel - do they (CRAN) sometimes revert their updates or fix issues that might cause r-devel to error for us? Just don't want to fix something that is reverted later.

Think the trouble is that it can either be:

1. something that they are actually implementing and we need to fix such as the recent is.atomic(NULL) example
2. a small bug during their development process that they do indeed need to revert in which case we don't need to do anything

So agreed the not-required check is probably best option so that we do see it, but take action on a case-to-case basis, I don't imagine this comes up too often though

[ddsjoberg]

I think it's fine to not make it required. Most failures that only occur on devel have nothing to do with the submitted PR.