Get R from RStudio provided apt packages (.deb files) #1161
Conversation
yuvipanda
requested review from
minrk,
manics,
consideRatio and
betatim
and removed request for
minrk
|
This seems to break RStudio < 4.0 :( |
|
Nah, RStudio < 4.0 was already broken lol. Fixed by #1148. Had to tell rstudio exactly where R is by setting R_HOME: 0fed94c Also, it rstudio will fail to start if the uid is < 1000. And on my mac, my uid was 502! So I had to test repo2docker with a |
Might be simpler than getting it from R project managed apt packages
Other wise our semver package cries
It fetches dependencies and installs them too, unlike dpkg
apt also seems to fetch dependencies when needed
We want users to be able to install new versions of R as soon as they are out, without us having to do anything special in repo2docker. So we can't actually check for 'invalid' R versions. This is ok, as we don't have to special case apt repos or similar when installing R from rstudio's debs
2020 had an older version of testthat that ran into issues > # Error in get(genname, envir = envir) : object 'testthat_print' not found
This allows us to dynamically change the base image without having to manually change the version string here
As Dockerfile executes with /bin/sh, not /bin/bash
|
ok, this is ready for review! It's the last thing left to do before we can make the base image configurable. |
| # it is - just $PATH isn't enough. I discovered these are the env vars it | ||
| # looks for by digging through RStudio source and finding https://github.com/rstudio/rstudio/blob/main/src/cpp/r/session/RDiscovery.cpp | ||
| ("R_HOME", f"/opt/R/{self.r_version}/lib/R"), | ||
| ("R_DOC_DIR", "${R_HOME}/doc"), |
There was a problem hiding this comment.
Is ${R_HOME} expanded to f"/opt/R/{self.r_version}/lib/R" at runtime? Or is it actually completely separate?
If it's the former I think it'd be clearer to use the expanded variable here, if it's the latter would you mind adding a comment explaining the difference?
There was a problem hiding this comment.
@manics it gets expanded at runtime. I think it's actually clearer to let it expand at runtime, as you can see that these are dependent if you run env while you are using the image.
| @@ -7,20 +7,6 @@ | |||
| from repo2docker import buildpacks | |||
|
|
|||
|
|
|||
| def test_unsupported_version(tmpdir): | |||
There was a problem hiding this comment.
Does the removal of this test mean all versions are supported?
There was a problem hiding this comment.
YESSS! basically anything that has a deb produced by RStudio is now supported
Let's remove unnecessary changes
Co-authored-by: Simon Li <orpheus+devel@gmail.com>
|
Thanks a lot for the review, @manics! I think I've addressed your comments now |
|
@consideRatio ty, I've accounted for both your suggestions i think. |
|
How risky do you think this change is to users? Are you happy for it to go into a tag/release in the near future, or do you think it needs extended testing on mybinder.org first? |
yuvipanda
changed the title
|
@consideRatio done! @manics in general i always like testing in repo2docker for a week or two before a release :D |
|
@consideRatio @manics anything else i can do to help get this merged? :D |
|
I saw no unresolved comments and this has been open for a long time so i figure a merge is in order! Thanks Yuvi and good that you pinged!! |
|
Yay thanks a lot for the merge, @consideRatio! |
downloaded on a per-version basis rather than an entire apt repository with
all the quirks that entails.
making it much easier for us to switch to a different base Ubuntu image (see
Let
FROM <base_image>in the Dockerfile template be configurable #909)stops providing them in the future, they can be built.
R from conda), as these are appropriate deb builds for particular Ubuntu versions that
also come from rstudio.
be available as soon as RStudio hosts them! No more changes required in repo2docker
when a new R version is available. This means this PR also adds support for R 4.2.
This is awesome!