Skip to content

Commit 102044a

Browse files
nbafrankclaude
andcommitted
B-Nilson batch: #75, #81, #76, #77, uvr-r #8 / #9
Six small fixes accumulated on main toward the next batched release. No tag yet — pending brutal-code-reviewer pass + #74 verification. #75 — `uvr init` false-positive "No .r-version pin" notice Regression on the v0.3.1 fallback notice. The notice fired on every fresh init when R was uvr-managed, nagging the user to "pin a project R" even though the resolved binary was already a sane default. Now only fires when find_r_binary returns a non-uvr-managed R (system R on PATH) — the case actually worth nagging about. #81 — `uvr run script.R` no longer dumps R session intro banner Pass `--quiet` to R when invoked with a script path. Interactive `uvr run` (no script) still shows the banner since that's part of the REPL experience. #76 — `uvr add --no-lock` / `--no-install` Two new flags for programmatic uvr.toml construction. `--no-lock` short-circuits before resolution (manifest update only). `--no-install` keeps the resolution but skips the install. Useful for batched scripted `uvr add` calls followed by one explicit `uvr sync`. #77 — `uvr import --name <NAME>` Override the project name on import. In merge mode, replaces the existing manifest's name; in fresh-import mode, replaces the cwd basename default. uvr-r #8 — `uvr::add("nbafrank/uvr-r")` resolves the actual package name Was deriving the package name from the URL basename ("uvr-r"), which diverges from the actual `Package:` field in DESCRIPTION ("uvr") for R companion-package-style repos. Now fetches the remote DESCRIPTION via raw.githubusercontent.com at the requested ref and uses the Package field. Falls back to URL basename + warn on network failure. uvr-r #9 — `..` no longer appears in `uvr r list --all` output The directory-listing scraper for Windows /base/old/ was picking up `href="../"` (parent dir link) as a fake "version" because the prior digits-and-dots sanity check passed for the literal string `..`. New `is_real_r_version` requires at least one digit per component and 2-4 components; covers ".." and similar listing artefacts. Tests added. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent d408de5 commit 102044a

7 files changed

Lines changed: 226 additions & 22 deletions

File tree

crates/uvr-core/src/r_version/downloader.rs

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,22 @@ async fn version_not_found_error(
340340
))
341341
}
342342

