yihui / knitr

A general-purpose tool for dynamic report generation in R
https://yihui.org/knitr/
2.38k stars 878 forks source link

Move the markdown package from Imports to Suggests #1864

Closed yihui closed 3 years ago

yihui commented 4 years ago

For package authors who use R Markdown vignettes based on the vignette engine knitr::knitr (or knitr::docco_linear, knitr::docco_classic, etc.), you should declare the (soft) dependency on markdown, e.g., in the package DESCRIPTION:

Suggests: markdown
VignetteBuilder: knitr

Previously they didn't need to do this because markdown has been a hard dependency of knitr (in Imports), so the availability of markdown is guaranteed by knitr, but I want to make markdown a soft dependency of knitr in the future.

I think most packages use the vignette engine knitr::rmarkdown to write vignettes now, but there may still be a few vignettes using the knitr::knitr engine, which depends on markdown. If I move markdown to Suggests in knitr, users have to make sure markdown is available to R CMD check. To achieve that, they have to add markdown to Suggests to their DESCRIPTION files.

Similarly, if your vignette is based on the vignette engine knitr::rmarkdown, you have to declare the dependency on rmarkdown, e.g.,

Suggests: rmarkdown
VignetteBuilder: knitr

The error message when building the source package via R CMD build should tell you if you need to add markdown or rmarkdown to Suggests, which looks like this:

* checking for file ‘PKG/DESCRIPTION’ ... OK
* preparing ‘PKG’:
* checking DESCRIPTION meta-information ... OK
* installing the package to build vignettes
* creating vignettes ... ERROR
--- re-building ‘doc.Rmd’ using rmarkdown_notangle
Error: processing vignette 'doc.Rmd' failed with diagnostics:
The 'rmarkdown' package should be declared as a dependency of
the 'PKG' package (e.g., in the  'Suggests' field of DESCRIPTION),
because the latter contains vignette(s) built with the 'rmarkdown'
package. Please see https://github.com/yihui/knitr/issues/1864
for more information.
--- failed re-building ‘doc.Rmd’

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

Error: Vignette re-building failed.
Execution halted

If you are a package author affected by this issue, but not clear about what you need to do, please reply below and I'll be glad to help. I recommend that you skip the replies below. Thanks!

tdhock commented 4 years ago

hi I got this from win-builder:


* checking re-building of vignette outputs ... [1s] WARNING
Error(s) in re-building vignettes:
--- re-building 'examples.Rmd' using knitr
Error: processing vignette 'examples.Rmd' failed with diagnostics:
The 'markdown' package should be declared as a dependency of the 'directlabels' package (e.g., in the  'Suggests' field of DESCRIPTION), because it contains vignette(s) built with the 'markdown' package. Please see https://github.com/yihui/knitr/issues/1864 for more information.
--- failed re-building 'examples.Rmd'

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

Error: Vignette re-building failed.
Execution halted

my vignette uses

%\VignetteEngine{knitr::knitr}

which I think does not require markdown (even in suggests), if I understand your discussion above correctly. so it seems strange that I would need to add markdown in suggests. do I really have to?

yihui commented 4 years ago

Yes, you have to.

tdhock commented 4 years ago

ok

mschubert commented 4 years ago

I think I'm getting a false positive warning on the CRAN Winbuilder. I've got all my vignettes set to

%\VignetteEngine{knitr::rmarkdown}

If I have not set in DESCRIPTION that

Suggests:
    rmarkdown

I'm getting no warnings locally (R=4.0.2, rmarkdown=2.2, knitr=1.28). On the CRAN Winbuilder, I'm getting:

Error: processing vignette 'quickstart.Rmd' failed with diagnostics: The 'markdown' package should be declared as a dependency of the 'clustermq' package (e.g., in the 'Suggests' field of DESCRIPTION), because it contains vignette(s) built with the 'markdown' package. Please see https://github.com/yihui/knitr/issues/1864 for more information.

The missing package, however, is Suggests: rmarkdown and not markdown.

yihui commented 4 years ago

