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

Display error traceback when vignettes fail in R CMD build #2390

Merged
merged 7 commits into from
Feb 28, 2025

Conversation

hadley
Copy link
Contributor

@hadley hadley commented Jan 22, 2025

Recording some work so that I can come back to it later.

res[i] = xfun:::handle_error(
withCallingHandlers(
if (tangle) process_tangle(group) else process_group(group),
error = function(e) {
if (progress && is.function(pb$interrupt)) pb$interrupt()
if (xfun::pkg_available('rlang', '1.0.0')) rlang::entrace(e)

if (xfun::pkg_available('rlang', '1.0.0')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For vignettes, this implies that your package needs to depend on rlang. I think that's ok, but should be documented somewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

depend on rlang

Adding as Suggest would be enough ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry, I meant depend in the loose sense.

R/output.R Outdated
# return a string to point out the current location in the doc
get_loc = function(label = '') {
paste0(current_lines(), label, sprintf(' (%s)', knit_concord$get('infile')))
paste0(knit_concord$get('infile'), ":", current_lines(), label)
Copy link
Contributor Author

@hadley hadley Jan 22, 2025

Choose a reason for hiding this comment

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

This uses a more standard format for file + line numbers that VS Code/Positron will autolink (when you hold down cmd in the terminal). cli hyperlinks don't work in R CMD build, because even if they did, it would open in the file in the check directory, not the current project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should help solve this issue then

I can see it works in Terminal in Positron, but not in R console unfortunately;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, although using cli would be even more useful there (although not helpful for vignettes)

@hadley hadley changed the title WIP: Display error tracebacks when vignettes fail Display error traceback when vignettes fail in R CMD build Feb 17, 2025
@hadley
Copy link
Contributor Author

hadley commented Feb 17, 2025

I think this is ready for review. When I install this version of knitr and build https://github.com/hadley/testvignette, I see this:

E  creating vignettes (1.1s)
   --- re-building ‘fail.Rmd’ using rmarkdown
   
   Quitting from fail.Rmd:16-18 [unnamed-chunk-1]
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   <error/rlang_error>
   Error in `h()`:
   ! This is a problem
   ---
   Backtrace:
       ▆
    1. └─global f()
    2.   └─global g()
    3.     └─global h()
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   
   Error: processing vignette 'fail.Rmd' failed with diagnostics:
   This is a problem
   --- failed re-building ‘fail.Rmd’
   
   SUMMARY: processing the following file failed:
     ‘fail.Rmd’
   
   Error: Vignette re-building failed.
   Execution halted

There's quite a lot of duplication in the output, but I don't see an obvious way to resolve this, because it's generated by R CMD build, not knitr or rmarkdown.

@hadley hadley marked this pull request as ready for review February 17, 2025 15:00
@bastistician
Copy link
Contributor

Thanks for looking into this. Inspecting knitr-based vignette errors when mass-checking R packages has been a bit cumbersome in the past, mostly because the call was missing (so I have sometimes used a patched R to get more output). So I was happy to find this PR and hope that diagnostics will be provided for both vignette build and check errors in the future.

Both R CMD build and check use tools::buildVignettes() to build ("weave") each package vignette using the engine it declares. When the engine's weave() function errors, this is reported with

Error: processing vignette '<file>' failed with diagnostics:
<conditionMessage>

For Sweave vignettes, the message includes the call (from evaluating chunk expressions using try()) as well as the chunk number/label (from RweaveTryStop()). Given that this PR is specifically about building package vignettes, I wonder if it would make more sense to amend the weave() functions in knitr's vignette engines rather than process_file()? Could knitr:::vweave() etc. catch errors and collect all useful information in the message?

Alternatively, I have experimented with a minor change of the buildVignettes output from <conditionMessage> to <as.character(e)> (the patch mentioned above), which would include both the call and the message from the error condition, also for "rlang_error"s (but not their backtrace it seems), so something like:

Error: processing vignette 'fail.Rmd' failed with diagnostics:
Error in `h()`:
! This is a problem

Printing the condition object might be another option.

@gaborcsardi
Copy link

AFAICT you already get a traceback for R CMD build, with line numbers and chunk names:

❯ R CMD build testvignette
* checking for file ‘testvignette/DESCRIPTION’ ... OK
* preparing ‘testvignette’:
* checking DESCRIPTION meta-information ... OK
* installing the package to build vignettes
* creating vignettes ... ERROR
--- re-building ‘fail.Rmd’ using rmarkdown

Quitting from fail.Rmd:16-18 [unnamed-chunk-1]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<error/rlang_error>
Error in `h()`:
! This is a problem
---
Backtrace:
    ▆
 1. └─global f()
 2.   └─global g()
 3.     └─global h()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Error: processing vignette 'fail.Rmd' failed with diagnostics:
This is a problem
--- failed re-building ‘fail.Rmd’

SUMMARY: processing the following file failed:
  ‘fail.Rmd’

Error: Vignette re-building failed.
Execution halted

@hadley
Copy link
Contributor Author

hadley commented Feb 19, 2025

@bastistician those all sounds like reasonable alternative implementations, but I can't quite tell if you think there's something missing in this PR?

@bastistician
Copy link
Contributor

Yes, I was especially hoping for improved error reporting for knitr-based vignettes also during R CMD check (e.g., when an updated dependency breaks a vignette or downstream vignettes are broken in a revdep check). AFAICS this PR adds special instrumentation only for R CMD build so wouldn't improve the check output:

* checking re-building of vignette outputs ... ERROR
Error(s) in re-building vignettes:
  ...
--- re-building ‘fail.Rmd’ using rmarkdown

Quitting from fail.Rmd:16-18 [unnamed-chunk-1]
Error: processing vignette 'fail.Rmd' failed with diagnostics:
This is a problem
--- failed re-building ‘fail.Rmd’

In my comment above I was reflecting whether the error handling should instead go to knitr's vignette engines so that it affects tools::buildVignettes() regardless of being called during build or check.

OTOH, I can also see some value in a more general change of what tools::buildVignettes() outputs in case it catches a "weave" error, not just the conditionMessage(). I think that would need a report on R's Bugzilla.

@hadley
Copy link
Contributor Author

hadley commented Feb 19, 2025

@bastistician oh gotcha, we could easily add a || is_R_CMD_check() to make this PR to work then too.

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.

Looks good.

could easily add a || is_R_CMD_check() to make this PR to work then too.

Doing this seems to work also

* using log directory 'C:/Users/chris/AppData/Local/Temp/testvignette/testvignette.Rcheck'
* using R version 4.4.2 (2024-10-31 ucrt)
* using platform: x86_64-w64-mingw32
* R was compiled by
    gcc.exe (GCC) 13.3.0
    GNU Fortran (GCC) 13.3.0
* running under: Windows 11 x64 (build 26100)
* using session charset: UTF-8
* checking for file 'testvignette/DESCRIPTION' ... OK
* this is package 'testvignette' version '0.0.0.9000'
* package encoding: UTF-8
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking whether package 'testvignette' can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking loading without being on the library search path ... OK
* checking files in 'vignettes' ... WARNING
Files in the 'vignettes' directory but no files in 'inst/doc':
  'fail.Rmd'
* checking examples ... NONE
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes ... WARNING
Directory 'inst/doc' does not exist.
Package vignette without corresponding single PDF/HTML:
  'fail.Rmd'
* checking re-building of vignette outputs ... ERROR
Error(s) in re-building vignettes:
  ...
--- re-building 'fail.Rmd' using rmarkdown

Quitting from fail.Rmd:16-18 [unnamed-chunk-1]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<error/rlang_error>
Error in `h()`:
! This is a problem
---
Backtrace:
    ▆
 1. └─global f()
 2.   └─global g()
 3.     └─global h()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Error: processing vignette 'fail.Rmd' failed with diagnostics:
This is a problem
--- failed re-building 'fail.Rmd'

SUMMARY: processing the following file failed:
  'fail.Rmd'

Error: Vignette re-building failed.
Execution halted

* checking PDF version of manual ... OK
* DONE
Status: 1 ERROR, 2 WARNINGs

res[i] = xfun:::handle_error(
withCallingHandlers(
if (tangle) process_tangle(group) else process_group(group),
error = function(e) {
if (progress && is.function(pb$interrupt)) pb$interrupt()
if (xfun::pkg_available('rlang', '1.0.0')) rlang::entrace(e)

if (xfun::pkg_available('rlang', '1.0.0')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

depend on rlang

Adding as Suggest would be enough ?

R/output.R Outdated
# return a string to point out the current location in the doc
get_loc = function(label = '') {
paste0(current_lines(), label, sprintf(' (%s)', knit_concord$get('infile')))
paste0(knit_concord$get('infile'), ":", current_lines(), label)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should help solve this issue then

I can see it works in Terminal in Positron, but not in R console unfortunately;

Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

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

we could easily add a || is_R_CMD_check() to make this PR to work

I'll merge the PR after that is added. Thanks!

@hadley
Copy link
Contributor Author

hadley commented Feb 27, 2025

@yihui done!

@yihui yihui merged commit c47d34f into yihui:master Feb 28, 2025
1 check passed
@hadley
Copy link
Contributor Author

hadley commented Feb 28, 2025

Thanks @yihui!

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.

5 participants