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

Add use_circleci() #703

Merged
merged 11 commits into from Jun 21, 2019
Merged

Add use_circleci() #703

merged 11 commits into from Jun 21, 2019

Conversation

@jdblischak
Copy link
Contributor

jdblischak commented Apr 5, 2019

I wrote a function use_circleci() to setup continuous integration with CircleCI. I modeled it after use_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 argument image.

  • 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 like cache-{{ checksum "DESCRIPTION" }}.

  • The *.Rcheck/ directory is uploaded as an artifact at the end of build.

Other notes

  • The tests I added passed on my local machine and on the AppVeyor and Travis builds of my forked repository
  • I couldn't find the function ui_ that is used in check_uses_travis() (line 67), so I used ui_field() in check_uses_circleci()
  • If this new function is accepted, I'll add a bullet point to NEWS.md
@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Apr 5, 2019

I'm releasing usethis today, so it's about to rest for a little while. But we anticipate another release before useR!

@jennybc jennybc added this to the Backlog milestone Apr 5, 2019
@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Apr 8, 2019

Hi @jdblischak I used this PR as a demo of pr_*() functions in usethis during our group meeting today, which is why you see this odd little commit from me.

@jdblischak

This comment has been minimized.

Copy link
Contributor Author

jdblischak commented Apr 8, 2019

I used this PR as a demo of pr_*() functions in usethis during our group meeting today, which is why you see this odd little commit from me.

@jennybc Understood. Thanks!

@jennybc jennybc removed this from the Backlog milestone Apr 10, 2019
@jdblischak

This comment has been minimized.

Copy link
Contributor Author

jdblischak commented May 17, 2019

Some potential ideas to improve the default configuration (with the downside of increased complexity):

Report installation information

Similar to the current setup for Travis or Appveyor, we could add a step that reports the installation information, e.g.

      - run:
          name: Session information and installed package versions
          command: |
            Rscript -e 'sessionInfo()'
            Rscript -e 'installed.packages()[, c("Package", "Version")]'
            Rscript -e 'rmarkdown::pandoc_version()'

Separate cache for each job

If 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:

    steps:
      - restore_cache:
          keys:
            - cache-{{ .Environment.CIRCLE_JOB }}

      - save_cache:
          key: cache-{{ .Environment.CIRCLE_JOB }}
          paths:
            - "/usr/local/lib/R/site-library"
- checkout
- run:
name: Install package dependencies
command: R -e "devtools::install_deps(dependencies = TRUE)"

This comment has been minimized.

Copy link
@jimhester

jimhester May 17, 2019

Member

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.

This comment has been minimized.

Copy link
@jdblischak

jdblischak May 17, 2019

Author Contributor

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.

- save_cache:
key: cache
paths:
- "/usr/local/lib/R/site-library"

This comment has been minimized.

Copy link
@jimhester

jimhester May 17, 2019

Member

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...

This comment has been minimized.

Copy link
@jimhester

jimhester May 17, 2019

Member

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.

This comment has been minimized.

Copy link
@jdblischak

jdblischak May 17, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@jdblischak

jdblischak May 17, 2019

Author Contributor

R_LIBS_USER has to be set in .Renviron, right? Do you want me to add a step to create this file?

This comment has been minimized.

Copy link
@jimhester

jimhester May 20, 2019

Member

No, it is just a normal shell environment variable, it can be set like you would normally in the CI.

This comment has been minimized.

Copy link
@jdblischak

jdblischak May 21, 2019

Author Contributor

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"

This comment has been minimized.

Copy link
@jdblischak

jdblischak May 21, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@jdblischak

jdblischak May 28, 2019

Author Contributor

@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?

This comment has been minimized.

Copy link
@jimhester

jimhester May 29, 2019

Member

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.

This comment has been minimized.

Copy link
@jdblischak

jdblischak Jun 11, 2019

Author Contributor

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" 
steps:
- restore_cache:
keys:
- cache

This comment has been minimized.

Copy link
@jimhester

jimhester May 17, 2019

Member

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.

This comment has been minimized.

Copy link
@jdblischak

jdblischak May 17, 2019

Author Contributor

OK. I can update the caching logic.

@jdblischak

This comment has been minimized.

Copy link
Contributor Author

jdblischak commented Jun 11, 2019

@jimhester This PR is ready for another round of review. Thanks!

@jdblischak jdblischak force-pushed the jdblischak:circleci branch from 31fe11c to e1e6890 Jun 11, 2019
organisations
pkgbuild
=======

This comment has been minimized.

Copy link
@jimhester

jimhester Jun 13, 2019

Member

looks like you have some unresolved conflict markers here

This comment has been minimized.

Copy link
@jdblischak

jdblischak Jun 13, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@jimhester

jimhester Jun 19, 2019

Member

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.

@jimhester

This comment has been minimized.

Copy link
Member

jimhester commented Jun 13, 2019

LGTM other than the conflict marker issue.

Copy link
Member

jennybc left a comment

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))

This comment has been minimized.

Copy link
@jennybc

jennybc Jun 19, 2019

Member

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", {

This comment has been minimized.

Copy link
@jennybc

jennybc Jun 19, 2019

Member

Can you add skip_if_no_git_config()?

scoped_temporary_package()
expect_false(uses_circleci())
use_git()
git2r::remote_add(name = "origin", url = "https://github.com/fake/fake")

This comment has been minimized.

Copy link
@jennybc

jennybc Jun 19, 2019

Member

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.


test_that("check_uses_circleci() can throw error", {
scoped_temporary_package()
expect_error(check_uses_circleci(),

This comment has been minimized.

Copy link
@jennybc

jennybc Jun 19, 2019

Member

I think styler would re-format this too.

"Do you need to run `use_circleci\\(\\)`")
})

test_that("use_circleci() configures CircleCI", {

This comment has been minimized.

Copy link
@jennybc

jennybc Jun 19, 2019

Member

Can you add skip_if_no_git_config()?

test_that("use_circleci() configures CircleCI", {
scoped_temporary_package()
use_git()
git2r::remote_add(name = "origin", url = "https://github.com/fake/fake")

This comment has been minimized.

Copy link
@jennybc

jennybc Jun 19, 2019

Member

Please switch over to usethis::use_git_remote().

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,

This comment has been minimized.

Copy link
@jennybc

jennybc Jun 19, 2019

Member

styler might change this

paste0(project_name(), ".Rcheck/"))
})

test_that("use_circleci() specifies Docker image", {

This comment has been minimized.

Copy link
@jennybc

jennybc Jun 19, 2019

Member

Same tweaks as above

@jdblischak

This comment has been minimized.

Copy link
Contributor Author

jdblischak commented Jun 20, 2019

@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.

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Jun 21, 2019

Thanks!

@jennybc jennybc merged commit 8f6cc34 into r-lib:master Jun 21, 2019
4 checks passed
4 checks passed
codecov/patch 90.32% of diff hit (target 46.24%)
Details
codecov/project 46.32% (+0.08%) compared to 2baa06f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jdblischak jdblischak deleted the jdblischak:circleci branch Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.