@mschubert I need a reproducible example. Could you provide a link to your package (presumably on Github?)? Thanks!

mschubert commented 4 years ago

Package was clustermq:3d223af and you can find the Winbuilder output here. I won't have time to track this down further unfortunately (since adding Suggests: rmarkdown solves the issue & a warning for the right package is there), sorry.

yihui commented 4 years ago

@mschubert In your case, you are using the vignette engine knitr::rmarkdown, so you have to add rmarkdown to Suggests (which you did https://github.com/mschubert/clustermq/commit/02e06f87b3b6997981210e7be80ea28c02fe8e46): https://yihui.org/knitr/demo/vignette/ Because you didn't do that, you saw the message about markdown, and that was because knitr fell back to using markdown to build your vignettes. Neither rmarkdown nor markdown was specified in your Suggests, and that's why you saw the error.

mschubert commented 4 years ago

Yes, that's what I tried to say - sorry if that was not clear. My comment was that the error message of "missing markdown package in Suggests" is confusing when the vignette states that it uses rmarkdown (and the markdown package should never be required).

Perhaps a better error message would be to add whatever markdown builder you use to Suggests? Or, maybe do nothing, but I thought it was useful to document that this message is also displayed when rmarkdown is missing (I guess people will google the issue and find this).

yihui commented 4 years ago

@mschubert Yes, I definitely agree the messages were confusing in this case. I'll think more about it and try to make it clearer. Thanks a lot for your feedback!

nsheff commented 4 years ago

I agree with @mschubert. I had the same situation. I was about to add 'markdown' to suggests because of the message, when I really should have added 'rmarkdown' to suggests, because I use knitr::rmarkdown (if I understood correctly).

yihui commented 4 years ago

With knitr 1.30 (the current latest version on CRAN), the message should tell you to add rmarkdown instead of markdown to Suggests when the vignette engine knitr::rmarkdown is used.

jangorecki commented 3 years ago

Any idea when the actual moving to suggests may happen?

yihui commented 3 years ago

@jangorecki It may happen relatively soon. I'm talking to CRAN and if they could forgive me for breaking (potentially a small number of) packages that didn't declare markdown in Suggests, I'll move markdown to Suggests in knitr in the next CRAN release. There might be some BioC packages that will be affected, and I'm not sure how many will still have this problem after six months.

dnadave commented 3 years ago

Is there a way to get around this issue? This error is preventing us from submitting two packages to Bioconductor...

yihui commented 3 years ago

@dnadave Yes, please do what the error message told you to do.

yihui commented 3 years ago

@jangorecki I talked to CRAN maintainers last month and about 470 CRAN packages will be broken if I moved markdown to Suggests. This is not a trivial number, and I definitely don't want these packages to be killed just because of this minor issue. The next step for me is to write to these package maintainers and see how many would be willing to make this small change and update their packages on CRAN.

terrytangyuan commented 3 years ago

With knitr 1.30 (the current latest version on CRAN), the message should tell you to add rmarkdown instead of markdown to Suggests when the vignette engine knitr::rmarkdown is used.

@yihui Looks like I am still getting the markdown error even though I already have rmarkdown in DESCRIPTION file: https://github.com/terrytangyuan/autoplotly/blob/d5bfe555fe6261964f2e79512f73cf215e142dff/DESCRIPTION#L30

The CI is using knitr==1.32. Please see the build log for more details: https://travis-ci.org/github/terrytangyuan/autoplotly/builds/767305693

Is there anything I missed?

nsheff commented 3 years ago

@terrytangyuan I am also seeing something similar. I use rmarkdown in my vignette, and have rmarkdown in suggest, but the error persists:

The 'markdown' package should be declared as a dependency of the 'loadr' package (e.g., in the 'Suggests' field of DESCRIPTION), because the latter contains vignette(s) built with the 'markdown' package. Please see https://github.com/yihui/knitr/issues/1864 for more information.

If I add markdown instead of rmarkdown in the Suggests, the error disappears.

> sessionInfo()
R Under development (unstable) (2021-04-08 r80148)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 20.04.2 LTS

Matrix products: default
BLAS/LAPACK: /usr/lib/x86_64-linux-gnu/openblas-pthread/libopenblasp-r0.3.8.so

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=C             
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] loadr_0.1.3    testthat_3.0.2

