Skip to content

Add regression test to quarto vignette #2872

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

jayhesselberth
Copy link
Collaborator

Calling library(pkgdown) should cause a GHA failure due to quarto not seeing the temporary directory that pkgdown is installed in.

Calling `library(pkgdown)` should cause a GHA failure due to quarto not seeing the temporary directory that pkgdown is installed in.
Copy link

github-actions bot commented Mar 19, 2025

@jayhesselberth
Copy link
Collaborator Author

jayhesselberth commented Mar 19, 2025

@cderv I realized that we never actually call library(pkgdown) in the quarto vignette, and adding it caused R CMD Check to fail. This is from the windows build:

--- re-building 'quarto.qmd' using html
processing file: quarto.qmd
1/5                  
2/5 [unnamed-chunk-1]
Error: Error in library(pkgdown) : there is no package called 'pkgdown'
Calls: .main ... withCallingHandlers -> withVisible -> eval -> eval -> library
Quitting from quarto.qmd:16-21 [unnamed-chunk-1]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<error/rlang_error>
Error in `library()`:
! there is no package called 'pkgdown'
---
Backtrace:
    ▆
 1. └─base::library(pkgdown)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Execution halted
Error: Error: processing vignette 'quarto.qmd' failed with diagnostics:
✖ Error running quarto cli.
Caused by error:
! System command 'quarto.exe' failed
--- failed re-building 'quarto.qmd'
SUMMARY: processing the following file failed:
  'quarto.qmd'

So this seems like a simple regression test.

An unrelated test is failing on other platforms, I think due to the recent knitr release

I was also confused why the netlify.yaml GHA succeeds, but noticed that the package is installed by that workflow:

- name: Install package
run: R CMD INSTALL .

And the pkgdown library is not installed in the same way by the standard pkgdown.yaml workflow. If I have this all straight, I think I'd predict that the pkgdown.yaml workflow would fail with this PR because of the temporary directory issue.

Next I'm going to install your quarto branch and see if it fixes this up.

@jayhesselberth
Copy link
Collaborator Author

jayhesselberth commented Mar 19, 2025

GHA now installs the quarto-dev/quarto-r@libpaths branch.

On windows I seethe expected "quarto not found" error, so this feels like the right test.

Wondering if I can add this to help debug:

        env:
          QUARTO_R_QUIET: FALSE

@jayhesselberth
Copy link
Collaborator Author

jayhesselberth commented Mar 19, 2025

QUARTO_R_QUIET did not appear to have an effect.

And ... the standard pkgdown workflow passes. Confused.

https://github.com/r-lib/pkgdown/actions/runs/13937206900?pr=2872

Copy link
Contributor

@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.

The error you have on other OS than Windows seem to be with a .Rmd vignette in your tests. Are they related to Quarto ?

For the problem on Windows CI, this is exactly what I am fighting against in quarto-dev/quarto-r#228

There is something in Windows (or Windows CI in GHA only) that is not expected. Setting R_LIBS in not enough - the R subprocess on Windows does not take it into account and this is why the package is not found.

This is windows specific and I still need to understand the problem. Plan is to fix this for sure.

I am still in "what the hell is happening !?" phase on this. So probably some difference in how R works between Windows and other OS.

Comment on lines +63 to +64
env:
QUARTO_R_QUIET: FALSE
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this should work 🤔

In your CI I see error with Rmd vignettes though, so it would not apply

Last 13 lines of output:
      ! Failed to render 'vignettes/test.Rmd'.
      x Quitting from test.Rmd:4-9 [unnamed-chunk-1]
  +   x ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  +   x NULL
  +   x ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      Caused by error:
      ! Error!
      ---

For windows, you see the error from the .qmd rendering so I think it works
https://github.com/r-lib/pkgdown/actions/runs/13937475969/job/39008052370?pr=2872#step:7:68

processing file: quarto.qmd
1/5                  
2/5 [unnamed-chunk-1]
3/5                  
4/5 [unnamed-chunk-2]
Error: Error in library(pkgdown) : there is no package called 'pkgdown'
Calls: .main ... withCallingHandlers -> withVisible -> eval -> eval -> library
Quitting from quarto.qmd:113-117 [unnamed-chunk-2]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<error/rlang_error>
Error in `library()`:
! there is no package called 'pkgdown'
---
Backtrace:
    ▆
 1. └─base::library(pkgdown)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Execution halted
