-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-21805][SPARKR] Disable R vignettes code on Windows #19016
Conversation
Test build #80965 has finished for PR 19016 at commit
|
retest this please |
what's with jenkins? looks like JIRA is not updated either... |
Yes ... for JIRA linking problem, there are some information here - http://apache-spark-developers-list.1001551.n3.nabble.com/Some-PRs-not-automatically-linked-to-JIRAs-td22067.html just FYI. I think -9 has increased recently.. |
Test build #80972 has finished for PR 19016 at commit
|
Thanks @felixcheung ! Are the warnings about the missing PDF unavoidable ? I see something like
|
I think that is caused by the way I build the source package - I use the option --no-build-vignettes so the source package I test with doesn't have the PDF in it
Similarly for the failed test - I change the version to 2.2.0 to get it to run without setting the download URL and there is a change on the wrapper class parameter list.
|
Ah I see. Yeah the failed tests makes sense. We can also try to submit a custom tar.gz to r-hub to test it with the PDF and a different version number ? |
It likely best to back port this to branch-2.2 so the test can pass with matching API in a released jar.
Then we can see everything passing (except having to hand edit the version number)
|
@@ -27,6 +27,17 @@ vignette: > | |||
limitations under the License. | |||
--> | |||
|
|||
```{r setup, include=FALSE} | |||
library(knitr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, the opt_hooks is provided by knitr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes!
Sure - change LGTM. Lets see if @HyukjinKwon has any more comments ? If not we can merge to master, branch-2.2 and then do some more tests. |
Test build #81040 has finished for PR 19016 at commit
|
Sure, I have no more comments and it looks good to me too. |
## What changes were proposed in this pull request? Code in vignettes requires winutils on windows to run, when publishing to CRAN or building from source, winutils might not be available, so it's better to disable code run (so resulting vigenttes will not have output from code, but text is still there and code is still there) fix * checking re-building of vignette outputs ... WARNING and > %LOCALAPPDATA% not found. Please define the environment variable or restart and enter an installation path in localDir. ## How was this patch tested? jenkins, appveyor, r-hub before: https://artifacts.r-hub.io/SparkR_2.2.0.tar.gz-49cecef3bb09db1db130db31604e0293/SparkR.Rcheck/00check.log after: https://artifacts.r-hub.io/SparkR_2.2.0.tar.gz-86a066c7576f46794930ad114e5cff7c/SparkR.Rcheck/00check.log Author: Felix Cheung <felixcheung_m@hotmail.com> Closes #19016 from felixcheung/rvigwind. (cherry picked from commit 43cbfad) Signed-off-by: Felix Cheung <felixcheung@apache.org>
thanks, merged to master/2.2. |
I tested with the nightly, it passed |
Thats great ! I will also run this by winbuilder later today. |
## What changes were proposed in this pull request? Code in vignettes requires winutils on windows to run, when publishing to CRAN or building from source, winutils might not be available, so it's better to disable code run (so resulting vigenttes will not have output from code, but text is still there and code is still there) fix * checking re-building of vignette outputs ... WARNING and > %LOCALAPPDATA% not found. Please define the environment variable or restart and enter an installation path in localDir. ## How was this patch tested? jenkins, appveyor, r-hub before: https://artifacts.r-hub.io/SparkR_2.2.0.tar.gz-49cecef3bb09db1db130db31604e0293/SparkR.Rcheck/00check.log after: https://artifacts.r-hub.io/SparkR_2.2.0.tar.gz-86a066c7576f46794930ad114e5cff7c/SparkR.Rcheck/00check.log Author: Felix Cheung <felixcheung_m@hotmail.com> Closes apache#19016 from felixcheung/rvigwind.
## What changes were proposed in this pull request? Code in vignettes requires winutils on windows to run, when publishing to CRAN or building from source, winutils might not be available, so it's better to disable code run (so resulting vigenttes will not have output from code, but text is still there and code is still there) fix * checking re-building of vignette outputs ... WARNING and > %LOCALAPPDATA% not found. Please define the environment variable or restart and enter an installation path in localDir. ## How was this patch tested? jenkins, appveyor, r-hub before: https://artifacts.r-hub.io/SparkR_2.2.0.tar.gz-49cecef3bb09db1db130db31604e0293/SparkR.Rcheck/00check.log after: https://artifacts.r-hub.io/SparkR_2.2.0.tar.gz-86a066c7576f46794930ad114e5cff7c/SparkR.Rcheck/00check.log Author: Felix Cheung <felixcheung_m@hotmail.com> Closes apache#19016 from felixcheung/rvigwind. (cherry picked from commit 43cbfad) Signed-off-by: Felix Cheung <felixcheung@apache.org>
What changes were proposed in this pull request?
Code in vignettes requires winutils on windows to run, when publishing to CRAN or building from source, winutils might not be available, so it's better to disable code run (so resulting vigenttes will not have output from code, but text is still there and code is still there)
fix * checking re-building of vignette outputs ... WARNING
and
How was this patch tested?
jenkins, appveyor, r-hub
before: https://artifacts.r-hub.io/SparkR_2.2.0.tar.gz-49cecef3bb09db1db130db31604e0293/SparkR.Rcheck/00check.log
after: https://artifacts.r-hub.io/SparkR_2.2.0.tar.gz-86a066c7576f46794930ad114e5cff7c/SparkR.Rcheck/00check.log