Reduce exposure to CRAN checks and add initial Quarto support

[gadenbuie]

chromote::find_chrome() doesn't error (maybe it did at one point before chromote's CRAN release) and it doesn't check that the path exists.

Fixes #63 by checking that the path returned by find_chrome() actually exists.

[jhelvy]

Thanks for the quick fix on this! I wasn't sure what the source of the error was here.

[jhelvy]

If this is the only source of the CRAN error I could bump the version up to 0.1.1 and send it back to CRAN so it doesn't get taken down.

[gadenbuie]

If this is the only source of the CRAN error I could bump the version up to 0.1.1 and send it back to CRAN so it doesn't get taken down.

Yeah I'm pretty sure that's the source and bumping to 0.1.1 and sending back to CRAN sounds perfect.

Funnily the fix will just help us skip a few tests on CRAN.

Although... we do have a note about the existence of a file called Crashpad on Linux, but I have zero idea what that's about or how to reproduce or fix it (and searching didn't help either). It seems like a note they might be ignoring? Or maybe if it is a problem they'll tell us what it's about...

[jhelvy]

This fell behind as we were moving, but it's on my calendar to submit asap. The deadline to fix it is 8/31.

I can't seem to run the checks locally on my laptop, but I've used devtools::check_win_release() and devtools::check_win_devel() and both are good.

Whenever I devtools::check() I get this:

But when I select "yes" nothing happens. Something must be missing on my macbook, but I haven't figured it out yet.

Anyway, I was going to ship this back out to CRAN. Can you want to devtools::check() to double check things look good to submit?

[gadenbuie]

Hope your move went well! I just ran devtools::check() locally and all is good there. I think this is okay to send to CRAN 😄

[jhelvy]

Okay got more feedback from CRAN. They want us to address the note about crashpad:

under one check flavour (MKL, bus this may be unrelated) :

• checking for detritus in the temp directory ... NOTE
  Found the following files/directories:
  ‘Crashpad’

However, we cannot easily check the macOS issues, which seem to be from
assuming Chrome which is not present. And AFAIR leaving CrashPad is
related to Chrome ....
So please do not make such struct assumptions.

Please fix and resubmit.

[gadenbuie]

This is incredibly frustrating. We definitely don't assume that Chrome is present and the changes in this PR should guarantee that we don't try to use Chrome unless it exists on the system. I strongly believe that the changes in this PR would have fixed the macOS problem. I think it's likely that the Crashpad issue is actually unrelated and equally opaque.

Did CRAN share the pre-test results? Usually I receive an email with links to pre-check tests for Debian and Windows. It could be helpful to see if those give any insight.

[jhelvy]

To add insult to injury, they've removed renderthis from CRAN now: https://cran.r-project.org/web/packages/renderthis/index.html

Archived on 2022-09-05 as issues were not corrected in time.

Which is ridiculous as we were clearly in the process of addressing all issues and had even sent a revised version back. The request to send a revised version was sent on 9/4 at 10am, so we had less than half a day to respond to this obscure Crashpad issue.

[gadenbuie]

Lol, and if you click through to see our last check results and try to find out what were the MKL issues under Additional Issues, well you can't. Down for maintenance until Wednesday September 6. (Wednesday is Sept 7.)

[jhelvy]

I don't know what to do next. I just wrote an email back asking for more time to address this issue and explaining that we were clearly in the process of addressing the issues. Our fixes in v0.1.1 address the Chrome issue. I asked for more time and to see the pre-test results.

[gadenbuie]

My Crashpad theory is that it's something in the examples. The approach in the examples is to let errors happen but catch them and downgrade them to messages. I think that something, possibly out of our control, is throwing an error or causing a problem that only shows up on CRAN. (This isn't super hard to do with the packages we depend on.) So my next step would probably be to put most if not all examples behind if (interactive()).

But really, I'm just guessing here. Ultimately, the best strategy is to do less on CRAN: reduce examples as much as possible and possibly also split our tests into on-CI and on-CRAN tests. (That's what pagedown does.) Both are annoying and certainly don't contribute to the quality of our package. OTOH since we have reasonably robust tests on GitHub, I wouldn't feel bad about taking that approach.

[jhelvy]

That's annoying...but not a bad strategy. Kind of silly that we have to engineer a testing environment like this, but it will probably make life easier in the future. I'm cool with this set up.

If you split up the tests, do you think we should make that v0.1.2?

[jhelvy]

...I'm watching these commits come in and just thinking how unnecessary all this work is...hopefully when it's all said and done it won't be as arduous to fix things in the future.

[gadenbuie]

just thinking how unnecessary all this work is...hopefully when it's all said and done it won't be as arduous to fix things in the future.

Yeah, I totally agree with that. Sheesh. On the other hand, I've been trying to remind myself that it speaks to the utility of the package; we bring together a lot of packages with plenty of edge cases and system deps, etc. So I'm sure we're making something easier for some people 😄

So... I think we're in a good place now with the CRAN stuff but I also ended up throwing a little wrench in the mix and adding in quarto support.

CRAN Stuff