Error: Error: processing vignette 'quarto.qmd' failed with diagnostics:
✖ Error running quarto cli.
Caused by error:
! System command 'quarto.exe' failed

@jayhesselberth
Copy link
Collaborator Author

jayhesselberth commented Mar 19, 2025

That other test failure is unrelated, I think caused by the recent knitr release. Skipping for now to get to the relevant quarto test, which now passes on linux/osx. Yay.

The GHA results now mirror what you're seeing in quarto-dev/quarto-r@libpaths, so I think we just need to sort the windows issue there.

Whenever I have seen windows-specific issues, @gaborcsardi is cc'd to magically appear and fix things. Hoping for a miracle.

@jayhesselberth
Copy link
Collaborator Author

jayhesselberth commented Mar 19, 2025

I am a little concerned though that I never actually saw the regression test fail on GHA linux or osx. And the pkgdown workflow worked from the start, which is still confusing given the current hypothesis.

Here is the output of the new regression test from deployed site above.

image

@jayhesselberth
Copy link
Collaborator Author

OK, this test does not trigger an error on GHA linux/osx.

I am now thinking this is just a windows-specific problem. Wondering if those that reported this quarto issue are all using windows.

The dev quarto version should appear in the regression test.
@jayhesselberth
Copy link
Collaborator Author

jayhesselberth commented Mar 19, 2025

Adding Remotes didn't have an effect. Do I need to add quarto to Suggests?

image

This is all so vexing. It feels like we might be trying to fix more than 1 bug at the same time.

#2830 does not report quarto build errors per se, only that a dev package is not seen by quarto. Though my expectation is that this is related to R_LIBS not being handled by quarto.

#2852 reports the windows-specific quarto build failure. I am guessing that once the quarto build is fixed on windows, quarto may still not see dev packages?

@cderv
Copy link
Contributor

cderv commented Mar 19, 2025

#2830 does not report quarto build errors per se, only that a dev package is not seen by quarto. Though my expectation is that this is related to R_LIBS not being handled by quarto.

Yes I believe this is so because package is installed by default in temp lib by default with buld_site() (as install = TRUE).

This is a problem locally and not in CI because the pkgdown.yml does install first (local::.), and use install = FALSE after

extra-packages: any::pkgdown, local::.

run: pkgdown::build_site_github_pages(new_process = FALSE, install = FALSE)

It happens on ALL os and quarto-dev/quarto-r#228 intends to fix that issue.

If dev package is installed first in default library, then no issue with CRAN quarto as R subprocess will see the default library. Hope it makes my understanding clearer

#2852 reports the windows-specific quarto build failure. I am guessing that once the quarto build is fixed on windows, quarto may still not see dev packages?

This is exactly the same as explained above. I tried to explain it in #2852 (comment)

quarto-dev/quarto-r#228 should also fix it. It fixes it for me locally on Windows !!

The remaining problem is that the Windows GHA runner with R actions from r-lib/actions does not behave like local windows. Somehow, R_LIBS set to .libPaths() content in quarto::quarto_render() is not taken into account for the R subprocess, which is why it still fails to find the dev package installed in the temp lib.

I am looking into that and this is not related to pkgdown IMO.

@cderv
Copy link
Contributor

cderv commented Mar 19, 2025

OK, this test does not trigger an error on GHA linux/osx.

I am now thinking this is just a windows-specific problem. Wondering if those that reported this quarto issue are all using windows.

About your try with the test, here is my understanding.

R CMD check won't fail like pkgdown::build_site(install = TRUE) because R will set R_LIBS env var itself. Which is inherit by the R subprocess. Even if CRAN quarto does not it, it is done by R in R CMD check context. This is among the analysis I did in quarto-dev/quarto-r#217 (comment)

It fails on Windows though for this problem I found: Setting R_LIBS in main R process does not have effect on R subprocess when run on Windows GHA runner using r-lib/actions.

This is windows specific error, independant of Quarto or pkgdown. It is reveled by Quarto vignette engine because we do render the vignette in a new process calling quarto CLI which in turn call R.

I hope I am clear enough. Sorry for not having managed to sum up my findings until now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants