diff --git a/crates/socket-patch-cli/tests/e2e_vendor_pypi_build.rs b/crates/socket-patch-cli/tests/e2e_vendor_pypi_build.rs index 44f59333c..09fba0879 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_pypi_build.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_pypi_build.rs @@ -1300,6 +1300,135 @@ fn uv_vendor_revert_after_dev_group_declaration_edit() { ); } +/// #869, the PEP 723 script-lock twin of #840: `uv add --script s.py +/// "six>=1.15"` while six is vendored rewrites the script but leaves +/// `s.py.lock` byte-identical (a path source records no specifier). The +/// revert must write the script's NEW specifier into the lock's +/// `[manifest] requirements`, not the recorded `==1.16.0` that leaves +/// `uv run --locked --script` red while the revert reports success. +#[test] +#[serial_test::serial] +fn uv_vendor_script_revert_after_declaration_edit() { + const TAG: &str = "uv-script-declaration-edit"; + let Some((uv, python)) = capstone_uv(TAG) else { + return; + }; + if !uv_vex::uv_under_test().is_ok_and(|uv| uv.help_has(&["lock"], "--script")) { + println!("UV-VEX lane=script({TAG}) step=all result=n/a (no `uv lock --script`)"); + return; + } + bake_leak_guards(); + let tmp = tempfile::tempdir().unwrap(); + let proj = tmp.path().join("proj"); + std::fs::create_dir_all(&proj).unwrap(); + let cache = tmp.path().join("uv-cache"); + let venv = proj.join(".venv"); + let mut cache_env: Vec<(&str, &str)> = vec![("UV_CACHE_DIR", cache.to_str().unwrap())]; + if let Some(py) = python.as_deref() { + cache_env.push(("UV_PYTHON", py)); + } + let mut venv_env = cache_env.clone(); + venv_env.push(("VIRTUAL_ENV", venv.to_str().unwrap())); + std::fs::write( + proj.join("s.py"), + "# /// script\n# requires-python = \">=3.9\"\n# dependencies = [\n# \"six==1.16.0\",\n# \"attrs>=20\",\n# ]\n# ///\nimport six\n", + ) + .unwrap(); + // uv runs a script in a cache env the crawlers cannot see; the + // developer's in-project venv holds the copy the writers read. + for (step, env) in [ + (&["lock", "-q", "--script", "s.py"][..], &cache_env), + (&["venv", "-q", ".venv"][..], &cache_env), + (&["pip", "install", "-q", "six==1.16.0"][..], &venv_env), + ] { + let out = tool(&uv, &proj, step, env); + if !out.status.success() { + println!( + "SKIP e2e_vendor_pypi_build({TAG}): `uv {}` failed (PyPI unreachable?):\n{}", + step.join(" "), + String::from_utf8_lossy(&out.stderr) + ); + return; + } + } + stage_patch(&proj, &site_packages(&venv).join("six.py")); + + let (code, stdout, stderr) = run_vendored(&VendorDriver::VendorOffline, &proj); + assert_eq!( + code, 0, + "vendor failed.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + assert_vendored_applied(&parse_envelope(&stdout)); + let check = &["lock", "--script", "s.py", "--check"][..]; + assert_tool_ok( + &tool(&uv, &proj, check, &cache_env), + "`uv lock --script --check` on the wired pair", + ); + let wired = std::fs::read_to_string(proj.join("s.py.lock")).unwrap(); + + let out = tool( + &uv, + &proj, + &["add", "-q", "--script", "s.py", "six>=1.15"], + &cache_env, + ); + if !out.status.success() { + println!( + "SKIP e2e_vendor_pypi_build({TAG}): `uv add --script` failed (PyPI unreachable?):\n{}", + String::from_utf8_lossy(&out.stderr) + ); + return; + } + let script = std::fs::read_to_string(proj.join("s.py")).unwrap(); + assert!(script.contains("six>=1.15"), "{script}"); + assert_eq!( + std::fs::read_to_string(proj.join("s.py.lock")).unwrap(), + wired, + "uv leaves the vendored script lock byte-identical" + ); + + let (code, stdout, stderr) = run_socket( + &proj, + &[ + "vendor", + "--revert", + "--json", + "--cwd", + proj.to_str().unwrap(), + ], + ); + assert_eq!( + code, 0, + "revert failed.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + let renv = parse_envelope(&stdout); + assert_eq!(renv["status"], "success", "revert envelope: {renv}"); + assert!( + !stdout.contains("vendor_lock_entry_drifted"), + "a single-clause declaration edit is not drift: {renv}" + ); + for file in ["s.py", "s.py.lock"] { + let text = std::fs::read_to_string(proj.join(file)).unwrap(); + assert!( + !text.contains(".socket/vendor"), + "{file} must be fully unwired:\n{text}" + ); + } + let lock = std::fs::read_to_string(proj.join("s.py.lock")).unwrap(); + assert!( + lock.contains("{ name = \"six\", specifier = \">=1.15\" }"), + "the restored lock carries the script's specifier:\n{lock}" + ); + assert_tool_ok( + &tool(&uv, &proj, check, &cache_env), + "`uv lock --script --check` after the revert", + ); + assert!( + !proj.join(".socket/vendor").exists(), + ".socket/vendor must be fully removed after revert" + ); +} + /// `get --mode vendored` twin of the uv capstone above (v3.6): the /// SAME vendor engine and wiring, driven through get's uuid path — exempt /// from installed narrowing, so only the mocked `view/{uuid}` route is diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/uv.rs b/crates/socket-patch-core/src/patch/redirect/upstream/uv.rs index 15a7f5b03..26f92be61 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/uv.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/uv.rs @@ -1272,7 +1272,9 @@ pub(crate) enum LockRequirementArray<'a> { } /// The `specifier` a vendored revert should restore for `name`'s entry -/// in `array`, given the one it recorded when vendoring (`None` when the +/// in `array` of the lock paired with `pyproject_text` (the metadata +/// document `rel` names: pyproject.toml, or a script's PEP 723 block), +/// given the one it recorded when vendoring (`None` when the /// entry had none) and the entry's `marker`. /// /// A path source records no specifier, so a user who changes the @@ -1292,6 +1294,7 @@ pub(crate) enum LockRequirementArray<'a> { /// mirrors is ambiguous, so restoring any spelling may break `--locked`. pub(crate) fn respell_lock_specifier( pyproject_text: &str, + rel: &str, array: LockRequirementArray<'_>, name: &str, recorded: Option<&str>, @@ -1301,7 +1304,7 @@ pub(crate) fn respell_lock_specifier( return Ok(None); }; let meta = Metadata { - rel: "pyproject.toml".to_string(), + rel: rel.to_string(), text: String::new(), script: false, doc, @@ -1354,8 +1357,8 @@ pub(crate) fn respell_lock_specifier( // longer shows this entry's spelling, so any guess may break // `--locked`. _ => Err(format!( - "pyproject.toml now declares {name} with a multi-clause specifier, whose \ - spelling in uv.lock is not derivable" + "{rel} now declares {name} with a multi-clause specifier, whose spelling in \ + the lock is not derivable" )), } } diff --git a/crates/socket-patch-core/src/vendor/pypi_lock.rs b/crates/socket-patch-core/src/vendor/pypi_lock.rs index 65b7c7d47..f4672f146 100644 --- a/crates/socket-patch-core/src/vendor/pypi_lock.rs +++ b/crates/socket-patch-core/src/vendor/pypi_lock.rs @@ -13,6 +13,8 @@ use crate::utils::python_script::{ refuse_uv_no_sources, replace_script_metadata, rewrite_script_metadata, script_metadata, }; +use crate::patch::redirect::upstream::{respell_lock_specifier, LockRequirementArray}; + use super::common::{record, refuse_symlinked}; use super::parse_memo::ParseMemo; use super::state::{VendorEntry, WiringAction, WiringRecord}; @@ -955,6 +957,162 @@ fn without_package(text: &str, name: &str) -> Result { Ok(doc.to_string()) } +/// The recorded pre-vendor script lock `original` with every `[manifest]` +/// requirement entry for `name` (PEP 503) re-spelled to the specifier the +/// live `script`'s PEP 723 block declares now ([`respell_lock_specifier`], +/// the rule the uv.lock lane uses for pyproject.toml, #840). Entries whose +/// declaration still agrees, and every other entry, come back unchanged, +/// so an untouched script still reverts byte for byte. `Err` when the +/// declaration changed but uv's spelling of it isn't derivable. +fn respell_script_lock( + original: &str, + rel: &str, + script: &str, + vendored_script: Option<&str>, + name: &str, +) -> Result { + // A script whose declarations are the ones it had right after + // vendoring still matches the recorded lock: nothing to re-derive, and + // an ambiguous declaration match below must not turn that into drift. + let declarations = script_declarations(script); + if declarations.is_some() && vendored_script.map(script_declarations) == Some(declarations) { + return Ok(original.to_string()); + } + let (Ok((_, metadata)), Ok(mut doc)) = + (script_metadata(script), original.parse::()) + else { + return Ok(original.to_string()); + }; + let mut changed = false; + if let Some(manifest) = doc.get_mut("manifest").and_then(Item::as_table_like_mut) { + for (key, item) in manifest.iter_mut() { + let key = key.get().to_string(); + if !matches!( + key.as_str(), + "requirements" | "constraints" | "build-constraints" | "overrides" + ) { + continue; + } + let Some(array) = item.as_array_mut() else { + continue; + }; + for value in array.iter_mut() { + let Some(entry) = value.as_inline_table_mut() else { + continue; + }; + if entry + .get("name") + .and_then(Value::as_str) + .is_none_or(|n| canonicalize_pypi_name(n) != name) + { + continue; + } + let field = |k: &str| entry.get(k).and_then(Value::as_str).map(str::to_string); + let (specifier, marker) = (field("specifier"), field("marker")); + let Some(spec) = respell_lock_specifier( + &metadata, + rel, + LockRequirementArray::Manifest(&key), + name, + specifier.as_deref(), + marker.as_deref(), + )? + else { + continue; + }; + match (spec, entry.get_mut("specifier")) { + (Some(spec), Some(value)) => { + let decor = value.decor().clone(); + *value = Value::from(spec); + *value.decor_mut() = decor; + } + (Some(spec), None) => { + // Appended last, where uv writes it, taking over + // the padding before the closing brace. + let mut value = Value::from(spec); + if let Some((_, last)) = entry.iter_mut().last() { + if let Some(suffix) = last.decor().suffix().cloned() { + last.decor_mut().set_suffix(""); + value.decor_mut().set_suffix(suffix); + } + } + entry.insert("specifier", value); + } + (None, _) => { + // The padding before the closing brace moves to + // the entry's new last value. + let was_last = entry.iter().last().is_some_and(|(k, _)| k == "specifier"); + let removed = entry.remove("specifier"); + let suffix = removed.and_then(|v| v.decor().suffix().cloned()); + if let (true, Some(suffix), Some((_, last))) = + (was_last, suffix, entry.iter_mut().last()) + { + last.decor_mut().set_suffix(suffix); + } + } + } + changed = true; + } + } + } + Ok(if changed { + crate::utils::python_lock::preserve_line_endings(original, doc.to_string()) + } else { + original.to_string() + }) +} + +/// The script text vendoring wrote for the script `lock` pairs with (its +/// `python_script_metadata` record's `new`). +fn vendored_script<'e>(entry: &'e VendorEntry, lock: &str) -> Option<&'e str> { + let script = script_of_lock(lock)?; + entry + .wiring + .iter() + .find(|record| record.kind == SCRIPT_KIND && record.file == script)? + .new + .as_ref()? + .as_str() +} + +/// Every requirement a script's PEP 723 block declares that its lock's +/// `[manifest]` mirrors (`dependencies` and the `[tool.uv]` constraint, +/// build-constraint and override lists), keyed by list; `None` when the +/// block doesn't parse. +fn script_declarations(script: &str) -> Option> { + let (_, metadata) = script_metadata(script).ok()?; + let doc = metadata.parse::().ok()?; + let uv = doc + .get("tool") + .and_then(|tool| tool.get("uv")) + .and_then(Item::as_table_like); + let mut out = Vec::new(); + for (key, item) in [ + ("dependencies", doc.get("dependencies")), + ( + "constraint-dependencies", + uv.and_then(|uv| uv.get("constraint-dependencies")), + ), + ( + "build-constraint-dependencies", + uv.and_then(|uv| uv.get("build-constraint-dependencies")), + ), + ( + "override-dependencies", + uv.and_then(|uv| uv.get("override-dependencies")), + ), + ] { + out.extend( + item.and_then(Item::as_array) + .into_iter() + .flatten() + .filter_map(Value::as_str) + .map(|spec| (key, spec.to_string())), + ); + } + Some(out) +} + pub(super) async fn revert_python_locks( entry: &VendorEntry, root: &Path, @@ -1102,6 +1260,42 @@ pub(super) async fn revert_python_locks( } else { (original, new) }; + // #869: a path source records no specifier, so a requirement the + // user changed in the PEP 723 block while vendored left the script + // lock byte-identical: restore the specifier the script declares + // NOW, never the recorded one blindly (the script-lock twin of + // #840). + let respelled; + let original = match ( + record.kind == KIND, + script_texts.get(record.file.as_str()), + package_name.as_deref(), + ) { + (true, Some(script), Some(name)) => match respell_script_lock( + original, + pair_of(&record.file), + script, + vendored_script(entry, &record.file), + name, + ) { + Ok(text) => { + respelled = text; + respelled.as_str() + } + Err(reason) => { + warnings.push(VendorWarning::new( + "vendor_lock_entry_drifted", + format!( + "{} entry for {name} can't be restored to match its script \ + ({reason}); left untouched", + record.file + ), + )); + continue; + } + }, + _ => original, + }; let restored = if record.kind == SCRIPT_KIND { (|| { if live == new || live == original { @@ -2038,7 +2232,7 @@ mod tests { /// Vendor `one==1` into a PEP 723 script and its `.py.lock`; returns the /// ledger entry and the wired script and lock texts. async fn vendor_script_pair(root: &Path) -> (VendorEntry, String, String) { - let lock = "version = 1\nrevision = 3\nrequires-python = \">=3.9\"\n\n[manifest]\nrequirements = [{name = \"one\", specifier = \"==1\"}]\n\n[[package]]\nname = \"one\"\nversion = \"1\"\nsource = {registry = \"https://pypi.org/simple\"}\n"; + let lock = SCRIPT_LOCK; let script = "# /// script\n# requires-python = \">=3.9\"\n# dependencies = [\"one==1\", \"attrs>=20\"]\n# ///\nimport one\n"; write_pylock(root, "job.py.lock", lock).await; tokio::fs::write(root.join("job.py"), script).await.unwrap(); @@ -2201,6 +2395,144 @@ mod tests { ); } + /// #869: a path source records no specifier, so a user who changes the + /// vendored package's requirement in the PEP 723 block while it is + /// vendored (`uv add --script job.py "one>=0.9"`) leaves the script + /// lock byte-identical. The revert must restore the `[manifest] + /// requirements` element with the specifier the script declares NOW, + /// not the one recorded when vendoring, or `uv run --locked --script` + /// fails on the restored lock. + #[tokio::test] + async fn script_revert_respells_a_requirement_changed_while_vendored() { + for (declared, element) in [ + ("one>=0.9", "{name = \"one\", specifier = \">=0.9\"}"), + ("one==1.*", "{name = \"one\", specifier = \"==1.*\"}"), + ("one", "{name = \"one\"}"), + ] { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path(); + let (entry, wired_script, wired_lock) = vendor_script_pair(root).await; + let edited = wired_script.replace("\"one==1\"", &format!("\"{declared}\"")); + assert_ne!(edited, wired_script); + tokio::fs::write(root.join("job.py"), &edited) + .await + .unwrap(); + + let outcome = revert_python_locks(&entry, root, false).await; + assert!(outcome.success, "{declared}: {outcome:?}"); + assert!( + outcome.warnings.is_empty(), + "{declared}: {:?}", + outcome.warnings + ); + let script = std::fs::read_to_string(root.join("job.py")).unwrap(); + let lock = std::fs::read_to_string(root.join("job.py.lock")).unwrap(); + assert_eq!( + script, + format!( + "# /// script\n# requires-python = \">=3.9\"\n# dependencies = [\"{declared}\", \"attrs>=20\"]\n# ///\nimport one\n" + ), + "{declared}" + ); + assert!(!lock.contains(UUID), "{declared}: {lock}"); + assert!( + lock.contains(&format!("requirements = [{element}]")), + "{declared}: {lock}" + ); + assert_ne!(lock, wired_lock); + } + } + + /// #869: when the script now declares a multi-clause range, uv's + /// spelling of it in the lock is not derivable, so the revert keeps + /// both files and their recovery state (`vendor_lock_entry_drifted`) + /// rather than restoring a stale or guessed specifier. + #[tokio::test] + async fn script_revert_keeps_drift_for_an_underivable_new_requirement() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path(); + let (entry, wired_script, wired_lock) = vendor_script_pair(root).await; + let edited = wired_script.replace("\"one==1\"", "\"one>=0.9,<2\""); + tokio::fs::write(root.join("job.py"), &edited) + .await + .unwrap(); + + let outcome = revert_python_locks(&entry, root, false).await; + assert!(outcome.success, "{outcome:?}"); + assert!(outcome.drift_skipped(), "{:?}", outcome.warnings); + assert_eq!( + std::fs::read_to_string(root.join("job.py")).unwrap(), + edited + ); + assert_eq!( + std::fs::read_to_string(root.join("job.py.lock")).unwrap(), + wired_lock + ); + } + + /// #869: a requirement recorded bare (`dependencies = ["six"]`) that the + /// user pinned while vendored gets its specifier back last, where uv + /// writes it; a marker entry is matched by its marker; other packages' + /// entries keep their bytes. + #[test] + fn respell_script_lock_inserts_a_specifier_where_uv_writes_it() { + let original = "version = 1\r\nrevision = 3\r\n\r\n[manifest]\r\nrequirements = [\r\n { name = \"attrs\", specifier = \">=20\" },\r\n { name = \"six\" },\r\n { name = \"six\", marker = \"sys_platform == 'win32'\", specifier = \"==1.16.0\" },\r\n]\r\n"; + let script = "# /// script\n# dependencies = [\"attrs>=20\", \"six>=1.15\", \"six==1.16.*; sys_platform == 'win32'\"]\n# ///\n"; + let respelled = respell_script_lock(original, "s.py", script, None, "six").unwrap(); + assert_eq!( + respelled, + original + .replace( + "{ name = \"six\" }", + "{ name = \"six\", specifier = \">=1.15\" }" + ) + .replace("\"==1.16.0\"", "\"==1.16.*\"") + ); + let bare = "# /// script\n# dependencies = [\"attrs>=20\", \"six\", \"six; sys_platform == 'win32'\"]\n# ///\n"; + assert_eq!( + respell_script_lock(original, "s.py", bare, None, "six").unwrap(), + original.replace(", specifier = \"==1.16.0\" }", " }") + ); + let unchanged = "# /// script\n# dependencies = [\"attrs>=20\", \"six\", \"six==1.16.0; sys_platform == 'win32'\"]\n# ///\n"; + assert_eq!( + respell_script_lock(original, "s.py", unchanged, None, "six").unwrap(), + original + ); + } + + /// A script whose declarations are unchanged since vendoring keeps the + /// recorded lock even when which declaration an entry mirrors is + /// ambiguous (uv reordered an `or` marker): that is no edit, so it + /// must not become drift. + #[test] + fn respell_script_lock_keeps_an_unchanged_ambiguous_script() { + let original = "version = 1\n\n[manifest]\nrequirements = [\n { name = \"six\", marker = \"sys_platform == 'darwin' or sys_platform == 'win32'\", specifier = \"==1.16.0\" },\n { name = \"six\", marker = \"sys_platform == 'linux'\", specifier = \">=1.15\" },\n]\n"; + let script = "# /// script\n# dependencies = [\"six==1.16.0; sys_platform == 'win32' or sys_platform == 'darwin'\", \"six>=1.15; sys_platform == 'linux'\"]\n# ///\n"; + let vendored = format!("{script}# vendored\n"); + assert!(respell_script_lock(original, "s.py", script, None, "six").is_err()); + assert_eq!( + respell_script_lock(original, "s.py", script, Some(&vendored), "six").unwrap(), + original + ); + } + + /// The respell leaves an unchanged declaration alone: the script lock + /// reverts to the recorded original byte for byte. + #[tokio::test] + async fn script_revert_with_an_unchanged_requirement_is_byte_exact() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path(); + let (entry, ..) = vendor_script_pair(root).await; + + let outcome = revert_python_locks(&entry, root, false).await; + assert!(outcome.success, "{outcome:?}"); + assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings); + assert_eq!( + std::fs::read_to_string(root.join("job.py.lock")).unwrap(), + SCRIPT_LOCK + ); + } + const SCRIPT_LOCK: &str = "version = 1\nrevision = 3\nrequires-python = \">=3.9\"\n\n[manifest]\nrequirements = [{name = \"one\", specifier = \"==1\"}]\n\n[[package]]\nname = \"one\"\nversion = \"1\"\nsource = {registry = \"https://pypi.org/simple\"}\n"; const SCRIPT: &str = "# /// script\n# requires-python = \">=3.9\"\n# dependencies = [\"one==1\", \"attrs>=20\"]\n# ///\nimport one\n"; diff --git a/crates/socket-patch-core/src/vendor/pypi_uv.rs b/crates/socket-patch-core/src/vendor/pypi_uv.rs index 96a6afa53..5da3546b8 100644 --- a/crates/socket-patch-core/src/vendor/pypi_uv.rs +++ b/crates/socket-patch-core/src/vendor/pypi_uv.rs @@ -1267,8 +1267,14 @@ fn respell_original( let array = array.unwrap_or(LockRequirementArray::RequiresDist); let specifier = entry_value(element, "specifier"); let marker = entry_value(element, "marker"); - if let Some(spec) = respell_lock_specifier(pyproject_text, array, canon, specifier, marker)? - { + if let Some(spec) = respell_lock_specifier( + pyproject_text, + "pyproject.toml", + array, + canon, + specifier, + marker, + )? { out.replace_range(s..e, &with_specifier(element, spec.as_deref())); } } diff --git a/docs/testing/uv-compatibility.md b/docs/testing/uv-compatibility.md index f944d1b6b..39a38afff 100644 --- a/docs/testing/uv-compatibility.md +++ b/docs/testing/uv-compatibility.md @@ -243,7 +243,9 @@ restores the `requires-dist`, `requires-dev` and `[manifest] constraints` entries with the specifier pyproject.toml declares at revert time, not the one recorded when vendoring. When uv's spelling can't be derived, as with a multi-clause range (uv orders clauses differently across releases), the edit is -treated as drift and both files are kept. Tests also cover restoring one package while +treated as drift and both files are kept. A PEP 723 script lock follows the +same rule: its `[manifest]` entries are restored with the specifier the +script's `# /// script` block declares at revert time. Tests also cover restoring one package while preserving another package's vendored entries. ## Reproduce the release-family matrix