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.

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

Conversation

hadley
Copy link
Member

@hadley hadley commented Jan 21, 2025

Because par("page") will open a device if one is not already open. Fixes #234.

Because `par("page")` will open a device if one is not already open. Fixes #234.
@hadley hadley requested a review from cderv January 21, 2025 23:00
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!

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

rlang 1.1.5 was released yesterday it seems (https://github.com/r-lib/rlang/releases/tag/v1.1.5) and it was installed in the CI.

This needs to be a minimum requirement in DESCRIPTION and we need to update the snapshot as 1.1.5 was about reporting full backtrace.

This is the snapshot diff we see in CI

diff --git "a/.\\rmd-stop-error-auto-entrace.txt" "b/.\\rmd-stop-error-auto-entrace.new.txt"
index c2ea7b0..c6c1d52 100644
--- "a/.\\rmd-stop-error-auto-entrace.txt"
+++ "b/.\\rmd-stop-error-auto-entrace.new.txt"
@@ -4,9 +4,10 @@ processing file: ressources/with-stop-error-auto-entrace.Rmd
 Error in `h()`:
 ! !
 Backtrace:
- 1. global f()
- 2. global g()
- 3. global h()
+    x
+ 1. \-global f()
+ 2.   \-global g()
+ 3.     \-global h()

 Quitting from lines 6-10 [unnamed-chunk-1] (ressources/with-stop-error-auto-entrace.Rmd)
 Execution halted   

@hadley hadley merged commit 9b41223 into main Jan 23, 2025
14 checks passed
@hadley hadley deleted the no-device branch January 23, 2025 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latest release seems to have started ignoring new_device=FALSE
2 participants