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

Remove the unused pandoc and TeX installations from the RSPM image #300

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atheriel
Copy link
Collaborator

Previously the image was based on rstudio/r-base, which includes TinyTeX and pandoc. However RSPM's Git builds do not build package manuals or vignettes, so we don't actually need these programs.

This shaves about 300MB off the image size.

The upstream rstudio/r-base image also does the following, which we replicate directly instead:

  • Install R from RStudio's own packages.

  • Set TZ=UTC.

  • Set the locale.

Previously the image was based on rstudio/r-base, which includes TinyTeX
and pandoc. However RSPM's Git builds do not build package manuals or
vignettes, so we don't actually need these programs.

This shaves about 300MB off the image size.

The upstream rstudio/r-base image also does the following, which we
replicate directly instead:

* Install R from RStudio's own packages.

* Set TZ=UTC.

* Set the locale.

Signed-off-by: Aaron Jacobs <[email protected]>
@atheriel atheriel requested a review from glin April 29, 2022 18:52
Copy link
Contributor

@glin glin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, this is a great idea. Should we add a NEWS item as well?

Comment on lines +16 to +18
&& ln -s /opt/R/${R_VERSION}/bin/R /usr/bin/R \
&& ln -s /opt/R/${R_VERSION}/bin/Rscript /usr/bin/Rscript \
&& ln -s /opt/R/${R_VERSION}/lib/R /usr/lib/R
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this now, I think it was a mistake that we symlinked R to the OS default /usr/bin instead of /usr/local/bin in the rstudio/r-base images. The symlink to /usr/lib also isn't necessary at all, I copy pasted that from somewhere else 😬. I don't know if any users would ever want to switch their R versions here, but using /usr/local/bin would be nice for consistency with the instructions at https://docs.rstudio.com/resources/install-r/#create-a-symlink-to-r

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could remove it since RSPM doesn't need R to be on the PATH. Unless users might be running R scripts in this image.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt that users would be running R scripts in this image 😮 At least I hope that would not be the case, I guess it is possible with some type of CI / etc. but definitely not a happy path recommendation 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not the happy path. I'll remove these symlinks.

Copy link
Contributor

@colearendt colearendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great call, along with @glin 's recommendations. In particular, I think a NEWS entry would definitely be worthwhile

@atheriel
Copy link
Collaborator Author

atheriel commented May 3, 2022

I'm going to wait for #270 to be merged before creating more churn with this one.

@colearendt colearendt added the package-manager Specific to Package Manager images label Aug 15, 2023
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package-manager Specific to Package Manager images
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants