Something is wrong re: checking for src/ directory

[jennybc]

I think something is not right / finished about: 9cea172

With dev pkgload installed, suddenly several of usethis's long-standing tests start to file like so:

Error in `pkgload::load_all(path, helpers = FALSE, attach_testthat = FALSE)`: The package "pkgbuild" is required to compile packages with a `src/` directory.

To experience this for yourself, run usethis's tests with CRAN pkgload (1.3.2) versus the current dev version.

[jennybc]

Here's an example of one of the new test failures, which I think are false positives caused by a pkgload bug:

── Error ('test-use_import_from.R:4:3'): use_import_from() imports the related package & adds line to package doc ──
<rlib_error_package_not_found/rlang_error/error/condition>
Error in `pkgload::load_all(path, helpers = FALSE, attach_testthat = FALSE)`: The package "pkgbuild" is required to compile packages with a `src/` directory.
Backtrace:
     ▆
  1. └─usethis::use_import_from("tibble", "tibble") at test-use_import_from.R:4:2
  2.   └─usethis:::roxygen_update_ns(load)
  3.     ├─utils::capture.output(...)
  4.     │ └─base::withVisible(...elt(i))
  5.     ├─base::suppressMessages(roxygen2::roxygenise(proj_get(), "namespace"))
  6.     │ └─base::withCallingHandlers(...)
  7.     └─roxygen2::roxygenise(proj_get(), "namespace")
  8.       └─roxygen2 (local) load_code(base_path)
  9.         └─pkgload::load_all(path, helpers = FALSE, attach_testthat = FALSE)
 10.           └─rlang::check_installed("pkgbuild", reason = "to compile packages with a `src/` directory.") at pkgload/R/load.R:156:4

[jennybc]

I think you'll need to do a release for #248 (I'm surprised you aren't on the same email I am, but maybe CRAN did this in batches), but I also think it's important that this get sorted out before said release.

[lionel-]

This error doesn't happen for end users because devtools depends on pkgbuild.

To fix this on reduced stack (e.g. CI where Jenny encountered the problem IIUC), should we just make pkgload depend on pkgbuild @gaborcsardi?

[gaborcsardi]

I don't mind if pkgload imports pkgbuild, but it seems that that pkgload change made pkgbuild a test dependency of usethis, so maybe usethis can suggest pkgbuild?

[lionel-]

IIUC with the change load_all() fails by default (compile = NA) when pkgbuild is missing, so that effectively makes pkgbuild a hard dep of pkgload, and in that case it's best to reflect that in DESCRIPTION?

[gaborcsardi]

Oh, right, that makes sense. Otoh maybe we can undo that change and look at /src/, /configure and the NeedsCompilation field instead? Just a suggestion, I don't mind adding it as a dependency, either.

[lionel-]

I wondered about that too but it seems simpler to just take the dependency. I'll do that and release pkgload.

[jennybc]

I'll contribute something more concrete soon. But in the tests that started failing in usethis, the toy package doesn't have a src/ directory. So I'm wondering if the logic is the way you want it to be, in terms of requring pkgbuild.

[lionel-]

We could make pkgbuild an optional dependency by adding a bunch of logic to detect if we do need to build, as Gabor suggested.

[jennybc]

And/or change the message? It's just really confusing when it claims there's a src/ directory, but there is not a src/ directory.

[jennybc]

I think it is conceivably worth it to actually try and determine if pkgbuild is necessary, but I will defer to you folks on that point.

[lionel-]

I opted for simplicity and made it a dependency.

Does it fix your issue @jennybc?

[jennybc]

Yes, usethis passes R CMD check cleanly on r-release with r-lib/pkgload@58567be ✅