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.

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

RFC: Globally Locked Cargo #1781

Closed
wants to merge 11 commits into from
29 changes: 29 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ toml = "0.1"
url = "0.2"
winapi = "0.2"

[dependencies.file-lock]
git = "https://github.com/Byron/file-lock"
branch = "cargo-adjustments"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merging I'd prefer to depend on a crates.io version of this library

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely - my preferred path of action is to get the cargo-related integration right, then port file-lock to windows, and see if cargo indeed works and builds on all supported platforms. Probably you can ask bors to do that, as travis can't test windows for instance.

Once it does work everywhere, a new crates.io release of the original crate should be made (I just have a fork).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll see if I can't set up appveyor CI for cargo, that should give us some Windows coverage for PRs at least.


[dev-dependencies]
tempdir = "0.3"
hamcrest = "0.1"
Expand Down
9 changes: 8 additions & 1 deletion src/bin/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ use std::process::Command;

use cargo::{execute_main_without_stdin, handle_error, shell};
use cargo::core::MultiShell;
use cargo::util::{CliError, CliResult, lev_distance, Config};
use cargo::util::{CliError, CliResult, lev_distance, Config, CargoLock, LockKind, human,
ChainError};

#[derive(RustcDecodable)]
struct Flags {
Expand Down Expand Up @@ -95,6 +96,12 @@ macro_rules! each_subcommand{ ($mac:ident) => ({
on this top-level information.
*/
fn execute(flags: Flags, config: &Config) -> CliResult<Option<()>> {
let mut fl = CargoLock::new(config.home().join(".global-lock"));

try!(fl.lock(LockKind::Blocking).chain_error(|| {
human("Failed to obtain global cargo lock")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we'll want to improve this error message slightly, for example how can this come up? If it comes up because another Cargo is running, then it should probably say that, but if it just failed because of an I/O error we may also want to say that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we'll want to improve this error message slightly, for example how can this come up?

Good idea. Also to check for IOErrors specifically. These shouldn't happen though, and will be provided as cause as FileLock errors are declared human right now.

}));

try!(config.shell().set_verbosity(flags.flag_verbose, flags.flag_quiet));
try!(config.shell().set_color_config(flags.flag_color.as_ref().map(|s| &s[..])));

Expand Down
1 change: 1 addition & 0 deletions src/cargo/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ extern crate term;
extern crate time;
extern crate toml;
extern crate url;
extern crate file_lock;

use std::env;
use std::error::Error;
Expand Down
8 changes: 8 additions & 0 deletions src/cargo/util/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use curl;
use git2;
use toml;
use url;
use file_lock;

pub type CargoResult<T> = Result<T, Box<CargoError>>;

Expand Down Expand Up @@ -307,12 +308,18 @@ from_error! {
url::ParseError,
toml::DecodeError,
ffi::NulError,
file_lock::filename::Error,
file_lock::filename::ParseError,
}

impl<E: CargoError> From<Human<E>> for Box<CargoError> {
fn from(t: Human<E>) -> Box<CargoError> { Box::new(t) }
}

impl CargoError for file_lock::filename::Error {
fn is_human(&self) -> bool { true }
}

impl CargoError for semver::ReqParseError {}
impl CargoError for io::Error {}
impl CargoError for git2::Error {}
Expand All @@ -325,6 +332,7 @@ impl CargoError for toml::Error {}
impl CargoError for toml::DecodeError {}
impl CargoError for url::ParseError {}
impl CargoError for ffi::NulError {}
impl CargoError for file_lock::filename::ParseError {}

// =============================================================================
// Construction helpers
Expand Down
33 changes: 33 additions & 0 deletions src/cargo/util/lock.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
use file_lock::filename::Mode;
use file_lock::filename::Lock as FileLock;

use std::path::PathBuf;
use std::fs;

use util::{CargoResult, ChainError, human};

pub use file_lock::filename::Kind as LockKind;

pub struct CargoLock {
inner: FileLock,
}

impl CargoLock {
pub fn new(path: PathBuf) -> CargoLock {
CargoLock {
inner: FileLock::new(path, Mode::Write)
}
}

pub fn lock(&mut self, kind: LockKind) -> CargoResult<()> {
{
let lock_dir = self.inner.path().parent().unwrap();
try!(fs::create_dir_all(lock_dir).chain_error(|| {
human(format!("Failed to create parent directory of lock-file at '{}'",
lock_dir.display()))
}));
}
debug!("About to acquire file lock: '{}'", self.inner.path().display());
Ok(try!(self.inner.any_lock(kind)))
}
}
2 changes: 2 additions & 0 deletions src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub use self::sha256::Sha256;
pub use self::to_semver::ToSemver;
pub use self::to_url::ToUrl;
pub use self::vcs::{GitRepo, HgRepo};
pub use self::lock::{CargoLock, LockKind};

pub mod config;
pub mod errors;
Expand All @@ -33,4 +34,5 @@ mod dependency_queue;
mod sha256;
mod shell_escape;
mod vcs;
mod lock;
mod rustc;
26 changes: 26 additions & 0 deletions tests/test_cargo_build_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,32 @@ test!(build_with_no_lib {
.with_stderr("no library targets found"));
});

test!(build_global_lock {

let p = project("foo")
.file("Cargo.toml", r#"
[package]

name = "foo"
version = "0.0.1"
authors = ["[email protected]"]
"#)
.file("src/main.rs", r#"
fn main() {}
"#)
.file("src/lib.rs", r#" "#);

p.build();

let bg_process = p.cargo("build");
let fg_process = bg_process.clone();
let mut bg_process_handle = bg_process.build_command().spawn().unwrap();

assert_eq!(fg_process.build_command().output().unwrap().status.code().unwrap(), 101);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test looks like it runs a risk of being flaky because of various bits of timing here and there, and I think it'd also be useful to match the stdout/stderr here as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is also why it doesn't work most of the time. I am not sure how to make it work without having to make the two processes communicate (i.e. master waits for child to come up).

Maybe I could launch the cargo implementation from within the test-case directly (without a sub-process), and then spawn the test-executable. That would give me more control, and is similar to the working tests in the file-lock crate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah, that sounds like a good idea!

assert!(bg_process_handle.wait().unwrap().success());
});


test!(build_with_relative_cargo_home_path {
let p = project("foo")
.file("Cargo.toml", r#"
Expand Down