Adds min package version

[averissimo]

WIP :: parent issue: insightsengineering/nestdevs-tasks#7

Supersede:

• verdepcheck #45

🔴 Checklist for PR Reviewer

(see comment below)

• Tag yourself next to this repo on Specify minimal version dependencies + add action nestdevs-tasks#7
• Package versions are the same or higher than main
• Package list is the same
  • Only exception is rmarkdown (may have been removed on Suggests)
• All packages in Imports, Depends & Suggests are in new section Config/Needs/verdepcheck
• Added entry to NEWS.md
• Last scheduled.yaml action was run succesfully (all 4 strategies)
  • important: it's not the last commit, it's the one that runs 4 Scheduled 🕰️ / Dependency actions
• scheduled.yaml SHOULD NOT have any push on any branches

🔴 What's needed before merging?

This PR depends on some upstream changes that need to be finalized/merged before being ready to review.

Change in code

• verdepcheck.yml action (see comments)
  • Remove on: push section
  • Change branch to main

PRS

• verdepcheck
  • Training verdepcheck#24
  • Fixes and improvements for strategies verdepcheck#26
• verdepcheck-action
  • Supports new strategies min_isolated and min_cohort r-verdepcheck-action#16

Changes description

• Adds minimum version for packages DESCRIPTION
• Adds Config/Need/verdepcheck section in DESCRIPTION
• Updates verdepcheck action

[donyunardi]

Please hold on merging this until we finish teal.logger CRAN release.
#51

[averissimo]

@donyunardi 👍 this was still at a draft stage, so it wouldn't be merged at this initial sweep.

What do you think about integrating a step on the CRAN checklist that explicitly says "remove Config/Needs/verdepcheck" from CRAN's description file?

As that section is purely a development requirement

• Remove the additional fields (Remotes and Config/Needs/*) from the DESCRIPTION file where applicable.

Maybe by adding (such as verdepcheck)

• Remove the additional fields (Remotes and Config/Needs/*) from the DESCRIPTION file where applicable.

[donyunardi]

Hi @averissimo,
I'm not sure about the idea of repeatedly deleting and restoring Config/Needs/verdepcheck every time we want to make a release. Unless CRAN raises concerns about this aspect, I'd prefer to leave it in the DESCRIPTION file. Otherwise, it seems we have no choice but to delete and restore it.

[averissimo]

max strategy is expected to be failing due to this issue #60

See here for workflow (triggered manually): https://github.com/insightsengineering/teal.logger/actions/runs/6247976226/job/16961624494

[github-actions]

Code Coverage Summary

Filename               Stmts    Miss  Cover    Missing
-------------------  -------  ------  -------  ---------
R/register_logger.R       56       1  98.21%   113
R/supress_logs.R           5       5  0.00%    17-21
R/utils.R                 17       0  100.00%
R/zzz.R                    2       2  0.00%    3-4
TOTAL                     80       8  90.00%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 3a1476d

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

10 tests   10 ✔️  0s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 3a1476d.

♻️ This comment has been updated with latest results.

[m7pr]

Starting the workflow again for verdepcheck branch as the last build failed https://github.com/insightsengineering/teal.logger/actions/runs/6249701997

[m7pr]

@averissimo there is no rmarkdown and we failed on vignettes creation in R CMD Check action : )

[averissimo]

@m7pr I don't understand the error, it shouldn't fail with knitr >= 1.42 as teal.slice and others do not.

Regardless, it's failing, I propose we get rmarkdown dependency back, WDYT @m7pr

[m7pr]

@averissimo is it possible that knitr version inside docker container is smaller and not enough?

[m7pr]

I would recommend updating the container with the never knitr version. I think we should contact @cicdguy for this. And once we have an updated container used for tests, we either update the tag or the container name https://github.com/insightsengineering/r.pkg.template/blob/main/.github/workflows/build-check-install.yaml#L175-L176 or we don't update it, if the latest tag gets never knitr version.

Updating knitr in the container for tests will allow us to remove rmarkdown from so many other R packages.

[averissimo]

knitr and rmarkdown are installed in the CI image.

It's weird because the same setup has been done across different repositories and only teal.logger is failing

[m7pr]

No idea what's going on @averissimo
Any way you can push an empty commit in here to restart R CMD Check?

[pawelru]

Isn't it because of this?

teal.logger/vignettes/teal-logger.Rmd

Line 4 in db3031f

output: rmarkdown::html_vignette

EDIT:
No sorry. This line is no different than any other vignette in other packages.

[averissimo]

I can't reproduce this error locally (i.e. building and checking with CI image as done below)

@cicdguy any idea on how to best reproduce the action?

git pull ghcr.io/insightsengineering/rstudio_4.3.1_bioc_3.17:latest
git clone [email protected]:insightsengineering/teal.logger --branch=verdepcheck_action remove_me_teal.logger
docker run -v ./remove_me_teal.logger:/workspace/teal.logger -it ghcr.io/insightsengineering/rstudio_4.3.1_bioc_3.17:latest \
  bash -c "R CMD build teal.logger/ && R CMD check --as-cran teal.logger_0.1.3.9005.tar.gz"

I also tried setting up the same env variables within the docker container

export junit_xml_storage=_junit_xml_reports
export junit_xml_diff_branch=main
export junit_xml_comparison=true
export junit_xml_positive_threshold=1
export junit_xml_negative_threshold=1
export PKGBUILD=teal.logger_0.1.3.9005.tar.gz
export PKGNAME=teal.logger
export TESTING_DEPTH=1
export _R_CHECK_TESTS_NLINES_=0
export _R_CHECK_VIGNETTES_NLINES_=0
export _R_CHECK_FORCE_SUGGESTS_=true

No warnings / errors

[cicdguy]

Yeah this is an odd one. Let me see what the deal is.

[cicdguy]

The trick was to add rmarkdown to the suggests (as suggested in https://bookdown.org/yihui/rmarkdown-cookbook/package-vignette.html. Seems to work after that.

[averissimo]

Thanks for that 😃, however, (for the full context) we were trying to remove the {rmarkdown} dependency as @pawelru mentions on this comment

This approach was successful on other {teal.*} packages.

The same setup without {rmarkdown} on Depends/Suggests/Imports works on {teal.widgets}

{teal.widgets} has the same exact conditions as teal.logger (the only mention of {rmarkdown} is in the vignette's YAML header with rmarkdown::html_vignette)

This creates a sort of random effect that I was hoping to understand. Especially as R CMD check teal.logger_.... passes without errors when running locally (ghcr.io/..._4.3.1_bioc_3.17:latest).

[cicdguy]

Yeah I figured that was the case. Let me remove it and try again. This is so odd.

[m7pr]

Brough back rmarkdown and scheduled the Scheduled Action for this feature branch https://github.com/insightsengineering/teal.logger/actions/runs/6420601095

[m7pr]

It started breaking on min_isolated and on max

[m7pr]

I would merge if it did not broke on min_isolated

[pawelru]

You need to add it also under Suggests

[m7pr]

@pawelru I did 6903308

[averissimo]

@m7pr rmarkdown needs to be at least v2.19 as earlier versions don't successfully build due to Rcpp older versions

The automated action runs fine with the latest commit I added today: https://github.com/insightsengineering/teal.logger/actions/runs/6533296712

[m7pr]

@averissimo you're good to merge