-
Notifications
You must be signed in to change notification settings - Fork 289
Add use_circleci() #703
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
Add use_circleci() #703
Conversation
I'm releasing usethis today, so it's about to rest for a little while. But we anticipate another release before useR! |
Hi @jdblischak I used this PR as a demo of |
@jennybc Understood. Thanks! |
Some potential ideas to improve the default configuration (with the downside of increased complexity): Report installation informationSimilar to the current setup for Travis or Appveyor, we could add a step that reports the installation information, e.g.
Separate cache for each jobIf a user tests multiple versions of R in different jobs, they won't want to use the same cache. We could make the default caching mechanism slightly more complicated by default to anticipate this use case. The following will store a separate cache for each job:
|
inst/templates/circleci-config.yml
Outdated
- checkout | ||
- run: | ||
name: Install package dependencies | ||
command: R -e "devtools::install_deps(dependencies = TRUE)" |
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.
Probably best to use the remotes packages for this, something like
R -e 'if (!require("remotes")) install.packages("remotes")' -e 'remotes::install_deps(dependencies = TRUE)'
We might even just want to unconditionally call install.packages("remotes")
if for no other reason that to ensure it is installed and is the latest version from CRAN.
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.
To ensure it was always up-to-date, I think we'd need to unconditionally install it. The rocker image verse already has remotes/devtools installed.
inst/templates/circleci-config.yml
Outdated
- save_cache: | ||
key: cache | ||
paths: | ||
- "/usr/local/lib/R/site-library" |
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.
At least the rocker containers set the R_LIBS_USER
environment variable (to this location), maybe this should be $R_LIBS_USER ? Although I am not sure if circleci will expand the environment variable here...
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.
Also IIRC because this is the default library location for the verse docker container, the CircleCI cache will contain all of the package already installed in the library. It might be better to do something like R_LIBS_USER=/foo/bar:${R_LIBS_USER}
and setup new library that is only dependencies installed by CircleCI.
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.
This might be possible using parameters.
But I do think that most users will use a rocker image, and any user that can create an R environment from a more minimal Docker image (e.g. a standard Ubuntu image) should be able to handle updating the library path.
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.
R_LIBS_USER
has to be set in .Renviron
, right? Do you want me to add a step to create this file?
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.
No, it is just a normal shell environment variable, it can be set like you would normally in the CI.
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.
Starting a Docker container with docker run --rm -it rocker/verse:3.5.2 bash
export R_LIBS_USER=~/R/Library
echo $R_LIBS_USER
## /root/R/Library/
mkdir -p $R_LIBS_USER
Rscript -e 'Sys.getenv("R_LIBS_USER")'
## [1] "/usr/local/lib/R/site-library"
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.
OK. I confirmed that the problem is that the rocker image is setting R_LIBS_USER
in a system Renviron
(line). When I delete that line, then it respects the setting of the environment variable.
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.
@jimhester Since setting R_LIBS_USER
as a standard environment variable didn't work in the rocker image, I tried defining it in .Renviron
. This was at least recognized by R, but it was added to the end of the of the library path.
mkdir -p ~/R/Library
echo R_LIBS_USER=~/R/Library > ~/.Renviron
Rscript -e '.libPaths()'
## [1] "/usr/local/lib/R/site-library" "/usr/local/lib/R/library"
## [3] "/root/R/Library"
Thus I then tried using .Rprofile
. I've had to resort to this hack before in order to configure R properly on my HPC. Note that only the call to .libPaths()
is actually required. Including the call to Sys.setenv()
is more for an end user trying to understand how the library paths were setup.
mkdir -p ~/R/Library
echo 'Sys.setenv(R_LIBS_USER = "~/R/Library")' > ~/.Rprofile
echo '.libPaths("~/R/Library")' >> ~/.Rprofile
cat ~/.Rprofile
## Sys.setenv(R_LIBS_USER = "~/R/Library")
## .libPaths("~/R/Library")
Rscript -e '.libPaths()'
## [1] "/root/R/Library" "/usr/local/lib/R/site-library"
## [3] "/usr/local/lib/R/library"
Another alternative would be to set lib = "~/R/Library"
when calling install.packages()
and remotes::install_deps()
.
Thoughts on which solution I should implement?
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.
The problem is the rocker container is also setting R_LIBS
and R_LIBS
takes precedence over R_LIBS_USER
, so you have to use that in this case.
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.
Setting R_LIBS
worked. Thanks! By setting R_LIBS=~/R/library
and creating this directory, .libPaths()
returns the desired order:
export R_LIBS=~/R/library
mkdir -p ~/R/library
Rscript -e '.libPaths()'
## [1] "/root/R/library" "/usr/local/lib/R/site-library"
## [3] "/usr/local/lib/R/library"
inst/templates/circleci-config.yml
Outdated
steps: | ||
- restore_cache: | ||
keys: | ||
- cache |
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.
From reading https://circleci.com/docs/2.0/caching/#using-keys-and-templates I think we would want something like
- r-pkg-cache-{{ arch }}-{{ .Branch }}
- r-pkg-cache-{{ arch }}-
This would give you specific caches for the architecture and branch, but would fall back to caches if they are available not available.
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.
OK. I can update the caching logic.
@jimhester This PR is ready for another round of review. Thanks! |
inst/WORDLIST
Outdated
organisations | ||
pkgbuild | ||
======= |
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.
looks like you have some unresolved conflict markers here
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.
Strange. I didn't think Git would allow me to add the file and git rebase --continue
if the file still contained the conflict markers. I'll fix that real quick.
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.
No by default, but you can use git diff --check
to warn about them, or put this in a pre-commit hook if you want to automate it.
LGTM other than the conflict marker issue. |
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.
Thanks to @jdblischak for the PR and @jimhester for reviewing. I've taken another pass, with a different hat on. Just a few small changes.
R/ci.R
Outdated
data = list(package = project_name(), image = image), | ||
ignore = TRUE | ||
) | ||
if (!new) return(invisible(FALSE)) |
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.
Can you run styler over any code that you're touching? I think we want this in curly braces, with newlines.
@@ -0,0 +1,38 @@ | |||
context("use_circleci") | |||
|
|||
test_that("uses_circleci() reports usage of CircleCI", { |
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.
Can you add skip_if_no_git_config()
?
tests/testthat/test-use-circleci.R
Outdated
scoped_temporary_package() | ||
expect_false(uses_circleci()) | ||
use_git() | ||
git2r::remote_add(name = "origin", url = "https://github.com/fake/fake") |
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.
Please switch over to usethis::use_git_remote()
. We will probably switch over to gert at some point, so I don't want to gain any new git2r calls.
tests/testthat/test-use-circleci.R
Outdated
|
||
test_that("check_uses_circleci() can throw error", { | ||
scoped_temporary_package() | ||
expect_error(check_uses_circleci(), |
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.
I think styler would re-format this too.
"Do you need to run `use_circleci\\(\\)`") | ||
}) | ||
|
||
test_that("use_circleci() configures CircleCI", { |
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.
Can you add skip_if_no_git_config()
?
tests/testthat/test-use-circleci.R
Outdated
test_that("use_circleci() configures CircleCI", { | ||
scoped_temporary_package() | ||
use_git() | ||
git2r::remote_add(name = "origin", url = "https://github.com/fake/fake") |
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.
Please switch over to usethis::use_git_remote()
.
tests/testthat/test-use-circleci.R
Outdated
expect_proj_dir(".circleci") | ||
expect_proj_file(".circleci/config.yml") | ||
yml <- yaml::yaml.load_file(".circleci/config.yml") | ||
expect_identical(yml$jobs$build$steps[[7]]$store_artifacts$path, |
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.
styler might change this
paste0(project_name(), ".Rcheck/")) | ||
}) | ||
|
||
test_that("use_circleci() specifies Docker image", { |
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.
Same tweaks as above
@jennybc Thanks for the review! I had clearly overlooked the bullet point about the code style. I think I've addressed everything, so please let me know if I missed anything. |
Thanks! |
I wrote a function
use_circleci()
to setup continuous integration with CircleCI. I modeled it afteruse_travis()
.Features
The configuration file it creates enables the following features:
Uses the Docker image
rocker/verse:latest
(DockerHub) as the environment for building and checking the package. This image contains TeX Live, pandoc, and the tidyverse packages, so it should be sufficient for many R packages. However, the user has the option of specifying the Docker image to use via the argumentimage
.Very simple caching of packages. The CircleCI platform provides a complex caching mechanism, but I chose not to use this since
devtools::install_deps()
can update packages as needed. Also, the default is for the cache to expire after 30 days. If desired, a more sophisticated option would be to invalidate the cache whenever a specific file is changed, e.g. using a key likecache-{{ checksum "DESCRIPTION" }}
.The
*.Rcheck/
directory is uploaded as an artifact at the end of build.Other notes
ui_
that is used incheck_uses_travis()
(line 67), so I usedui_field()
incheck_uses_circleci()
NEWS.md