Overzealous definition of "dependency error"

jennybc commented 3 years ago

@jimhester recently set _R_CHECK_FORCE_SUGGESTS_="false" in our cloud revdep checks, at my request, because a revdep has a soft dependency on a Bioconductor package: the usethis revdep butcher Suggests NMF, which is on Bioconductor.

This leads to butcher/dependency_install.log to contain this ERROR message:

ERROR: dependency 'Biobase' is not available for package 'NMF'

However butcher can still be installed and checked. But I can't inspect those details via cloud_details(revdep = "butcher") because cloud_compare() still interprets this as a dependency error and we end up here:

https://github.com/r-lib/revdepcheck/blob/db3bc3b3667db4f93b63ff2e87a3c5608feafa81/R/cloud.R#L342-L344

> cloud_details(revdep = "butcher")
══ Reverse dependency check ══════════════════════════════════════════ butcher  ══

Status: 

Error in if (nrow(rows) == 0) { : argument is of length zero
Run `rlang::last_error()` to see where the error occurred.
> lt
<error/rlang_error>
Error in if (nrow(rows) == 0) { : argument is of length zero
Backtrace:
    █
 1. ├─(function (x, ...) ...
 2. └─revdepcheck::print.revdepcheck_details(x)
 3.   ├─base::print(structure(x, class = "rcmdcheck_comparison"), header = FALSE) /Users/jenny/rrr/revdepcheck/R/results.R:53:2
 4.   └─rcmdcheck:::print.rcmdcheck_comparison(...)
 5.     └─rcmdcheck:::print_comparison(x, -1, "Fixed")

It's a printing problem, i.e. this leads to the formation of an unexpected or malformed object of class rcmdcheck_error.

I'm not sure which solution(s) is/are correct:

Detect that we successfully installed and checked butcher, despite the failure to install NMF
Construct a (more) valid rcmdcheck_error object by having a component named cmp here
Make rcmdcheck:::print_comparison resilient to the absence of x$cmp

cderv commented 2 years ago

I encountered this issue also

Error in if (nrow(rows) == 0) { : argument is of length zero

for package with installation error (due a issue on RSPM during the installation).

Same cause

jennybc commented 2 years ago

Based on what I learned when doing #319, I think these lines are "overloaded", i.e. too many different kinds of problems (or maybe even non-problems) are lumped together by:

any(grep("ERROR: .*is not available for package", readLines(dependency_path, warn = FALSE))) || !(file.exists(old) && file.exists(new))

https://github.com/r-lib/revdepcheck/blob/a600b05a462913a205952e33672327e01cc41337/R/cloud.R#L377-L385

r-lib / revdepcheck

Overzealous definition of "dependency error" #287