1. While we have examples for everything, with_example() will only let them run interactively, in CI, or when generating pkgdown docs (not that they're super useful there but it gives a better sense than nothing). This means that we'll have examples but they won't run on CRAN.
2. I couldn't do the test-splitting idea from pagedown because that actually depends on Yihui's testit package. So I made sure we are super liberal with skip_if_...() calls. The biggest is that skip_if_not_chrome_installed() also just straight-up skips on CRAN.

You can get a sense for what will run on CRAN by running this

devtools::check(cran = TRUE, manual = TRUE, env_vars = list(NOT_CRAN = ""))

Near the end you'll see output that reads something like

See
  ‘/private/var/folders/zw/8z2vpz1s0cl3gwpyh5p72mwh0000gn/T/RtmpAXYRGT/renderthis.Rcheck/00check.log’
for details.

Use file.edit() on that path or change 00check.log to tests/testthat.Rout to see the test output, which should look something like this

> library(testthat)
> library(renderthis)
> 
> test_check("renderthis")
[ FAIL 0 | WARN 0 | SKIP 21 | PASS 119 ]

══ Skipped tests ═══════════════════════════════════════════════════════════════
• On CRAN (21)

[ FAIL 0 | WARN 0 | SKIP 21 | PASS 119 ]

Showing that 21 sets of tests would be skipped on CRAN.

Quarto Stuff

c41b975 is the big commit that adds Quarto support. In a nutshell, we let to_html() support Quarto docs by calling quarto::quarto_render().

Then I took out some of the early warning guardrails in the higher-level functions, which is ultimately a good thing. Now lower-level functions keep track of the inputs they need. E.g. to_gif() needs the .pdf version, so it calls to_pdf() which eventually calls to_html() on the .qmd file. Previously we would have checked the file extension all the way up in to_gif(), which is just too early.

I updated most of the documentation, but we could probably use another pass. I didn't make any changes to the vignette, for example.

Also I set up a new convention of referring to PNG files in all caps when talking about the format and .png when talking about the file extension. (Same for PDF, ZIP, etc.) Not that I'm particularly attached to that convention, it just felt right.

It's late now and I've probably missed a thing or two; let me know if you have any questions or feedback.

[gadenbuie]

Btw, I actually ran into the CRAN error about Crashpad in testing! https://github.com/jhelvy/renderthis/runs/8241351503?check_suite_focus=true#step:9:191 It's from to_social() and is probably caused by something webshot2 does. We wouldn't have skipped that example on CRAN before, but we will now. So I'm pretty confident we've closed down our CRAN foot-guns.

Also I should mention the Quarto support is at an initial level of making quarto docs work where they most easily fit in to our workflow. There's definitely more work to do there and I'm okay with sending this to CRAN in this state and adding more Quarto support as we go. Mostly I just wanted to unblock the most common use case, e.g. #62.

[jhelvy]

Well I'll be...you found the Crashpad! This is awesome - I'll look through the documentation and other changes, then if all the checks work out I'll re-submit to CRAN. I'm thinking we should bump this to 0.2.0 with the Quarto support?

[jhelvy]

Okay I made a few small changes in the documentation. I moved a lot of the detailed installation instructions to the setup vignette and otherwise kept only the most basic install instructions in the README with a link to the more detailed Setup vignette.

Everything is looking good except for this one note:

checking for detritus in the temp directory ... NOTE
  Found the following files/directories:
    ‘quarto-session073af478’ ‘quarto-session2106081d’
    ‘quarto-session29357dbc’ ‘quarto-session4c8165df’
    ‘quarto-session5f186073’ ‘quarto-session71f26d61’
    ‘quarto-session73d551e0’ ‘quarto-sessiona3c1b791’
    ‘quarto-sessiona4c6ab9f’ ‘quarto-sessiond4bd60ff’

I'm guessing these temp files are being opened when testing quarto renders. Do you know how to get rid of them?

[gadenbuie]

I'm guessing these temp files are being opened when testing quarto renders. Do you know how to get rid of them?

Are you only seeing those files locally? I'm not seeing evidence that this happens in our tests...

...which of course led me to poke around and to discover that we're still seeing the Crashpad file in our ubuntu tests. I've gotta tell you, I've had it with this monkey fighting Crashpad file in our Monday to Friday CI checks (lol).

It's incredibly hard to tell what the right move is here. There's a reasonably large chance that we wouldn't even encounter that problem on CRAN because we might skip the test or example that creates the file. So one option is to submit and find out. It's also very hard to completely replicate CRAN's CI process so there's also a reasonably large chance that the only way we'll find out is by submitting to CRAN. An intermediate step here would be to try to set up an additional rcmdcheck step that tries to emulate CRAN on ubuntu for r-devel (effort level medium).

The other possibility is to try to get to the bottom of which test or example creates that file. My best guess as to how to do that would be to remove our tests and then add them back in one by one to see when the file shows up. (It could be from examples too, and the first run without tests would point to problematic examples.)

In related news: we recently got a new puppy and you just moved, so I'm sure our available free time for messing with this is pretty low.

[jhelvy]

Yeah nobody has time to deal with these Monkey fighting CRAN notes. I certainly don't have time to run through each example one at a time to identify the source. I'm thinking I might just submit it to see if that process helps find the source. It seems to me the most timely way to do so, and as you said if we can't even reproduce the CRAN CI process ourselves then this seems like the most logical thing to do. If you agree I'll get started on submitting it as v0.2.0.

[gadenbuie]

I agree! Submit and 🤞