loaded via a namespace (and not attached):
 [1] Rcpp_1.0.6        compiler_4.1.0    pillar_1.5.1      prettyunits_1.1.1
 [5] remotes_2.3.0     tools_4.1.0       digest_0.6.27     pkgbuild_1.2.0   
 [9] pkgload_1.2.1     memoise_2.0.0     lifecycle_1.0.0   tibble_3.1.0     
[13] pkgconfig_2.0.3   rlang_0.4.10      cli_2.4.0         rstudioapi_0.13  
[17] xopen_1.0.0       xfun_0.22         fastmap_1.1.0     withr_2.4.1      
[21] stringr_1.4.0     roxygen2_7.1.1    knitr_1.32        xml2_1.3.2       
[25] desc_1.3.0        fs_1.5.0          vctrs_0.3.7       devtools_2.4.0   
[29] rprojroot_2.0.2   glue_1.4.2        R6_2.5.0          processx_3.5.1   
[33] fansi_0.4.2       rcmdcheck_1.3.3   sessioninfo_1.1.1 callr_3.6.0      
[37] purrr_0.3.4       magrittr_2.0.1    ps_1.6.0          ellipsis_0.3.1   
[41] usethis_2.0.1     utf8_1.2.1        stringi_1.5.3     cachem_1.0.4     
[45] crayon_1.4.1     
nsheff commented 3 years ago

Follow-up: The above strange error is the behavior if you do not have rmarkdown installed. If you install.packages("rmarkdown"), then it works as expected.

The error messages are a bit confusing if you lack rmarkdown, I presume because then knitr falls back to markdown, and then yields the error suggesting to add markdown to suggests, instead of rmarkdown.

I think instead, the error message should reflect what is in the vignette builder and not depend on the package available in the current session.

terrytangyuan commented 3 years ago

Thanks but in my case the CI already installed rmarkdown==2.7 and still failed with that error. I also installed it explicitly here and got the same result (link to build).

yihui commented 3 years ago

The error messages are a bit confusing if you lack rmarkdown, I presume because then knitr falls back to markdown, and then yields the error suggesting to add markdown to suggests, instead of rmarkdown.

I think instead, the error message should reflect what is in the vignette builder and not depend on the package available in the current session.

@nsheff That's true, and also what I intended to do in the future: https://github.com/yihui/knitr/blob/3f89d2a2cb9e55fefe25d4b9768f3f757717ea22/R/utils-vignettes.R#L96

I thought it would be unlikely that package authors don't have rmarkdown installed when they have R Markdown vignettes. Anyway, I agree I should clarify it in the error message that rmarkdown must be installed in order to build R Markdown vignettes. Thanks!

nsheff commented 3 years ago

I thought it would be unlikely that package authors don't have rmarkdown installed when they have R Markdown vignettes

Indeed; in this case I wanted to start fresh, with the latest dev version of R, and so didn't have any packages pre-populated. But most of the time you're probably right. Still, it could bite people who took the route I did.

terrytangyuan commented 3 years ago

@yihui @nsheff Please see my comments above. The issue still appears when rmarkdown is installed.

yihui commented 3 years ago

@terrytangyuan You can ignore everything in this thread and just do exactly what the error message told you to do. That is, add markdown to Suggests (you still got the error because you added rmarkdown instead of markdown).

