Skip to content

Commit

Permalink
Auto merge of #12284 - epage:lock, r=weihanglo
Browse files Browse the repository at this point in the history
fix(embedded): Don't pollute script dir with lockfile

### What does this PR try to resolve?
This puts the lockfile back into a target directory in the users home,
like before #12268.

Another idea that came up was to move the workspace root to be in the
target directory (which would effectively be like pre-#12268) but I
think that is a bit hacky / misleading.

This does mean that the lockfile is buried away from the user and they
can't pass it along with their script.  In most cases I've dealt with,
this would be fine.  When the lockfile is needed, they will also most
likely have a workspace, so it shoud have a local lockfile in that case.
The inbetween case is something that needs further evaluation for
whether we should handle it and how.

### How should we test and review this PR?

Its a bit difficult to test for the lockfile in the new location, so this is mostly being tested in that the lockfile no longer exists next to the script.
  • Loading branch information
bors committed Jun 19, 2023
2 parents b81b251 + bb1f6f2 commit 768339b
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 12 deletions.
3 changes: 2 additions & 1 deletion src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1599,7 +1599,8 @@ impl MaybePackage {
}
}

fn is_embedded(&self) -> bool {
/// Has an embedded manifest (single-file package)
pub fn is_embedded(&self) -> bool {
match self {
MaybePackage::Package(p) => p.manifest().is_embedded(),
MaybePackage::Virtual(_) => false,
Expand Down
35 changes: 24 additions & 11 deletions src/cargo/ops/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ use crate::util::Filesystem;
use anyhow::Context as _;

pub fn load_pkg_lockfile(ws: &Workspace<'_>) -> CargoResult<Option<Resolve>> {
if !ws.root().join("Cargo.lock").exists() {
let lock_root = lock_root(ws);
if !lock_root.as_path_unlocked().join("Cargo.lock").exists() {
return Ok(None);
}

let root = Filesystem::new(ws.root().to_path_buf());
let mut f = root.open_ro("Cargo.lock", ws.config(), "Cargo.lock file")?;
let mut f = lock_root.open_ro("Cargo.lock", ws.config(), "Cargo.lock file")?;

let mut s = String::new();
f.read_to_string(&mut s)
Expand All @@ -30,12 +30,12 @@ pub fn load_pkg_lockfile(ws: &Workspace<'_>) -> CargoResult<Option<Resolve>> {

/// Generate a toml String of Cargo.lock from a Resolve.
pub fn resolve_to_string(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoResult<String> {
let (_orig, out, _ws_root) = resolve_to_string_orig(ws, resolve);
let (_orig, out, _lock_root) = resolve_to_string_orig(ws, resolve);
Ok(out)
}

pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoResult<()> {
let (orig, mut out, ws_root) = resolve_to_string_orig(ws, resolve);
let (orig, mut out, lock_root) = resolve_to_string_orig(ws, resolve);

// If the lock file contents haven't changed so don't rewrite it. This is
// helpful on read-only filesystems.
Expand All @@ -55,7 +55,7 @@ pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoRes
"the lock file {} needs to be updated but {} was passed to prevent this\n\
If you want to try to generate the lock file without accessing the network, \
remove the {} flag and use --offline instead.",
ws.root().to_path_buf().join("Cargo.lock").display(),
lock_root.as_path_unlocked().join("Cargo.lock").display(),
flag,
flag
);
Expand All @@ -80,14 +80,19 @@ pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoRes
}

// Ok, if that didn't work just write it out
ws_root
lock_root
.open_rw("Cargo.lock", ws.config(), "Cargo.lock file")
.and_then(|mut f| {
f.file().set_len(0)?;
f.write_all(out.as_bytes())?;
Ok(())
})
.with_context(|| format!("failed to write {}", ws.root().join("Cargo.lock").display()))?;
.with_context(|| {
format!(
"failed to write {}",
lock_root.as_path_unlocked().join("Cargo.lock").display()
)
})?;
Ok(())
}

Expand All @@ -96,15 +101,15 @@ fn resolve_to_string_orig(
resolve: &mut Resolve,
) -> (Option<String>, String, Filesystem) {
// Load the original lock file if it exists.
let ws_root = Filesystem::new(ws.root().to_path_buf());
let orig = ws_root.open_ro("Cargo.lock", ws.config(), "Cargo.lock file");
let lock_root = lock_root(ws);
let orig = lock_root.open_ro("Cargo.lock", ws.config(), "Cargo.lock file");
let orig = orig.and_then(|mut f| {
let mut s = String::new();
f.read_to_string(&mut s)?;
Ok(s)
});
let out = serialize_resolve(resolve, orig.as_deref().ok());
(orig.ok(), out, ws_root)
(orig.ok(), out, lock_root)
}

fn serialize_resolve(resolve: &Resolve, orig: Option<&str>) -> String {
Expand Down Expand Up @@ -235,3 +240,11 @@ fn emit_package(dep: &toml::Table, out: &mut String) {
out.push_str(&format!("replace = {}\n\n", &dep["replace"]));
}
}

fn lock_root(ws: &Workspace<'_>) -> Filesystem {
if ws.root_maybe().is_embedded() {
ws.target_dir()
} else {
Filesystem::new(ws.root().to_owned())
}
}
30 changes: 30 additions & 0 deletions tests/testsuite/script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -591,3 +591,33 @@ args: []
)
.run();
}

#[cargo_test]
fn no_local_lockfile() {
let script = ECHO_SCRIPT;
let p = cargo_test_support::project()
.file("script.rs", script)
.build();
let local_lockfile_path = p.root().join("Cargo.lock");

assert!(!local_lockfile_path.exists());

p.cargo("-Zscript script.rs")
.masquerade_as_nightly_cargo(&["script"])
.with_stdout(
r#"bin: [ROOT]/home/.cargo/target/[..]/debug/script[EXE]
args: []
"#,
)
.with_stderr(
"\
[WARNING] `package.edition` is unspecifiead, defaulting to `2021`
[COMPILING] script v0.0.0 ([ROOT]/foo)
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s
[RUNNING] `[ROOT]/home/.cargo/target/[..]/debug/script[EXE]`
",
)
.run();

assert!(!local_lockfile_path.exists());
}

0 comments on commit 768339b

Please sign in to comment.