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
21 changes: 21 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 @@ -36,6 +36,10 @@ toml = "0.1"
url = "0.2"
winapi = "0.1"

[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 = { git = "https://github.com/carllerche/hamcrest-rust.git" }
Expand Down
10 changes: 9 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, human,
ChainError};

#[derive(RustcDecodable)]
struct Flags {
Expand Down Expand Up @@ -91,6 +92,13 @@ 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!(CargoLock::lock_kind(config)));

try!(fl.lock().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));

init_git_transports(config);
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 threadpool;
extern crate time;
extern crate toml;
extern crate url;
extern crate file_lock;

use std::env;
use std::error::Error;
Expand Down
92 changes: 92 additions & 0 deletions src/cargo/util/lock.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
use file_lock::filename::{Mode, ParseError};
use file_lock::filename::Lock as FileLock;
use file_lock::filename::Error as FileLockError;

use std::path::PathBuf;
use std::fs;
use std::thread::sleep_ms;

use util::{Config, CargoError, CargoResult, human, caused_human};

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

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

impl From<ParseError> for Box<CargoError> {
fn from(t: ParseError) -> Self {
Box::new(t)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add these to src/cargo/util/errors.rs? That's currently where these conversions and trait implementations are housed.


impl CargoError for FileLockError {
fn is_human(&self) -> bool { true }
}
impl CargoError for ParseError {}

pub struct CargoLock {
kind: LockKind,
inner: FileLock,
}

impl CargoLock {

pub fn lock_kind(config: &Config) -> CargoResult<LockKind> {
// TODO(ST): rename config key to something more suitable
const CONFIG_KEY: &'static str = "build.lock-kind";
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 it's fine to avoid this for now and just pick a reasonable default behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean cargo should just always abort, or always wait, without an option to configure it ?

Copy link
Member

Choose a reason for hiding this comment

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

For now yeah, and I'd probably choose "always wait" over "always abort"

match try!(config.get_string(CONFIG_KEY)).map(|t| t.0) {
None => Ok(LockKind::NonBlocking),
Some(kind_string) => match kind_string.parse() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little uneasy putting this parsing of configuration into an external crate, it seems like it's easy to make a change upstream which looks like it's not a breaking change but turns out to actually be a breaking change.

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'm a little uneasy putting this parsing of configuration into an external crate [...]

Do you mean external module ? Should I put the configuration handling into the Config type ?

Copy link
Member

Choose a reason for hiding this comment

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

In this case the FromStr implementation is located in the file_lock crate, and I would expect the actual parsing of a value to happen in this crate. If we're just gonna go with "always wait" though this isn't so bad.

Ok(kind) => Ok(kind),
Err(_) => Err(human(format!("Failed to parse value '{}' at configuration key \
'{}'. Must be one of '{}' and '{}'",
kind_string, CONFIG_KEY,
LockKind::NonBlocking.as_ref(),
LockKind::Blocking.as_ref())))
}
}
}

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

pub fn new_shared(path: PathBuf, kind: LockKind) -> CargoLock {
CargoLock {
kind: kind,
inner: FileLock::new(path, Mode::Read)
}
}

pub fn lock(&mut self) -> CargoResult<()> {
// NOTE(ST): This could fail if cargo is run concurrently for the first time
Copy link
Member

Choose a reason for hiding this comment

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

This "NOTE(ST)" and "TODO(ST)" isn't currently present throughout the rest of Cargo, can you remove these annotations?

// The only way to prevent it would be to take a lock in a directory which exists.
// This is why we don't try! here, but hope the directory exists when we
// try to create the lock file
{
let lock_dir = self.inner.path().parent().unwrap();
if let Err(_) = fs::create_dir_all(lock_dir) {
// We might compete to create one or more directories here
// Give the competing process some time to finish. Then we will
// retry, hoping it the creation works (maybe just because the )
// directory is available already.
// TODO(ST): magic numbers, especially in sleep, will fail at some point ... .
sleep_ms(100);
if let Err(io_err) = fs::create_dir_all(lock_dir) {
// Fail permanently if it still didn't work ...
return Err(caused_human(format!("Failed to create parent directory of \
lock-file at '{}'",
lock_dir.display()), io_err));
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be replaced with try! + chain_error

}
}
debug!("About to acquire file lock: '{}'", self.inner.path().display());
Ok(try!(self.inner.any_lock(self.kind.clone())))
}
}
2 changes: 2 additions & 0 deletions src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub use self::to_url::ToUrl;
pub use self::to_semver::ToSemver;
pub use self::vcs::{GitRepo, HgRepo};
pub use self::sha256::Sha256;
pub use self::lock::{CargoLock, LockKind};

pub mod config;
pub mod errors;
Expand All @@ -32,3 +33,4 @@ mod dependency_queue;
mod sha256;
mod shell_escape;
mod vcs;
mod lock;
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 mut 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