-
Notifications
You must be signed in to change notification settings - Fork 53
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 additional options to installBioc.r #103
Add additional options to installBioc.r #103
Conversation
Adds additional options to the installBioc.r example script based on the approach used in install2.r. The main goal was to be able to use the installBioc.r script as the main method to install bioconductor packages in Dockerfiles, and to make it fail the build process whenever an error occurs during package installation. The script now throws errors when a package fails to install due to missing dependencies, misspelled names, etc. Already installed packages can be skipped. Suggested dependencies can be toggled. Parallel install can be enabled. Download method can be set. A few options were added for BiocManager::install()'s unique arguments. A toggle to update other packages that are outdated. Additional repositories can be prepended to the standard BioC repository (only works for bioc sub-repositories like software/experiment). Already installed packages can be forced to redownload and install. In addition, the library path is now set in the same manner as is done in install2.r (by reading the first entry of .libPaths() and passing it to the install function, rather than setting it to `/usr/local/lib/R/site-library` via `.libPaths()`).
Thanks for taking the time work on improvements, and for writing such a detail issue and PR. I will try to take a look. As you can image I also had my issues with BioC installations but as I am not primarily a BioC user I tended to get by. I am not sure I myself have a full use case for testing here (i.e. 'fail and reinstall' logic which looks promising) but maybe that does not need to stand in the way of a merge as other can test. Lastly, as it is related and reflects some of my more recent work: have you seen r2u ? Full and complete binaries with proper dependencies. I aim for CRAN, have full coverage and because of the overlap also 200+ BioC packages. Might be possible to grow the BioC side. You cannot beat |
Thanks for having a look and for sharing your r2u project. It sounds interesting so I'll definitely explore it in more detail in the future. As a side note: I'll be unavailable for a few days, so my future responses will be delayed a bit ;) |
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 looks great, micro-nit comments notwithstanding.
I take it you tested it?
Can you maybe add a block with a one-liner in this file to ChangeLog? You will see the format, there are two spaces between the three fields date + name + email.
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 took care of the little nits. Thanks again for the solid contribution.
And I took this as an opportunity to roll up a new release for CRAN which I just sent there, and which should hopefully get processed without any issues. |
Thanks for the kind words and taking care of the small modifications yourself! Regarding testing: yes, but mostly ad-hoc. I've been using an earlier draft of this script for a while now without issues. For this PR I added a few of the missing options to make it more on-par with |
It's all good. And @eitsupi noticed r-devel wants us to change an error message pattern in the grep expression which he updated in the |
This PR adds additional options to the installBioc.r example script based on the approach used in install2.r.
The main goal was to be able to use the installBioc.r script as the main method to install bioconductor packages in Dockerfiles, because the standard method that is recommended by Bioconductor (
RUN R -e 'BiocManager::install("scAlign")'
, https://www.bioconductor.org/help/docker/) does not throw an error that causes the docker build process to stop.This can result in seemingly successfully built docker images that lack some of the packages which the creator expected to be installed. Since building docker images with r packages is so error prone (due to the need for system libraries that often just requires trial and error), this new script can make the task a lot easier. Builds will fail clearly, rather than having to scroll through the wall of text install log output or trying to manually load each package when running a container.
The script now throws errors when a package fails to install due to missing dependencies, misspelled names, etc. Already installed packages can be skipped. Suggested dependencies can be toggled. Parallel install can be enabled. Download method can be set.
A few options were added for BiocManager::install()'s unique arguments. A toggle to update other packages that are
outdated. Additional repositories can be prepended to the standard BioC repository (only works for bioc sub-repositories
like software/experiment, see https://rdrr.io/cran/BiocManager/man/install.html). Already installed packages can be forced to redownload and install.
In addition, the library path is now set in the same manner as is done in install2.r (by reading the first entry of .libPaths() and passing it to the install function, rather than setting it to
/usr/local/lib/R/site-library
via.libPaths()
).This addresses some of the issues I faced myself (Bioconductor/bioconductor_docker#38, Bioconductor/BiocManager#124, #93, rocker-org/rocker-versioned2#292) as well as others (rocker-org/rocker-versioned2#370).
I believe my modifications to the script could still benefit from a critical review. Especially the errors that the script now attempts to catch is somewhat guesswork. Based on my own testing, these messages are the most common ones, but perhaps I've missed some others.