JoSS Review 2: Repository and Install

vsoch commented 4 years ago

Repository

I can confirm that the source code is available at https://github.com/o2r-project/containerit/, and the VERSION is correct. The authorship is appropriate, and an OSI approved LICENSE is included.

Contributing

The author should consider adding a CONTRIBUTING.md, which can be in the root of the respository or a .github folder. The Contributor's Covenant is popular, but others are available (you can do a search online for templates, etc.) It's good practice to have sections in the README that describe broadly how to contribute (and link to that file) along with how to ask for support (questions, issues, etc.)

Installation

The steps are very nicely presented in the README! I'm not a super active R user, so I don't have the remotes::install_github command (how to load it from a library) in my head. Could this be provided?

# install.packages("remotes")
remotes::install_github("o2r-project/containerit")

Just a note that I hit this error - my version of R is old enough so it would be expected to use bioconductor and not BiocManager:

Error: Failed to install 'containerit' from GitHub:
  (converted from warning) package ‘BiocManager’ is not available (for R version 3.4.4)

I decided to instead install in a Docker image:

docker run --rm -p 8787:8787 -e PASSWORD=meatballs rocker/rstudio

That seems to start to work, but fails with:

trying URL 'https://mran.microsoft.com/snapshot/2019-08-06/src/contrib/digest_0.6.20.tar.gz'
Error in download.file(url, destfile, method, mode = "wb", ...) : 
  (converted from warning) URL 'https://mran.microsoft.com/snapshot/2019-08-06/src/contrib/digest_0.6.20.tar.gz': status was 'Couldn't connect to server'
Error: Failed to install 'containerit' from GitHub:
  (converted from warning) download of package ‘digest’ failed

Can you advise? If I've hit two different errors trying to install, others might too.

vsoch commented 4 years ago

okay this seems to work!

install.packages('digest', repos='http://cran.us.r-project.org')

Let me see if I can create a Dockerfile that could be provided to the user to bypass these issues. Even if the user winds up eventually installing on their host, we want to make their first test / introduction to the software as painless as possible.

vsoch commented 4 years ago

Wrong button, lol.

vsoch commented 4 years ago

okay! So this is a bit of a catch-22 - I was able to create a Dockerfile that would allow me to install containerit (sorry it took me so long, literally was try and fail all day!)

FROM rocker/rstudio

# docker build -t containerit .
# docker run --rm -p 8787:8787 -e PASSWORD=meatballs containerit
# open to localhost:8787, username rstudio and password set above

RUN apt-get update && apt-get install -y zlib1g-dev libxml2-dev && \
    Rscript -e "install.packages('digest', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('debugme', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('BiocManager', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('remotes')" && \
    Rscript -e "install.packages('formatR', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('stringr', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('purrr', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('R6', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('devtools', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('plogr', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('utf8', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('semver', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('automagic', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('shiny', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('rjson', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('futile.logger', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('miniUI', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('shinyFiles', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('stevedore', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('versions', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('usethis', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('git2r', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('roxygen2', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('lambda.r', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('futile.options', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('htmltools', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('httpuv', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('xtable', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('sourcetools', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('later', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('promises', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('xml2', repos='http://cran.us.r-project.org')" && \
    Rscript -e "install.packages('devtools')" && \
    Rscript -e "remotes::install_github('o2r-project/containerit')"

And then I was able to start the container (see the header) and generate a recipe, as per the instructions in the readme (well done here!) And the generated recipe is the following:

FROM rocker/r-ver:3.6.1
LABEL maintainer="rstudio"
RUN export DEBIAN_FRONTEND=noninteractive; apt-get -y update \
  && apt-get install -y libcurl4-openssl-dev \
    libssl-dev \
    make
RUN ["install2.r", "assertthat", "backports", "crayon", "curl", "desc", "digest", "formatR", "fs", "futile.logger", "futile.options", "htmltools", "httpuv", "jsonlite", "lambda.r", "later", "magrittr", "mime", "miniUI", "pillar", "pkgconfig", "promises", "R6", "Rcpp", "rlang", "rprojroot", "rstudioapi", "semver", "shiny", "shinyFiles", "stevedore", "stringi", "stringr", "tibble", "versions", "xtable"]
WORKDIR /payload/
CMD ["R"]

I was really happy because the above is much cleaner / nicer looking than my Dockerfile, and also very funny because I used containerit to fix a dependency issue with containerit! What do you think about adding this Dockerfile to the repository? I'll open a PR for it soon.

vsoch commented 4 years ago

See https://github.com/o2r-project/containerit/pull/157 - the recipe is working and provides an easy way to quickly get started! I'd recommend an automated build from the repository so the user doesn't have to wait for it to build (it takes a bit of time) but instead can just pull from Docker Hub.

cole-brokamp commented 4 years ago

Just a note that I hit this error - my version of R is old enough so it would be expected to use bioconductor and not BiocManager:
Error: Failed to install 'containerit' from GitHub:
  (converted from warning) package ‘BiocManager’ is not available (for R version 3.4.4)

This package requires R 3.5.0 or greater. This is probably why you're seeing the install error for its dependency BiocManager not being available?

https://github.com/o2r-project/containerit/blob/de6749f943a0a20bffadb285470afb205759202c/DESCRIPTION#L47-L48

vsoch commented 4 years ago

I hit that exact error @cole-brokamp ! That's why I created the Docker container that would give you a quick start - it's in the pull request here --> https://github.com/o2r-project/containerit/pull/157

It's important that a user (like yourself) is able to try it out quickly - if there is high barrier to that, it can hugely hurt a piece of software from being able to take off.

I'm not sure about the status of the tests, because I didn't change anything but (for the other PR too) they seem to be failing. @nuest must be busy because he hasn't had a chance to comment on either, so I paused my review until he's back.

nuest commented 4 years ago

@vsoch Thanks for the first round of feedback. I'll get to your suggestion of a user Dockerfile in the PR.

I've started a PR to track the changes of the first round of reviews: https://github.com/o2r-project/containerit/pull/160

added a "Prerequisites" section to README
added comment with installation command for remotes
a code of conduct is in CONDUCT.md, also mentioned in the readme
clarified in the README how the existing Docker images can be used to run RStudio: https://github.com/nuest/containerit/commit/1b73a6cf939e5ebfb2bc2d74198288d4108c0094

will add a CONTRIBUTING.md about the contribution process (probably a few words about fork and pull, tag some issues as good "first issues")

vsoch commented 4 years ago

Awesome! Your changes address all of my concerns, minus the last one with the CONTRIBUTING.md. Ping me here when you've added and we can close the issue.

nuest commented 4 years ago

Contribution documentation added @vsoch : https://github.com/nuest/containerit/commit/e16adaf037a5d95e2dd2a8dcde59173fd75b193c

vsoch commented 4 years ago

Great work! My questions and issues have been addressed here, closing issue.

o2r-project / containerit