From fbd43d923ca33755f44f0f2ca82d7cf4f9360a67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Hern=C3=A1ndez?= Date: Thu, 8 Sep 2016 15:13:31 -0400 Subject: [PATCH 1/4] Add a test that reproduces the error of parsing home config twice. --- tests/cargotest/support/mod.rs | 5 +++++ tests/rustflags.rs | 28 +++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/tests/cargotest/support/mod.rs b/tests/cargotest/support/mod.rs index c99e8e294a0..fb6c7daf629 100644 --- a/tests/cargotest/support/mod.rs +++ b/tests/cargotest/support/mod.rs @@ -198,6 +198,11 @@ pub fn project(name: &str) -> ProjectBuilder { ProjectBuilder::new(name, paths::root().join(name)) } +// Generates a project layout inside our fake home dir +pub fn project_in_home(name: &str) -> ProjectBuilder { + ProjectBuilder::new(name, paths::home().join(name)) +} + // === Helpers === pub fn main_file(println: &str, deps: &[&str]) -> String { diff --git a/tests/rustflags.rs b/tests/rustflags.rs index 264c47219ef..ccdc6a91dc9 100644 --- a/tests/rustflags.rs +++ b/tests/rustflags.rs @@ -5,7 +5,7 @@ use std::io::Write; use std::fs::{self, File}; use cargotest::rustc_host; -use cargotest::support::{project, execs, paths}; +use cargotest::support::{project, project_in_home, execs, paths}; use hamcrest::assert_that; #[test] @@ -852,3 +852,29 @@ fn build_rustflags_no_recompile() { assert_that(p.cargo("build").env("RUSTFLAGS", "--cfg foo"), execs().with_stdout("").with_status(0)); } + +#[test] +fn build_rustflags_with_home_config() { + // We need a config file inside the home directory + let home = paths::home(); + let home_config = home.join(".cargo"); + fs::create_dir(&home_config).unwrap(); + File::create(&home_config.join("config")).unwrap().write_all(br#" + [build] + rustflags = ["-Cllvm-args=-x86-asm-syntax=intel"] + "#).unwrap(); + + // And we need the project to be inside the home directory + // so the walking process finds the home project twice. + let p = project_in_home("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.0.1" + "#) + .file("src/lib.rs", ""); + p.build(); + + assert_that(p.cargo("build").arg("-v"), + execs().with_status(0)); +} From 478f591f926af1b57729b7fe4eb43bc8c7226ecc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Hern=C3=A1ndez?= Date: Thu, 8 Sep 2016 15:58:53 -0400 Subject: [PATCH 2/4] Don't parse the config in home if it was parsed already. This is the first version of the fix. It needs clean up. --- src/cargo/util/config.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 21d68c1cef4..0d0a89791dc 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -1,6 +1,7 @@ use std::cell::{RefCell, RefMut, Cell}; use std::collections::hash_map::Entry::{Occupied, Vacant}; -use std::collections::hash_map::{HashMap}; +use std::collections::hash_map::HashMap; +use std::collections::HashSet; use std::env; use std::fmt; use std::fs::{self, File}; @@ -675,13 +676,20 @@ fn walk_tree(pwd: &Path, mut walk: F) -> CargoResult<()> where F: FnMut(File, &Path) -> CargoResult<()> { let mut current = pwd; + let mut stash: HashSet = HashSet::new(); loop { let possible = current.join(".cargo").join("config"); if fs::metadata(&possible).is_ok() { - let file = try!(File::open(&possible)); + let canonical = fs::canonicalize(possible).unwrap(); + let string = canonical.to_str().unwrap().to_owned(); + if stash.get(&string).is_none() { + let file = try!(File::open(&canonical)); - try!(walk(file, &possible)); + try!(walk(file, &canonical)); + + stash.insert(string); + } } match current.parent() { Some(p) => current = p, @@ -696,8 +704,9 @@ fn walk_tree(pwd: &Path, mut walk: F) -> CargoResult<()> human("Cargo couldn't find your home directory. \ This probably means that $HOME was not set.") })); - if !pwd.starts_with(&home) { - let config = home.join("config"); + let config = home.join("config"); + let key = config.to_str().unwrap().to_owned(); + if stash.get(&key).is_none() { if fs::metadata(&config).is_ok() { let file = try!(File::open(&config)); try!(walk(file, &config)); From ec172126eadf558df253c09547cea870512302d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Hern=C3=A1ndez?= Date: Thu, 8 Sep 2016 16:20:36 -0400 Subject: [PATCH 3/4] Add ConfigFile struct to handle config file paths. --- src/cargo/util/config.rs | 54 +++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 0d0a89791dc..70dadbd988d 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -22,6 +22,30 @@ use util::toml as cargo_toml; use self::ConfigValue as CV; +#[derive(PartialEq, Eq, Hash)] +struct ConfigFile { + path: Option, +} + +impl ConfigFile { + pub fn new(path: PathBuf) -> ConfigFile { + let canonical = match fs::canonicalize(path) { + Ok(p) => Some(p), + Err(_) => None, + }; + + ConfigFile { path: canonical } + } + + pub fn exist(&self) -> bool { + self.path.is_some() + } + + pub fn as_path_buf(&self) -> Option { + self.path.clone() + } +} + pub struct Config { home_path: Filesystem, shell: RefCell, @@ -676,21 +700,19 @@ fn walk_tree(pwd: &Path, mut walk: F) -> CargoResult<()> where F: FnMut(File, &Path) -> CargoResult<()> { let mut current = pwd; - let mut stash: HashSet = HashSet::new(); + let mut stash: HashSet = HashSet::new(); loop { - let possible = current.join(".cargo").join("config"); - if fs::metadata(&possible).is_ok() { - let canonical = fs::canonicalize(possible).unwrap(); - let string = canonical.to_str().unwrap().to_owned(); - if stash.get(&string).is_none() { - let file = try!(File::open(&canonical)); + let possible = ConfigFile::new(current.join(".cargo").join("config")); + if possible.exist() && stash.get(&possible).is_none() { + let name = possible.as_path_buf().unwrap(); + let file = try!(File::open(&name)); - try!(walk(file, &canonical)); + try!(walk(file, &name)); - stash.insert(string); - } + stash.insert(possible); } + match current.parent() { Some(p) => current = p, None => break, @@ -704,13 +726,11 @@ fn walk_tree(pwd: &Path, mut walk: F) -> CargoResult<()> human("Cargo couldn't find your home directory. \ This probably means that $HOME was not set.") })); - let config = home.join("config"); - let key = config.to_str().unwrap().to_owned(); - if stash.get(&key).is_none() { - if fs::metadata(&config).is_ok() { - let file = try!(File::open(&config)); - try!(walk(file, &config)); - } + let config = ConfigFile::new(home.join("config")); + if config.exist() && stash.get(&config).is_none() { + let name = config.as_path_buf().unwrap(); + let file = try!(File::open(&name)); + try!(walk(file, &name)); } Ok(()) From 68d78231970b8c1f62c60b05682866c5e8889093 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Hern=C3=A1ndez?= Date: Thu, 8 Sep 2016 20:35:13 -0400 Subject: [PATCH 4/4] Remove fs::canonicalize in walk_tree fix. * Remove ConfiFile struct, it is not needed without the fs::canonicalize call. * Don't check if the file is in the stash when walking the tree, without "canonicalization" it is not necessary. --- src/cargo/util/config.rs | 44 ++++++++-------------------------------- 1 file changed, 9 insertions(+), 35 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 70dadbd988d..2f207d86eab 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -22,30 +22,6 @@ use util::toml as cargo_toml; use self::ConfigValue as CV; -#[derive(PartialEq, Eq, Hash)] -struct ConfigFile { - path: Option, -} - -impl ConfigFile { - pub fn new(path: PathBuf) -> ConfigFile { - let canonical = match fs::canonicalize(path) { - Ok(p) => Some(p), - Err(_) => None, - }; - - ConfigFile { path: canonical } - } - - pub fn exist(&self) -> bool { - self.path.is_some() - } - - pub fn as_path_buf(&self) -> Option { - self.path.clone() - } -} - pub struct Config { home_path: Filesystem, shell: RefCell, @@ -700,15 +676,14 @@ fn walk_tree(pwd: &Path, mut walk: F) -> CargoResult<()> where F: FnMut(File, &Path) -> CargoResult<()> { let mut current = pwd; - let mut stash: HashSet = HashSet::new(); + let mut stash: HashSet = HashSet::new(); loop { - let possible = ConfigFile::new(current.join(".cargo").join("config")); - if possible.exist() && stash.get(&possible).is_none() { - let name = possible.as_path_buf().unwrap(); - let file = try!(File::open(&name)); + let possible = current.join(".cargo").join("config"); + if fs::metadata(&possible).is_ok() { + let file = try!(File::open(&possible)); - try!(walk(file, &name)); + try!(walk(file, &possible)); stash.insert(possible); } @@ -726,11 +701,10 @@ fn walk_tree(pwd: &Path, mut walk: F) -> CargoResult<()> human("Cargo couldn't find your home directory. \ This probably means that $HOME was not set.") })); - let config = ConfigFile::new(home.join("config")); - if config.exist() && stash.get(&config).is_none() { - let name = config.as_path_buf().unwrap(); - let file = try!(File::open(&name)); - try!(walk(file, &name)); + let config = home.join("config"); + if !stash.contains(&config) && fs::metadata(&config).is_ok() { + let file = try!(File::open(&config)); + try!(walk(file, &config)); } Ok(())