-
Notifications
You must be signed in to change notification settings - Fork 15
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 attribute to repos
, for renv users
#502
Conversation
} | ||
} | ||
attr(repos, "Positron") <- TRUE |
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.
We should not unconditionally set this. IIUC, we should only set this if we detect the "default" repos behavior, i.e. if we see "@CRAN@"
as the repos[["CRAN"]]
value
That is what RStudio does
I think that means you only set the attribute in the if (identical(repos[["CRAN"]], "@CRAN@")) {
branch
In theory this allows a user to set their own repos
in their .Rprofile
, and in that case we are not supposed to mark that with a "Positron"
attribute, as that did not come from us. In practice that might not work, because I think this code might get run before we source a user's .Rprofile
on their behalf, but that would be another bug to fix separately, I think.
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.
Ah, I misunderstood before how this was being used! Thanks 👍
} | ||
} | ||
attr(repos, "Positron") <- TRUE | ||
options(repos = repos) | ||
|
||
# Show Plumber apps in the viewer |
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 have one other larger worry about this in general.
None of this is going to kick in if the user is using a version of R installed with Rig. That's because Rig sets repos
for you in the special /library/base/R/Rprofile
that R forcibly loads on every startup (like, rig straight up tweaks this file that you get when you download R itself)
That means that every time you load up a rig installed version of R, it will always have repos
set and it will never be "@CRAN@"
.
https://github.com/r-lib/rig/blob/140115c9b565167670cfc6f303e6c968c563db98/src/windows.rs#L342
https://github.com/r-lib/rig/blob/140115c9b565167670cfc6f303e6c968c563db98/src/macos.rs#L788
https://github.com/r-lib/rig/blob/140115c9b565167670cfc6f303e6c968c563db98/src/linux.rs#L579-L613
That means we would never add the "Positron"
attribute, and RStudio never adds the "RStudio"
attribute.
I can confirm this by loading up my non-rig R-devel version in a plain terminal:
> getOption("repos")
CRAN
"@CRAN@"
vs a rig installed R 4.4
> getOption("repos")
CRAN
"https://cloud.r-project.org"
Now, this may be ok for Positron because on Linux you can see in the above link that rig sets up PPM as the repos
option, so that should mean that users get binaries when using renv + rig on Linux, even though renv isn't the one setting the repos
option.
The bigger issue would be for renv itself, because it means that renv will never detect a "default" case and can't set the repos
option to the user's chosen configuration option for ppm.url()
, which defaults to normal PPM but can be customized by the user:
https://github.com/rstudio/renv/blob/fcfe748225a88bf56b1964868604353c2e73bf7c/R/init.R#L341
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 do not think we should let this stop us from moving on this PR, but @kevinushey may want to take a closer look at this weird rig + renv intersection issue, especially the inability to set a user customized config$ppm.url()
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.
@juliasilge if you do as I suggest above and only conditionally set repos[["CRAN"]]
when you see it is set to "@CRAN@"
, then you aren't ever going to see a "Positron"
attribute in your testing unless you swap to a non-rig version of R
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 am not a rig user 😆 so I did not notice this.
I will add a note for renv when I say this new attribute is available.
Addresses posit-dev/positron#4255
You can check out the approach RStudio takes here:
https://github.com/rstudio/rstudio/blob/078e21116b0e34aff92addf961699017adb62fc5/src/cpp/r/R/Tools.R#L698
I saw your note here @DavisVaughan. Given that we are unlikely to be able to change this attribute in RStudio, I don't tend to think we will gain much by using
IDE
ordefault
. If you have a strong opinion otherwise, happy to change this, though!QA Notes
After this PR is merged and ark is bumped, you will see (edited after code review to change name of attribute):
Created on 2024-09-04 with reprex v2.1.1
Notice the new attribute.
Once we get this into a release build, we should update at rstudio/renv#1963.