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.

Sign up for GitHub

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

Jump to bottom

Add attribute to repos, for renv users #502

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Add attribute to repos, for renv users #502

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
564a674
Add attribute to `repos`, for renv users
juliasilge Sep 3, 2024
b6a56a9
Fix doubled up `options()`
juliasilge Sep 4, 2024
b965820
Only attribute if we detect `@CRAN@`, use "IDE" for attribute
juliasilge Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions crates/ark/src/modules/positron/options.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,19 @@ options(device = function() {
# Set cran mirror
repos <- getOption("repos")
rstudio_cran <- "https://cran.rstudio.com/"

if (is.null(repos) || !is.character(repos)) {
options(repos = c(CRAN = rstudio_cran))
repos <- c(CRAN = rstudio_cran)
} else {
if ("CRAN" %in% names(repos)) {
if (identical(repos[["CRAN"]], "@CRAN@")) {
repos[["CRAN"]] <- rstudio_cran
options(repos = repos)
}
} else {
repos <- c(CRAN = rstudio_cran, repos)
options(repos = repos)
}
}
attr(repos, "Positron") <- TRUE
Copy link
Contributor

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

https://github.com/rstudio/rstudio/blob/078e21116b0e34aff92addf961699017adb62fc5/src/cpp/r/R/Tools.R#L682-L712

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.

Copy link
Contributor Author

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 👍

juliasilge marked this conversation as resolved.
Show resolved Hide resolved
options(repos = repos)

# Show Plumber apps in the viewer
Copy link
Contributor

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

Copy link
Contributor

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()

Copy link
Contributor

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

Copy link
Contributor Author

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.

options(plumber.docs.callback = function(url) {
Expand Down
Loading