Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent new_device = FALSE from accidentally opening a device #235

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Prevent new_device = FALSE from accidentally opening a device #235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cdf8686
Prevent `new_device = FALSE` from accidentally opening a device
hadley Jan 21, 2025
fe5d0e4
Merge branch 'main' into no-device
hadley Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# evaluate (development version)

* `evaluate()` once again doesn't open a device if `new_device = FALSE` (#234)

# evaluate 1.0.3

# evaluate 1.0.2
Expand Down
6 changes: 6 additions & 0 deletions R/watchout.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ watchout <- function(handler = new_output_handler(),
new_device = TRUE,
debug = FALSE,
frame = parent.frame()) {

if (new_device) {
# Ensure we have a graphics device available for recording, but choose
# one that's available on all platforms and doesn't write to disk
Expand Down Expand Up @@ -43,6 +44,11 @@ watchout <- function(handler = new_output_handler(),
sink_con <- local_persistent_sink_connection(debug, frame)

capture_plot <- function(incomplete = FALSE) {
# no plots open; par("page") will open a device
if (is.null(dev.list())) {
return()
}

# only record plots for our graphics device
if (!identical(dev.cur(), dev)) {
return()
Expand Down
9 changes: 6 additions & 3 deletions tests/testthat/test-conditions.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,12 @@ test_that("Error can be entraced and correctly handled in outputs", {
skip_if_not_installed("knitr")
skip_if_not_installed("callr")
skip_on_cran()
# install dev version of package in temp directory
withr::local_temp_libpaths()
quick_install(pkgload::pkg_path("."), lib = .libPaths()[1])

# if not inside of R CMD check, install dev version into temp directory
if (Sys.getenv("_R_CHECK_TIMINGS_") == "") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add this since you can't install the dev version during R CMD check (because the directory structure is different). Fortunately you don't need to since you know that the dev version is already installed. I'm not sure why this didn't affect you.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see I am affecting when running devtools::check(). I only ran devtools::test() I think.

And CI was passing so I trust it. It seems it is passing because this is called

rcmdcheck::rcmdcheck(args = (c("--no-manual", "--as-cran")), build_args = (c("--no-manual","--compact-vignettes=gs+qpdf")), error_on = ("warning"), check_dir = ("check"))

If I call this locally, it passes too. Does it mean rcmdcheck::rcmdcheck() will install the package ?

I recall I needed to have the package installed so that callr::rscript() can find it in libPath().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, R CMD check has to install the package in order to run the tests, so I think it should be ok. There must be some subtle difference in the directory structures that rcmdcheck uses.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R CMD check has to install the package in order to run the tests,

But it does install in an additional temp library right ? And when calling a subprocess we need this information to pass to the subprocess.

I make a parallel with the issue we have with Quarto vignette and pkgdown where, I need the quarto vignette engine to somehow pass R_LIBS to the subprocess called.

So maybe quick_install() was not the solution. If the package currently being check is installed, then it is a matter of passing the information of where to callr::rscript() which does not find the right version by default it seems.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got my answer. Details below as a reference from when I'll try to understand again.

R CMD CHECK will install the package in evaluate.Rcheck directory, which is added to libPaths() by rcmdcheck:::do_check(). This is why it is found by the subprocess without installing it.

There must be some subtle difference in the directory structures that rcmdcheck uses.

R CMD check will run tests frome evaluate.Rcheck/tests/testthat, so pkgload::pkg_path(".") does not manage to find root source package. During R CMD CHECK is seems source is inside evaluate.Rcheck/00_pkg_src and tests are not run from evaluate.Rcheck/00_pkg_src/evaluate/tests.

Though I looked at devtools::check() and it calls rcmdcheck::rcmdcheck(), so I was puzzled by the difference.

It is related to to NOT_CRAN = "true" being set by devtools::check_built() while not when calling rcmdcheck::rcmdcheck() directly.

  • withr::with_envvar(c(NOT_CRAN = "true"), rcmdcheck::rcmdcheck()) : This errors
  • rcmdcheck::rcmdcheck(): this does not.

So this was passing locally but in fact test are skiped.

So why it work on CI ? On CI NOT_CRAN: true is also set but rcmdcheck::rcmdcheck() is called with a check_dir within the source package.

Locally I can reproduce no failure with

withr::with_envvar(c(NOT_CRAN = "true"), rcmdcheck::rcmdcheck(check_dir = "check"))

because in that specific check_dir case, pkgload::pkg_path(".") is working, and finding the source package.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed exploration!

withr::local_temp_libpaths()
quick_install(pkgload::pkg_path("."), lib = .libPaths()[1])
}

out <- withr::local_tempfile(fileext = ".txt")

Expand Down
9 changes: 9 additions & 0 deletions tests/testthat/test-evaluate.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,15 @@ test_that("check_keep can integrate log option", {
expect_false(check_keep(FALSE, log = TRUE)$silence)
})

test_that("new_device = FALSE doesn't open any devices", {
graphics.off()
skip_if_not(is.null(dev.list()))

ev <- evaluate("1", new_device = FALSE)
expect_equal(dev.list(), NULL)
})


test_that("check_keep errors with bad inputs", {
expect_snapshot(error = TRUE, {
check_keep(1, "keep_message")
Expand Down
Loading