After you did that (https://github.com/terrytangyuan/autoplotly/commit/614ba38ad273688578debe334f83513f46fd421c), your CI passed: https://travis-ci.org/github/terrytangyuan/autoplotly/builds/767311497.

nsheff commented 3 years ago

@terrytangyuan Just to reiterate, your vignette does not appear to be using rmarkdown, so you should put markdown in the Suggests. I'm in a different situation since I'm using rmarkdown in my vignette, see: https://github.com/databio/loadr/blob/0124ef5548801bc5d82469313791045ca308d9f3/vignettes/loadrIntro.Rmd#L6

terrytangyuan commented 3 years ago

Yep got it working now. Thanks!

hpages commented 3 years ago

Gosh, this change in knitr 1.32 breaks ~333~ 253 Bioconductor packages in release and ~340~ 255 in devel!!

Both markdown and rmarkdown are installed on the BioC builders so there's no reason for knitr to not just go ahead and process the vignettes. As a general rule, if a package doesn't declare a dependency it's their problem and I don't think it should be knitr's business. Why not just move markdown and/or rmarkdown to Suggests in knitr and let things happen naturally? People will start declaring markdown or rmarkdown as a dependency of their package when it starts failing on a build machine where these packages are not installed.

yihui commented 3 years ago

@hpages First of all, my apologies for the hassle!

The error is not completely new---it was first introduced on June 21 last year, but it was signaled only during the CRAN incoming check, i.e. R CMD check with _R_CHECK_CRAN_INCOMING_REMOTE_=true. It seems BioC doesn't use this variable, so no one was aware of this upcoming change in knitr.

What I did in the last CRAN release of knitr was that I raised the error earlier, i.e., in R CMD build instead of R CMD check. For CRAN packages, this means package authors will only see the error locally when they build the source package, but not on CRAN. This was to minimize the impact of this change, but I didn't know that BioC runs R CMD build before R CMD check. I'm not familiar with the BioC package submission or build process.

Trust me that I hate disturbing package authors like this. I didn't set a deadline for this issue and was okay with waiting for a couple of years. However, after a few email exchanges with CRAN, it became clear that my previous gentle warning conditional on _R_CHECK_CRAN_INCOMING_REMOTE_=true was troublesome to CRAN, so I changed the strategy to avoid disturbing CRAN maintainers, but unfortunately it affected BioC.

Both markdown and rmarkdown are installed on the BioC builders so there's no reason for knitr to not just go ahead and process the vignettes.

There is a reason... If you don't declare the (soft) dependency, these packages won't be available during R CMD check --as-cran, even if they are installed (because _R_CHECK_SUGGESTS_ONLY_=true). Perhaps BioC uses a different setting for checking packages.

Why not just move markdown and/or rmarkdown to Suggests in knitr and let things happen naturally?

I've wished I could do that for several years, but that means about 450 CRAN packages would fail on CRAN, and CRAN usually gives only about two weeks for authors to fix check issues (although in this case, they allowed for about one month). Again, I didn't want to push package authors to fix this problem in a hurry, so I've been trying to hide the problem (with a fallback mechanism) from R CMD check for years.

If there is a way to detect if R CMD build or check is running on BioC (e.g., an env var), I'll be more than happy to use it to "let things happen naturally."

Apologies again, and thanks for your understanding!

hpages commented 3 years ago

testthat::skip_on_bioc() uses Sys.getenv("BBS_HOME") to detect if R is running on a BioC build machine. But...

There is a reason... If you don't declare the (soft) dependency, these packages won't be available during R CMD check --as-cran, even if they are installed (because _R_CHECK_SUGGESTSONLY=true)

I understand but this is a particular setup, not the default. The default is to use R CMD check (without --as-cran) and to not have any _R_CHECK_*_ environment variable defined. This default setup also happens to be the Bioconductor setup. Ideally you'd want to let things happen naturally by default and enable the new check specifically for the CRAN setup, rather than enabling the new check by default and disabling it specifically on the Bioconductor build machines.

Thanks!

yihui commented 3 years ago

Okay. Thanks for the info! I'll probably make a patch release in the next few days to bypass the check for dependency during R CMD build on BioC.

Ideally you'd want to let things happen naturally by default and enable the new check specifically for the CRAN setup, rather than enabling the new check by default and disabling it specifically on the Bioconductor build machines.

Ideally, yes, I hate using hacks as much as everyone else does. All I've been trying to achieve is to send a reminder to package authors as gently as possible, so they can declare the new dependency without having CRAN chasing after them. I thought R CMD build would be a good place to do this because only they would see the error by themselves, but I didn't realize BioC has a different process.

hpages commented 3 years ago

Ok, thanks a lot for helping us with this! Hopefully your patch can make it to CRAN soon. Sorry for being pushy but our next release is next month and we're going to freeze the current release (BioC 3.12) and stop the BioC 3.12 daily builds very soon (in a couple of weeks). Really bad timing.

yihui commented 3 years ago

@hpages I'll make the patch release as soon as possible (hopefully tomorrow). Is there a chance that you could test the current dev version of knitr? I want to make sure the fix c2ff34f actually works for BioC so that I won't waste a chance of CRAN submission. Thanks!

hpages commented 3 years ago

@yihui We've just installed knitr 1.32.4 on all our build machines. Let's see what happens on the build report tomorrow. Fingers crossed. Thanks again for helping us with this.

yihui commented 3 years ago

@hpages It seems the build errors have been disappearing gradually?

hpages commented 3 years ago

Let me check and make sure that the knitr issue is gone. Today's report was posted just before I joined a meeting and I didn't have time to look at it yet. I'll get back to you in a few minutes.

hpages commented 3 years ago

No more "r/markdown package should be declared as a dependency" error on the report but now I see this one:

It seems you should call rmarkdown::render() instead of knitr::knit2html() because overview.Rmd appears to be an R Markdown v2 document.

It's breaking R CMD build on 60+ packages.

hpages commented 3 years ago

Correction: it's breaking 35 software packages in BioC 3.13. e.g. https://bioconductor.org/checkResults/3.13/bioc-LATEST/AWFisher/nebbiolo1-buildsrc.html

yihui commented 3 years ago

Okay. I just addressed this problem in the dev version of knitr. You may install it and let's see if the dust settles tomorrow.

hpages commented 3 years ago

Thanks. Today's builds have started already so it's too late for going on the build machines to update knitr. We'll do this tomorrow (Thursday) and wait until Friday morning to see what happens on the build report.

hpages commented 3 years ago

Alright, latest knitr is now on all the Bioconductor build machines. We're ready to go!

yangjasp commented 3 years ago

Hi,

I have a package vignette that uses the engine knitr::rmarkdown, and in my DESCRIPTION file I have declared:

Suggests: rmarkdown
VignetteBuilder: knitr

I believe that this structure follows the recommendation in the initial post for vignettes that use knitr::rmarkdown. My issue is that certain users who do not have rmarkdown installed are getting the following error when they try to install the package (Similar to the error in the initial post):

--- re-building 'doc.Rmd' using rmarkdown
   Error: processing vignette 'doc.Rmd' failed with diagnostics:
   The 'markdown' package should be declared as a dependency of the 'PKG' package (e.g., in the  'Suggests' field of DESCRIPTION), because the latter contains vignette(s) built with the 'markdown' package. Please see https://github.com/yihui/knitr/issues/1864 for more information.
   --- failed re-building 'doc.Rmd'

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

   Error: Vignette re-building failed.
   Execution halted

If the user installs rmarkdown, the error goes away and the package installs fine.

I am confused why the message says that I should add markdown to the 'Suggests' field when rmarkdown is already there and my vignette engine is knitr::rmarkdown. It also isn't exactly a helpful error message for the user to see when the solution is simply installing rmarkdown. Is there a better solution such as moving rmarkdown to the 'Depends' field?

nsheff commented 3 years ago

I am confused why the message says that I should add markdown to the 'Suggests' field when rmarkdown is already there and my vignette engine is knitr::rmarkdown. It also isn't exactly a helpful error message for the user to see when the solution is simply installing rmarkdown. Is there a better solution such as moving rmarkdown to the 'Depends' field?

@yangjasp this is exactly the issue I raised earlier in this thread, see above, and @yihui's response:

The error messages are a bit confusing if you lack rmarkdown, I presume because then knitr falls back to markdown, and then yields the error suggesting to add markdown to suggests, instead of rmarkdown. I think instead, the error message should reflect what is in the vignette builder and not depend on the package available in the current session.

@nsheff That's true, and also what I intended to do in the future:

https://github.com/yihui/knitr/blob/3f89d2a2cb9e55fefe25d4b9768f3f757717ea22/R/utils-vignettes.R#L96

I thought it would be unlikely that package authors don't have rmarkdown installed when they have R Markdown vignettes. Anyway, I agree I should clarify it in the error message that rmarkdown must be installed in order to build R Markdown vignettes. Thanks!

hpages commented 3 years ago

Hi @yihui,

Today's Bioconductor build report is out: https://bioconductor.org/checkResults/3.13/bioc-LATEST/

I don't see any knitr-related error, only this warning: https://bioconductor.org/checkResults/3.13/bioc-LATEST/HCAExplorer/nebbiolo1-buildsrc.html (and this package is deprecated anyways)

So we're good! :-)

How long do you think it's going to take to get the latest knitr on CRAN?

Thanks!

yangjasp commented 3 years ago

@nsheff Thanks for the response! I must have missed that detail, but in my case the 'Suggests' field DOES contain Suggests: rmarkdown. My understanding was that the problem for you and @mschubert was that you needed to add rmarkdown to 'Suggests', and doing so solved your issue.

My R CMD check runs fine, but the issue is coming up when users who do not have rmarkdown installed try to build the vignettes. Their errors are telling me that I need to add markdown (or rmarkdown, based on this comment) to my suggests when rmarkdown is already there, and the real issue is that they just need to install rmarkdown themselves. Is there anything I can do about this on my end?

yihui commented 3 years ago

How long do you think it's going to take to get the latest knitr on CRAN?

@hpages Hopefully in the next two days. I still have a Solaris issue reported by CRAN that needs to be addressed (but as usual, not straightforward to reproduce, unfortunately).

the real issue is that they just need to install rmarkdown themselves. Is there anything I can do about this on my end?

@yangjasp Nothing else needs to be done on your end. The error message will be clearer in the next version of knitr that they need to install rmarkdown.

hpages commented 3 years ago

I still have a Solaris issue reported by CRAN ... but as usual, not straightforward to reproduce

C'mon, everybody should have a spare Solaris box at home to test their package! /s

nsheff commented 3 years ago

Thanks for the response! I must have missed that detail, but in my case the 'Suggests' field DOES contain Suggests: rmarkdown

@yangjasp That was an earlier comment, not the one I was quoting. I'm referring to the comment where I say, exactly what you said, I had rmarkdown in suggests and... Just look a bit further down in the thread and you'll find this issue you raise, and the solution yihui had proposed... Anyway, it doesn't matter, just wait for the fixed message like yihui said, that's all I was trying to say. You can scroll up and read those other posts if you want more detail about why it works that way.

yihui commented 3 years ago

@hpages The new version of knitr is on CRAN now.

hpages commented 3 years ago

Wow, that was fast! Excellent. Thanks again for all the help.

mkoohafkan commented 3 years ago

Note that you can also get the misleading error about markdown if you are having trouble with your Pandoc version, even if you've correctly specified knitr as your builder and put rmarkdown in SUGGESTS. If rmarkdown can't find a recent enough version of Pandoc it will automatically fall back to markdown which will trigger the error.

* creating vignettes ... ERROR
Error: --- re-building ‘curve-creation.Rmd’ using rmarkdown
Warning in engine$weave(file, quiet = quiet, encoding = enc) :
  Pandoc (>= 1.12.3) not available. Falling back to R Markdown v1.
Error: Error: processing vignette 'curve-creation.Rmd' failed with diagnostics:
The 'markdown' package should be installed and declared as a dependency of the 'culvrt' package (e.g., in the 'Suggests' field of DESCRIPTION), because the latter contains vignette(s) built with the 'markdown' package. Please see https://github.com/yihui/knitr/issues/1864 for more information.

In my case I was using github actions to run R CMD CHECK, and it threw this error because I had not added uses: r-lib/actions/setup-pandoc@v1 to my YAML.