343+
/// True when `s` looks like a real R version string (`X.Y` or `X.Y.Z`
344+
/// with all-digit components). Rejects directory-listing artefacts like
345+
/// `..` (parent-dir link) and `.` that pass a naive digits-and-dots
346+
/// check. Used by both the directory-listing scraper in `fetch_available_versions`
347+
/// (uvr-r #9) and `scan_versions_from_listing` to keep the version
348+
/// surface clean.
349+
fn is_real_r_version(s: &str) -> bool {
350+
let parts: Vec<&str> = s.split('.').collect();
351+
if parts.len() < 2 || parts.len() > 4 {
352+
return false;
353+
}
354+
parts
355+
.iter()
356+
.all(|p| !p.is_empty() && p.chars().all(|c| c.is_ascii_digit()))
357+
}
358+
343359
/// Pull `R-X.Y.Z` version strings out of an HTML directory listing.
344360
fn scan_versions_from_listing(body: &str) -> Vec<String> {
345361
use regex::Regex;
@@ -1087,8 +1103,7 @@ pub async fn fetch_available_versions(
10871103
.filter_map(|chunk| {
10881104
let end = chunk.find(suffix)?;
10891105
let ver = &chunk[..end];
1090-
// Sanity check: must be X.Y or X.Y.Z (digits and dots only).
1091-
if ver.chars().all(|c| c.is_ascii_digit() || c == '.') && ver.contains('.') {
1106+
if is_real_r_version(ver) {
10921107
Some(ver.to_string())
10931108
} else {
10941109
None
@@ -1106,12 +1121,18 @@ pub async fn fetch_available_versions(
11061121
.and_then(|r| r.error_for_status())
11071122
{
11081123
if let Ok(text) = old_html.text().await {
1109-
// Old directory lists subdirectories like href="4.4.2/"
1124+
// Old directory lists subdirectories like href="4.4.2/".
1125+
// It also lists `href="../"` (parent dir) — that one
1126+
// satisfies the prior digits-and-dots sanity check (`..`
1127+
// is two dots, no digits) and was getting picked up as a
1128+
// bogus "version", surfacing as a `..` row in `uvr r
1129+
// list --all` and `uvr::r_list(all = TRUE)` (uvr-r #9).
1130+
// `is_real_r_version` requires at least one digit per
1131+
// component.
11101132
for chunk in text.split("href=\"").skip(1) {
11111133
if let Some(end) = chunk.find('/') {
11121134
let ver = &chunk[..end];
1113-
if ver.chars().all(|c| c.is_ascii_digit() || c == '.') && ver.contains('.')
1114-
{
1135+
if is_real_r_version(ver) {
11151136
versions.push(ver.to_string());
11161137
}
11171138
}
@@ -1177,6 +1198,31 @@ mod tests {
11771198
);
11781199
}
11791200

1201+
#[test]
1202+
fn is_real_r_version_accepts_versions() {
1203+
assert!(is_real_r_version("4.5.3"));
1204+
assert!(is_real_r_version("4.6"));
1205+
assert!(is_real_r_version("3.6.3"));
1206+
assert!(is_real_r_version("4.5.3.0")); // 4 components, rare but valid
1207+
}
1208+
1209+
#[test]
1210+
fn is_real_r_version_rejects_directory_listing_noise() {
1211+
// Reproduces uvr-r #9 — the Windows `/base/old/` directory listing
1212+
// includes `href="../"` (parent dir). Without this guard the `..`
1213+
// string passed the all-digits-and-dots check and ended up in the
1214+
// version list, surfacing as a `..` row in `uvr r list --all` and
1215+
// `uvr::r_list(all = TRUE)`.
1216+
assert!(!is_real_r_version(".."));
1217+
assert!(!is_real_r_version("."));
1218+
assert!(!is_real_r_version(""));
1219+
assert!(!is_real_r_version("4."));
1220+
assert!(!is_real_r_version(".4.5"));
1221+
assert!(!is_real_r_version("4..5"));
1222+
assert!(!is_real_r_version("v4.5.3"));
1223+
assert!(!is_real_r_version("4.5.3-rc"));
1224+
}
1225+
11801226
#[test]
11811227
fn macos_arm64_dir_is_expected() {
11821228
let d = macos_arm64_dir();

crates/uvr/src/cli.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,17 @@ pub struct AddArgs {
141141
/// (override via `UVR_INSTALL_TIMEOUT`).
142142
#[arg(long, value_name = "DURATION")]
143143
pub timeout: Option<String>,
144+
145+
/// Update uvr.toml only — skip lockfile resolution and install.
146+
/// Useful when scripting multiple `uvr add` calls and running
147+
/// `uvr lock` + `uvr sync` once at the end (#76).
148+
#[arg(long)]
149+
pub no_lock: bool,
150+
151+
/// Update uvr.toml and lockfile, but don't install. Run `uvr sync`
152+
/// later to install. Implies `--no-lock` is unset (#76).
153+
#[arg(long)]
154+
pub no_install: bool,
144155
}
145156

146157
// ────────────────────────────────────────────────────────────
@@ -277,6 +288,12 @@ pub struct ImportArgs {
277288
#[arg(long, short = 'i', value_name = "FILE", conflicts_with = "path")]
278289
pub input: Option<String>,
279290

291+
/// Override the project name written to `uvr.toml`. Defaults to the
292+
/// current directory's basename (or, in merge mode, preserves the
293+
/// existing project name unless `--name` is given) (#77).
294+
#[arg(long, value_name = "NAME")]
295+
pub name: Option<String>,
296+
280297
/// Resolve and install packages after import
281298
#[arg(long)]
282299
pub lock: bool,

crates/uvr/src/commands/add.rs

Lines changed: 109 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,16 @@ fn parse_add_spec(raw: &str, bioc: bool) -> Result<(String, DependencySpec)> {
7575
Ok((name, spec))
7676
}
7777

78+
#[allow(clippy::too_many_arguments)]
7879
pub async fn run(
7980
packages: Vec<String>,
8081
dev: bool,
8182
bioc: bool,
8283
source: Option<String>,
8384
jobs: usize,
8485
timeout: Option<std::time::Duration>,
86+
no_lock: bool,
87+
no_install: bool,
8588
) -> Result<()> {
8689
let mut project = Project::find_cwd().context("Not inside a uvr project")?;
8790

@@ -117,11 +120,26 @@ pub async fn run(
117120
}
118121
}
119122

120-
let parsed: Vec<(String, DependencySpec)> = packages
123+
let mut parsed: Vec<(String, DependencySpec)> = packages
121124
.iter()
122125
.map(|p| parse_add_spec(p, bioc))
123126
.collect::<Result<Vec<_>>>()?;
124127

128+
// For GitHub specs (`user/repo@ref`), the URL-derived basename is only a
129+
// provisional package name. R's actual package name lives in the
130+
// remote's DESCRIPTION's `Package:` field — and for some packages
131+
// those don't match (the `nbafrank/uvr-r` repo ships package `uvr`,
132+
// see uvr-r #8). Fetch the DESCRIPTION up-front so the manifest entry
133+
// is keyed by the real package name and matches what the resolver
134+
// will produce in the lockfile.
135+
if let Err(e) = resolve_github_pkg_names(&mut parsed).await {
136+
// Don't bail — fall through with the URL-derived names if the
137+
// network is unreachable. The user can edit uvr.toml manually.
138+
ui::warn(format!(
139+
"Could not resolve GitHub package names from DESCRIPTION ({e}). Using repo basenames; you may need to edit uvr.toml if the package name differs."
140+
));
141+
}
142+
125143
// Reject base/recommended packages that ship with R — they can't be installed from CRAN.
126144
for (name, _) in &parsed {
127145
if is_base_package(name) {
@@ -160,7 +178,19 @@ pub async fn run(
160178
.save_manifest()
161179
.context("Failed to write uvr.toml")?;
162180

163-
// Re-resolve → update lockfile → install new packages
181+
// #76 — `--no-lock` short-circuits before resolution; useful for
182+
// building uvr.toml programmatically (e.g. from a script generating
183+
// multiple `uvr add` calls in a row, then a single explicit
184+
// `uvr lock` + `uvr sync` at the end). `--no-install` keeps the
185+
// resolution but skips the install — same use case at a coarser
186+
// grain. `--no-lock` implies `--no-install` since there's no
187+
// lockfile to install from.
188+
if no_lock {
189+
ui::bullet_dim("Skipped lock + install (--no-lock).");
190+
return Ok(());
191+
}
192+
193+
// Re-resolve → update lockfile (and roll back manifest on failure).
164194
let resolve_result = crate::commands::lock::resolve_and_lock(&project, false).await;
165195
if let Err(e) = resolve_result {
166196
// Roll back the manifest to its original state
@@ -172,13 +202,90 @@ pub async fn run(
172202
}
173203
let lockfile = resolve_result.unwrap();
174204

205+
if no_install {
206+
ui::bullet_dim("Skipped install (--no-install). Run `uvr sync` to install.");
207+
return Ok(());
208+
}
209+
175210
crate::commands::sync::install_from_lockfile(&project, &lockfile, jobs, None, timeout)
176211
.await
177212
.context("Failed to install packages after add")?;
178213

179214
Ok(())
180215
}
181216

217+
/// For each GitHub-sourced dep in `parsed`, fetch the remote DESCRIPTION
218+
/// and replace the URL-derived name with the actual `Package:` field
219+
/// (uvr-r #8). Mutates in place. Best-effort — if any individual fetch
220+
/// fails, that entry keeps its URL-derived name and we warn at the call
221+
/// site.
222+
async fn resolve_github_pkg_names(parsed: &mut [(String, DependencySpec)]) -> Result<()> {
223+
use uvr_core::registry::github::parse_github_spec;
224+
225+
let needs_resolve: Vec<usize> = parsed
226+
.iter()
227+
.enumerate()
228+
.filter_map(|(i, (_, spec))| match spec {
229+
DependencySpec::Detailed(d) if d.git.is_some() => Some(i),
230+
_ => None,
231+
})
232+
.collect();
233+
if needs_resolve.is_empty() {
234+
return Ok(());
235+
}
236+
237+
let client = crate::commands::util::build_client()?;
238+
for idx in needs_resolve {
239+
let (provisional_name, spec) = &parsed[idx];
240+
let DependencySpec::Detailed(d) = spec else {
241+
continue;
242+
};
243+
let Some(git) = d.git.as_deref() else {
244+
continue;
245+
};
246+
let git_ref = d.rev.as_deref().unwrap_or("HEAD").to_string();
247+
let spec_str = format!("{git}@{git_ref}");
248+
let Some((user, repo, resolved_ref)) = parse_github_spec(&spec_str) else {
249+
continue;
250+
};
251+
// Cheap path: hit raw.githubusercontent.com directly for the
252+
// DESCRIPTION at the requested ref. Avoids the commit-resolution
253+
// round-trip that the full resolver does — we only need the
254+
// Package: field.
255+
let url =
256+
format!("https://raw.githubusercontent.com/{user}/{repo}/{resolved_ref}/DESCRIPTION");
257+
match client
258+
.get(&url)
259+
.header("User-Agent", concat!("uvr/", env!("CARGO_PKG_VERSION")))
260+
.send()
261+
.await
262+
.and_then(|r| r.error_for_status())
263+
{
264+
Ok(resp) => {
265+
let text = resp.text().await.unwrap_or_default();
266+
let fields = uvr_core::dcf::parse_dcf_fields(&text);
267+
if let Some(actual) = fields.get("Package") {
268+
let actual = actual.trim().to_string();
269+
if !actual.is_empty() && actual != *provisional_name {
270+
ui::bullet_dim(format!(
271+
"{} → {} (Package: field in DESCRIPTION)",
272+
palette::dim(provisional_name),
273+
palette::pkg(&actual)
274+
));
275+
parsed[idx].0 = actual;
276+
}
277+
}
278+
}
279+
Err(e) => {
280+
tracing::warn!(
281+
"DESCRIPTION fetch failed for {git}@{resolved_ref}: {e}; using {provisional_name} as the package name"
282+
);
283+
}
284+
}
285+
}
286+
Ok(())
287+
}
288+
182289
fn format_spec(spec: &DependencySpec) -> String {
183290
match spec {
184291
DependencySpec::Version(v) => v.clone(),

crates/uvr/src/commands/import.rs

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,12 @@ use uvr_core::project::{Project, DOT_UVR_DIR, LIBRARY_DIR, MANIFEST_FILE};
1010
use crate::ui;
1111
use crate::ui::palette;
1212

13-
pub async fn run(path: Option<String>, lock: bool, jobs: usize) -> Result<()> {
13+
pub async fn run(
14+
path: Option<String>,
15+
name: Option<String>,
16+
lock: bool,
17+
jobs: usize,
18+
) -> Result<()> {
1419
// Find the renv.lock file
1520
let renv_path = path.unwrap_or_else(|| "renv.lock".to_string());
1621
let renv_path = Path::new(&renv_path);
@@ -42,14 +47,23 @@ pub async fn run(path: Option<String>, lock: bool, jobs: usize) -> Result<()> {
4247
let manifest_path = cwd.join(MANIFEST_FILE);
4348
let existing =
4449
std::fs::read_to_string(&manifest_path).context("Failed to read existing uvr.toml")?;
45-
existing
50+
let mut m = existing
4651
.parse::<Manifest>()
47-
.context("Failed to parse existing uvr.toml")?
52+
.context("Failed to parse existing uvr.toml")?;
53+
// #77 — `--name` overrides the existing manifest's project name
54+
// when merging. Without this, `uvr import --name foo` against a
55+
// pre-existing uvr.toml silently kept the old name.
56+
if let Some(n) = &name {
57+
m.project.name = n.clone();
58+
}
59+
m
4860
} else {
49-
let project_name = cwd
50-
.file_name()
51-
.map(|n| n.to_string_lossy().to_string())
52-
.unwrap_or_else(|| "imported-project".to_string());
61+
// Project name: explicit `--name` (#77) wins over the cwd basename.
62+
let project_name = name.clone().unwrap_or_else(|| {
63+
cwd.file_name()
64+
.map(|n| n.to_string_lossy().to_string())
65+
.unwrap_or_else(|| "imported-project".to_string())
66+
});
5367
Manifest::new(&project_name, r_version.clone())
5468
};
5569

crates/uvr/src/commands/init.rs

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -340,14 +340,23 @@ fn resolve_project_r_binary(dir: &Path) -> Option<std::path::PathBuf> {
340340
}
341341

342342
// No pin (or pin not yet installed) — fall through to whatever uvr
343-
// would find on the system. Surface the fallback so users notice the
344-
// IDE config is bound to whatever happened to be on PATH rather than
345-
// a project-pinned R.
343+
// would find on the system. Surface the fallback ONLY when it
344+
// resolves to a non-uvr-managed R (e.g. system R on PATH) — that's
345+
// the case worth nagging about, since system R can be removed or
346+
// upgraded out from under the IDE config. When the fallback is a
347+
// uvr-managed install, the binding is sound and the prior nag was
348+
// a false-positive that fired on every fresh `uvr init` (#75).
346349
let fallback = find_r_binary(None).ok()?;
347-
ui::bullet_dim(format!(
348-
"No .r-version pin — IDE config bound to {}. Re-run `uvr init --here` after pinning a project R version.",
349-
fallback.display()
350-
));
350+
let managed_root = dirs::home_dir().map(|h| h.join(".uvr").join("r-versions"));
351+
let is_managed = managed_root
352+
.as_ref()
353+
.is_some_and(|root| fallback.starts_with(root));
354+
if !is_managed {
355+
ui::bullet_dim(format!(
356+
"No .r-version pin — IDE config bound to system R at {}. Run `uvr r install <ver>` then `uvr r pin <ver>` for project-pinned R.",
357+
fallback.display()
358+
));
359+
}
351360
Some(fallback)
352361
}
353362

crates/uvr/src/commands/run.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,12 @@ pub async fn run(
9292
cmd.arg("--no-environ");
9393

9494
if let Some(script_path) = &script {
95+
// Script mode — suppress the R session intro banner. Without
96+
// --quiet, every `uvr run script.R` dumps the "R version 4.6.0
97+
// (2026-…) -- 'Because it was There'" preamble to stdout before
98+
// the script starts (#81). --quiet keeps prompts visible (so
99+
// interactive `browser()` still works) while dropping the banner.
100+
cmd.arg("--quiet");
95101
cmd.arg("--no-save");
96102
cmd.arg("--no-restore");
97103
cmd.arg(format!("--file={script_path}"));
@@ -100,6 +106,9 @@ pub async fn run(
100106
cmd.args(&args);
101107
}
102108
} else {
109+
// Interactive mode — keep the banner. It's part of the REPL
110+
// experience and a user typing `uvr run` (no script) expects
111+
// R's normal startup output.
103112
cmd.arg("--no-save");
104113
}
105114

crates/uvr/src/main.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,8 @@ async fn run() -> Result<()> {
9292
args.source,
9393
args.jobs,
9494
timeout,
95+
args.no_lock,
96+
args.no_install,
9597
)
9698
.await?;
9799
}
@@ -121,7 +123,7 @@ async fn run() -> Result<()> {
121123
// #71: --input/-i is an alternative spelling of the positional path.
122124
// clap's `conflicts_with` already rejects passing both.
123125
let path = args.input.or(args.path);
124-
commands::import::run(path, args.lock, args.jobs).await?;
126+
commands::import::run(path, args.name, args.lock, args.jobs).await?;
125127
}
126128
Commands::Completions(args) => {
127129
commands::completions::run(args.shell)?;

0 commit comments

Comments
 (0)