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

Regression in R 3.4 #2052

Closed
t-kalinowski opened this issue Sep 28, 2021 · 16 comments
Closed

Regression in R 3.4 #2052

t-kalinowski opened this issue Sep 28, 2021 · 16 comments
Labels
bug Bugs next Issues/PRs considered for the next release

Comments

@t-kalinowski
Copy link

t-kalinowski commented Sep 28, 2021

Hi,

I recently expanded the CI tests for the keras R package and I noticed that R CMD check would fail due to vignette chunks being evaluated that shouldn't be evaluated. It seems that knitr::opts_chunk$set(eval = FALSE) isn't respected in the latest few versions of knitr if R <= 3.4. Installing knitr version 1.30 makes my tests pass again.

@yihui
Copy link
Owner

yihui commented Sep 28, 2021

Could you read and follow the issue guide exactly to provide the required information? Otherwise it is usually hard for us to help you. In your case, it's difficult to know what the problem could be without a reproducible example. If it is impractical to provide a reproducible example, it will be helpful to let us know which exact version broke your package (e.g., you said 1.30 works, and does that mean 1.31 broke it?). Thanks!

@t-kalinowski
Copy link
Author

t-kalinowski commented Sep 28, 2021

Hi, apologies for not fleshing this out fully. I made a skeleton R package with some GHA workflows that demonstrate the issue:
You can see the failed run here: https://github.com/t-kalinowski/knitr.regression/runs/3735193979
relevant output, present in R 3.4 but not 3.5+:

── R CMD check results ───────────────────────────── knitr.regression 0.1.0 ────
Duration: 25.1schecking running R code from vignettes ...a_vignette.RmdusingUTF-8... failed
   ERROR
  Errors in running code in vignettes:
  when running code ina_vignette.Rmd...
  
  > knitr::opts_chunk$set(eval = FALSE)
  
  > stop("I shouldn't run!")
  
    When sourcinga_vignette.R:
  Error: I shouldn't run!
  Execution halted

1 error ✖ | 0 warnings ✔ | 0 notes ✔
Error: Error: R CMD check found ERRORs

@t-kalinowski
Copy link
Author

and does that mean 1.31 broke it?

Yes

@yihui
Copy link
Owner

yihui commented Sep 28, 2021

Hmm... That's weird. I looked at the changes in 1.31 but didn't see a possible change that could have led to the breakage: v1.30...v1.31 I'll investigate further. Thanks a lot for the minimal reproducible example! I really appreciate it.

@yihui
Copy link
Owner

yihui commented Sep 28, 2021

Identified the offending commit: d35590d but still need to understand why the problem only occurs with R 3.4.4. @cderv Do you have R 3.4.4 to test with?

@yihui yihui closed this as completed in 87d094a Sep 28, 2021
@yihui yihui added bug Bugs next Issues/PRs considered for the next release labels Sep 28, 2021
@yihui
Copy link
Owner

yihui commented Sep 28, 2021

@cderv Please ignore this issue. I've figured it out.

@t-kalinowski It's fixed now. Unfortunately I just made a CRAN release this morning, so I can't make another one in a short time (may need to wait for at least a month for the next CRAN release). Thanks for your patience!

@halldc
Copy link

halldc commented Sep 28, 2021

Hi @yihui - I'm encountering this same issue on R 4.1.1 when running rcmdcheck::rcmdcheck().

@yihui
Copy link
Owner

yihui commented Sep 28, 2021

@halldc Could you please provide a reproducible example? Thanks!

@halldc
Copy link

halldc commented Sep 28, 2021

@halldc
Copy link

halldc commented Sep 28, 2021

@yihui - I suspect this is related to #2036 interacting with {rcmdcheck}.

@yihui
Copy link
Owner

yihui commented Sep 28, 2021

@halldc You are correct---the check in Github action didn't use the --as-cran flag. Unfortunately I was wrong in #2036 (review) that it should be rare to run R CMD check without --as-cran.

For now, the workaround is to either use --as-cran (https://github.com/yihui/knitr.regression/commit/0af676ac41cd5f29994dc5d114eb2f825c618337), or set _R_CHECK_VIGNETTES_SKIP_RUN_MAYBE_ to TRUE like this: https://github.com/rstudio/keras/pull/1275/files.

I'll fix this problem in the next version of knitr.

@halldc
Copy link

halldc commented Sep 28, 2021

Thanks @yihui! Yep, I reached the same conclusion and I'm using the second option for now. 🙂

FYI - I'm not using --as-cran because the package is an internal package at a company. Many of the CRAN-specific checks are inappropriate in this setting (e.g. warnings about unreachable intranet URLs).

@yihui
Copy link
Owner

yihui commented Sep 29, 2021

I have submitted a new version of knitr to CRAN, which is pending manual inspection right now (because CRAN requires at least 7 days between two releases, otherwise a CRAN maintainer will have to manually check it). Hopefully this problem will go away in a day or two. Apologies for the trouble, and thanks for your patience!

@halldc
Copy link

halldc commented Sep 29, 2021

Thanks for the quick resolution, @yihui ! 🎉

@yihui
Copy link
Owner

yihui commented Sep 29, 2021

You are welcome! The new version was accepted on CRAN about 7 hours ago.

t-kalinowski added a commit to rstudio/keras3 that referenced this issue Sep 29, 2021
@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bugs next Issues/PRs considered for the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants