Repository navigation
Conversation
779e904 to
18815b5
Compare
`r_env::vars()` blanks R_ENVIRON_USER for `uvr run` and `uvr activate`,
and `uvr run` adds --no-environ on top. The stated reason is narrow: a
~/.Renviron that sets R_LIBS_USER would override the project library.
Blanking the whole file to defend one variable also drops API tokens,
GITHUB_PAT, proxy settings, TZ and locale -- which is what the reporter
hit. R_ENVIRON_USER names the single file to use, so a blank value skips
./.Renviron and ~/.Renviron both, and --no-environ means re-exporting it
does not help either. Meanwhile the install path deliberately keeps the
file (r_cmd_install.rs: "~/.Renviron often holds load-bearing TZ /
locale settings"), so `R CMD INSTALL` saw the user's tokens and
`uvr run` did not.
UVR_USER_ENVIRON=1 drops the R_ENVIRON_USER blank and, for consistency,
--no-environ -- the flag suppresses every environment file, so leaving it
on would undo the opt-out for `uvr run` while activation honoured it.
Scope kept deliberately narrow:
- Off by default. No existing behaviour changes.
- The *site* file (R_ENVIRON) stays blanked either way. It is machine
configuration, not the user's, and is not what nbafrank#260 asks for.
- Truthy parsing matches source_installs_forced / cache_lookup_disabled
exactly (1|true|yes|TRUE|YES).
Verified end-to-end against real R: with a ~/.Renviron setting
MY_API_TOKEN, `uvr run` reports it empty by default and reports the
value under UVR_USER_ENVIRON=1.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7PUPQNRsCDnJydr7FiUdQ
18815b5 to
1e953dd
Compare
gdevenyi
left a comment
There was a problem hiding this comment.
Self-review after the rebase onto main (04eb792). Local checks at this head: cargo fmt --all -- --check and cargo clippy --all-targets --all-features -- -D warnings are clean; cargo test --all passes except test_sync_url_dependency_verifies_checksum_then_installs, which also fails on main itself on a host with R (the test expects run `uvr lock` , the message from b728dc0 says run `uvr lock --upgrade` ).
Rebase note. user_environ() now uses the new truthy() helper from main.
I read REnv::vars, the uvr run command construction, the three activation emitters and the installer comment that the PR refers to. The switch does what the text says for a new shell: R_ENVIRON_USER is not exported, R_ENVIRON stays empty, and --no-environ is dropped. A new activation calls deactivate first, so it also removes an old blank value. Two points remain. uvr run does not remove a blank R_ENVIRON_USER that it inherits from an activated shell, so the switch has no effect there. And in the modes where no project .Rprofile runs (a headered script, a bare or unattended project), a ~/.Renviron with R_LIBS_USER removes the uvr library fully, which makes the script fail and is more than the README says.
| // `UVR_USER_ENVIRON=1`, which exists to let `~/.Renviron` through (#260) | ||
| // — the flag suppresses every environment file, so leaving it on would | ||
| // undo the opt-out for `uvr run` while activation honoured it. | ||
| if !uvr_core::env_vars::user_environ() { |
There was a problem hiding this comment.
The switch has no effect when uvr run starts from a shell that was activated without it.
.uvr/activate exports R_ENVIRON_USER='' (from REnv::vars() in the default mode). With UVR_USER_ENVIRON=1, vars() no longer contains R_ENVIRON_USER, so the loop above does not set it, and nothing removes it. The child R inherits the blank value. A blank R_ENVIRON_USER tells R to read no user file.
Scenario:
source .uvr/activate # exports R_ENVIRON_USER=''
export UVR_USER_ENVIRON=1 # the README instruction
uvr run token.R # Sys.getenv("MY_API_TOKEN") is still ""The user must know to source the shim again. A user who reads the README section in an activated shell gets this result first.
Suggested fix: when the switch is on, remove a blank inherited value, and keep a value that the user set to a path:
if uvr_core::env_vars::user_environ() {
if std::env::var_os("R_ENVIRON_USER").is_some_and(|v| v.is_empty()) {
cmd.env_remove("R_ENVIRON_USER");
}
} else {
cmd.arg("--no-environ");
}| ("R_ENVIRON", String::new()), | ||
| ("R_ENVIRON_USER", String::new()), | ||
| ]; | ||
| if !crate::env_vars::user_environ() { |
There was a problem hiding this comment.
With the switch on, the only thing that keeps the uvr library on .libPaths() is the project .Rprofile block. Three modes have no such block, and there the library is lost fully, not only put second.
R applies ~/.Renviron after the process environment, so R_LIBS_USER=~/R/library in that file replaces the value that uvr set. Many ~/.Renviron files hold a token and R_LIBS_USER together.
- Headered script:
run.rspointsR_PROFILE_USERat the null device, and the library is the ephemeral--withenvironment. Input:UVR_USER_ENVIRON=1 uvr run script.R, wherescript.Rhas an inline dependency header. Result:library(<header dependency>)fails with "there is no package called", or loads a different version from~/R/library. uvr init --bareand--unattendedprojects ([enhancement] opt-in IDE integration and CI/headless controls #257): no.Rprofileis written, souvr runuses only the user's library.
The README says that R_LIBS_USER "does override the project library". It does not say that a headered script stops working.
Suggested fix: when the switch is on, also put the uvr library list in R_LIBS, which is blank today. R builds the search path from R_LIBS first, then R_LIBS_USER. A ~/.Renviron that sets R_LIBS_USER then adds the user's library after the uvr library and cannot remove it:
let user_environ = crate::env_vars::user_environ();
("R_LIBS", if user_environ { self.r_libs_user() } else { String::new() }),If you do not want that, add the script and bare-project cases to the README text.
Addresses #260 — deliberately not "Fixes", because the design call is yours. See below.
The problem
r_env::vars()blanksR_ENVIRON_USERfor bothuvr runanduvr activate, anduvr runpasses--no-environon top. The reason given in the code is narrow: a~/.Renvironthat setsR_LIBS_USERwould silently override the project library.Blanking the whole file to defend one variable takes everything else with it.
R_ENVIRON_USERnames the single file R will read, so a blank value skips./.Renvironand~/.Renvironboth, and--no-environmeans re-exporting the variable does not help. The reporter lost API tokens; the same blanking also dropsGITHUB_PAT, proxy settings, TZ and locale.The asymmetry is the tell:
installer/r_cmd_install.rsdeliberately keeps the file, with the comment "~/.Renviron often holds load-bearing TZ / locale settings." SoR CMD INSTALLhas always seen the user's tokens anduvr runhas not.What this does
UVR_USER_ENVIRON=1drops theR_ENVIRON_USERblank, and drops--no-environto match — the flag suppresses every environment file, so leaving it on would undo the opt-out foruvr runwhile activation honoured it.Scope kept narrow on purpose:
R_ENVIRONis machine configuration rather than the user's, and it is not what setting api tokens #260 asks for.1|true|yes|TRUE|YES, as insource_installs_forcedandcache_lookup_disabled). No new spelling.The trade, stated plainly
With the opt-out on, a
~/.Renvironthat setsR_LIBS_USERdoes override the project library. That is the exact risk the original blanking existed to prevent, and turning the switch on accepts it. The README says so where it documents the variable.Why this option and not the other one
In my comment on #260 I sketched two routes: stop blanking and re-assert the library after startup, or keep blanking and add an opt-out. I built the opt-out because it is the smaller change and it cannot regress anyone who does not set it.
The other route is arguably better long-term — R reads Renviron before Rprofile, so the project
.Rprofile's.libPaths()call already wins — but it has a real gap:uvr runin script mode pointsR_PROFILE_USERat the null device, so that path would need the library re-asserted some other way. Happy to rework this into that shape if you prefer it. It is your call, and this PR does not foreclose it.Worth settling alongside #261 and #249, which are the same question about
~/.Rprofile. Right now three sites give three different answers to "does the user's startup configuration survive", and picking one policy is less work than three separate fixes.Verification
End-to-end against real R, with a
~/.RenvironsettingMY_API_TOKEN:Unit tests cover both directions, and assert that the opt-out does not export
R_ENVIRON_USERat all — a blank value suppresses the file just as effectively as a wrong one — whileR_ENVIRONstays blank. Both take the existingenv_lock()so they cannot race the other env-mutating tests under the parallel runner.cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings, and the full suite are clean: 177 uvr, 63 CLI, 517 uvr-core (+2).Merge note
Adds a bullet to
## UnreleasedinCHANGELOG.md, same spot as the other open PRs. Keep both bullets. No code overlap.