Auto merge of #2912 - matklad:move-target-dir, r=alexcrichton

Move `target_dir` to Workspace and fix #2848

Target dir is now an API of Workspace. It is still initialized eagerly, just later.

I also had to errors in the config file when retrieving `verbose` and `color` config. Seems fishy, but is probably OK.

Author: bors

src/cargo/core/workspace.rs

@@ -6,7 +6,7 @@ use std::slice;
 use core::{Package, VirtualManifest, EitherManifest, SourceId};
 use core::{PackageIdSpec, Dependency};
 use ops;
-use util::{Config, CargoResult};
+use util::{Config, CargoResult, Filesystem};
 use util::paths;
 
 /// The core abstraction in Cargo for working with a workspace of crates.
@@ -32,6 +32,10 @@ pub struct Workspace<'cfg> {
     // `current_manifest` was found on the filesystem with `[workspace]`.
     root_manifest: Option<PathBuf>,
 
+    // Shared target directory for all the packages of this workspace.
+    // `None` if the default path of `root/target` should be used.
+    target_dir: Option<Filesystem>,
+
     // List of members in this workspace with a listing of all their manifest
     // paths. The packages themselves can be looked up through the `packages`
     // set above.
@@ -78,6 +82,8 @@ impl<'cfg> Workspace<'cfg> {
     /// before returning it, so `Ok` is only returned for valid workspaces.
     pub fn new(manifest_path: &Path, config: &'cfg Config)
                -> CargoResult<Workspace<'cfg>> {
+        let target_dir = try!(config.target_dir());
+
         let mut ws = Workspace {
             config: config,
             current_manifest: manifest_path.to_path_buf(),
@@ -86,6 +92,7 @@ impl<'cfg> Workspace<'cfg> {
                 packages: HashMap::new(),
             },
             root_manifest: None,
+            target_dir: target_dir,
             members: Vec::new(),
         };
         ws.root_manifest = try!(ws.find_root(manifest_path));
@@ -103,7 +110,8 @@ impl<'cfg> Workspace<'cfg> {
     ///
     /// This is currently only used in niche situations like `cargo install` or
     /// `cargo package`.
-    pub fn one(package: Package, config: &'cfg Config) -> Workspace<'cfg> {
+    pub fn one(package: Package, config: &'cfg Config, target_dir: Option<Filesystem>)
+               -> CargoResult<Workspace<'cfg>> {
         let mut ws = Workspace {
             config: config,
             current_manifest: package.manifest_path().to_path_buf(),
@@ -112,15 +120,21 @@ impl<'cfg> Workspace<'cfg> {
                 packages: HashMap::new(),
             },
             root_manifest: None,
+            target_dir: None,
             members: Vec::new(),
         };
         {
             let key = ws.current_manifest.parent().unwrap();
             let package = MaybePackage::Package(package);
             ws.packages.packages.insert(key.to_path_buf(), package);
+            ws.target_dir = if let Some(dir) = target_dir {
+                Some(dir)
+            } else {
+                try!(ws.config.target_dir())
+            };
             ws.members.push(ws.current_manifest.clone());
         }
-        return ws
+        return Ok(ws)
     }
 
     /// Returns the current package of this workspace.
@@ -155,6 +169,12 @@ impl<'cfg> Workspace<'cfg> {
         }.parent().unwrap()
     }
 
+    pub fn target_dir(&self) -> Filesystem {
+        self.target_dir.clone().unwrap_or_else(|| {
+            Filesystem::new(self.root().join("target"))
+        })
+    }
+
     /// Returns the root [replace] section of this workspace.
     ///
     /// This may be from a virtual crate or an actual crate.

src/cargo/ops/cargo_clean.rs

@@ -16,7 +16,7 @@ pub struct CleanOptions<'a> {
 
 /// Cleans the project from build artifacts.
 pub fn clean(ws: &Workspace, opts: &CleanOptions) -> CargoResult<()> {
-    let target_dir = opts.config.target_dir(&ws);
+    let target_dir = ws.target_dir();
 
     // If we have a spec, then we need to delete some packages, otherwise, just
     // remove the whole target directory and be done with it!

src/cargo/ops/cargo_doc.rs

@@ -53,7 +53,7 @@ pub fn doc(ws: &Workspace,
         // Don't bother locking here as if this is getting deleted there's
         // nothing we can do about it and otherwise if it's getting overwritten
         // then that's also ok!
-        let target_dir = options.compile_opts.config.target_dir(ws);
+        let target_dir = ws.target_dir();
         let path = target_dir.join("doc").join(&name).join("index.html");
         let path = path.into_path_unlocked();
         if fs::metadata(&path).is_ok() {

src/cargo/ops/cargo_install.rs

@@ -76,7 +76,20 @@ pub fn install(root: Option<&str>,
                                             crates.io, or use --path or --git to \
                                             specify alternate source"))))
     };
-    let ws = Workspace::one(pkg, config);
+
+
+    let mut td_opt = None;
+    let overidden_target_dir = if source_id.is_path() {
+        None
+    } else if let Ok(td) = TempDir::new("cargo-install") {
+        let p = td.path().to_owned();
+        td_opt = Some(td);
+        Some(Filesystem::new(p))
+    } else {
+        Some(Filesystem::new(config.cwd().join("target-install")))
+    };
+
+    let ws = try!(Workspace::one(pkg, config, overidden_target_dir));
     let pkg = try!(ws.current());
 
     // Preflight checks to check up front whether we'll overwrite something.
@@ -89,27 +102,14 @@ pub fn install(root: Option<&str>,
         try!(check_overwrites(&dst, pkg, &opts.filter, &list, force));
     }
 
-    let mut td_opt = None;
-    let target_dir = if source_id.is_path() {
-        config.target_dir(&ws)
-    } else {
-        if let Ok(td) = TempDir::new("cargo-install") {
-            let p = td.path().to_owned();
-            td_opt = Some(td);
-            Filesystem::new(p)
-        } else {
-            Filesystem::new(config.cwd().join("target-install"))
-        }
-    };
-    config.set_target_dir(target_dir.clone());
     let compile = try!(ops::compile_ws(&ws, Some(source), opts).chain_error(|| {
         if let Some(td) = td_opt.take() {
             // preserve the temporary directory, so the user can inspect it
             td.into_path();
         }
 
         human(format!("failed to compile `{}`, intermediate artifacts can be \
-                       found at `{}`", pkg, target_dir.display()))
+                       found at `{}`", pkg, ws.target_dir().display()))
     }));
     let binaries: Vec<(&str, &Path)> = try!(compile.binaries.iter().map(|bin| {
         let name = bin.file_name().unwrap();
@@ -224,7 +224,7 @@ pub fn install(root: Option<&str>,
     if !source_id.is_path() {
         // Don't bother grabbing a lock as we're going to blow it all away
         // anyway.
-        let target_dir = target_dir.into_path_unlocked();
+        let target_dir = ws.target_dir().into_path_unlocked();
         try!(fs::remove_dir_all(&target_dir));
     }
 

src/cargo/ops/cargo_package.rs

@@ -52,7 +52,7 @@ pub fn package(ws: &Workspace,
     }
 
     let filename = format!("{}-{}.crate", pkg.name(), pkg.version());
-    let dir = config.target_dir(ws).join("package");
+    let dir = ws.target_dir().join("package");
     let mut dst = {
         let tmp = format!(".{}", filename);
         try!(dir.open_rw(&tmp, config, "package scratch space"))
@@ -266,7 +266,7 @@ fn run_verify(ws: &Workspace, tar: &File, opts: &PackageOpts) -> CargoResult<()>
     let new_pkg = Package::new(new_manifest, &manifest_path);
 
     // Now that we've rewritten all our path dependencies, compile it!
-    let ws = Workspace::one(new_pkg, config);
+    let ws = try!(Workspace::one(new_pkg, config, None));
     try!(ops::compile_ws(&ws, None, &ops::CompileOptions {
         config: config,
         jobs: opts.jobs,

src/cargo/ops/cargo_rustc/layout.rs

@@ -72,7 +72,7 @@ impl Layout {
     pub fn new(ws: &Workspace,
                triple: Option<&str>,
                dest: &str) -> CargoResult<Layout> {
-        let mut path = ws.config().target_dir(ws);
+        let mut path = ws.target_dir();
         // Flexible target specifications often point at filenames, so interpret
         // the target triple as a Path and then just use the file stem as the
         // component for the directory name.

src/cargo/util/config.rs

@@ -13,7 +13,7 @@ use std::str::FromStr;
 use rustc_serialize::{Encodable,Encoder};
 use toml;
 use core::shell::{Verbosity, ColorConfig};
-use core::{MultiShell, Workspace};
+use core::MultiShell;
 use util::{CargoResult, CargoError, ChainError, Rustc, internal, human};
 use util::{Filesystem, LazyCell};
 
@@ -28,7 +28,6 @@ pub struct Config {
     values: LazyCell<HashMap<String, ConfigValue>>,
     cwd: PathBuf,
     rustdoc: LazyCell<PathBuf>,
-    target_dir: RefCell<Option<Filesystem>>,
     extra_verbose: Cell<bool>,
     frozen: Cell<bool>,
     locked: Cell<bool>,
@@ -37,23 +36,18 @@ pub struct Config {
 impl Config {
     pub fn new(shell: MultiShell,
                cwd: PathBuf,
-               homedir: PathBuf) -> CargoResult<Config> {
-        let mut cfg = Config {
+               homedir: PathBuf) -> Config {
+        Config {
             home_path: Filesystem::new(homedir),
             shell: RefCell::new(shell),
             rustc: LazyCell::new(),
             cwd: cwd,
             values: LazyCell::new(),
             rustdoc: LazyCell::new(),
-            target_dir: RefCell::new(None),
             extra_verbose: Cell::new(false),
             frozen: Cell::new(false),
             locked: Cell::new(false),
-        };
-
-        try!(cfg.scrape_target_dir_config());
-
-        Ok(cfg)
+        }
     }
 
     pub fn default() -> CargoResult<Config> {
@@ -65,7 +59,7 @@ impl Config {
             human("Cargo couldn't find your home directory. \
                   This probably means that $HOME was not set.")
         }));
-        Config::new(shell, cwd, homedir)
+        Ok(Config::new(shell, cwd, homedir))
     }
 
     pub fn home(&self) -> &Filesystem { &self.home_path }
@@ -108,14 +102,15 @@ impl Config {
 
     pub fn cwd(&self) -> &Path { &self.cwd }
 
-    pub fn target_dir(&self, ws: &Workspace) -> Filesystem {
-        self.target_dir.borrow().clone().unwrap_or_else(|| {
-            Filesystem::new(ws.root().join("target"))
-        })
-    }
-
-    pub fn set_target_dir(&self, path: Filesystem) {
-        *self.target_dir.borrow_mut() = Some(path);
+    pub fn target_dir(&self) -> CargoResult<Option<Filesystem>> {
+        if let Some(dir) = env::var_os("CARGO_TARGET_DIR") {
+            Ok(Some(Filesystem::new(self.cwd.join(dir))))
+        } else if let Some(val) = try!(self.get_path("build.target-dir")) {
+            let val = self.cwd.join(val.val);
+            Ok(Some(Filesystem::new(val)))
+        } else {
+            Ok(None)
+        }
     }
 
     fn get(&self, key: &str) -> CargoResult<Option<ConfigValue>> {
@@ -292,8 +287,11 @@ impl Config {
                      locked: bool) -> CargoResult<()> {
         let extra_verbose = verbose >= 2;
         let verbose = if verbose == 0 {None} else {Some(true)};
-        let cfg_verbose = try!(self.get_bool("term.verbose")).map(|v| v.val);
-        let cfg_color = try!(self.get_string("term.color")).map(|v| v.val);
+
+        // Ignore errors in the configuration files.
+        let cfg_verbose = self.get_bool("term.verbose").unwrap_or(None).map(|v| v.val);
+        let cfg_color = self.get_string("term.color").unwrap_or(None).map(|v| v.val);
+
         let color = color.as_ref().or(cfg_color.as_ref());
 
         let verbosity = match (verbose, cfg_verbose, quiet) {
@@ -369,16 +367,6 @@ impl Config {
         }
     }
 
-    fn scrape_target_dir_config(&mut self) -> CargoResult<()> {
-        if let Some(dir) = env::var_os("CARGO_TARGET_DIR") {
-            *self.target_dir.borrow_mut() = Some(Filesystem::new(self.cwd.join(dir)));
-        } else if let Some(val) = try!(self.get_path("build.target-dir")) {
-            let val = self.cwd.join(val.val);
-            *self.target_dir.borrow_mut() = Some(Filesystem::new(val));
-        }
-        Ok(())
-    }
-
     fn get_tool(&self, tool: &str) -> CargoResult<PathBuf> {
         let var = tool.chars().flat_map(|c| c.to_uppercase()).collect::<String>();
         if let Some(tool_path) = env::var_os(&var) {

tests/bad-config.rs

@@ -112,7 +112,10 @@ fn bad5() {
     assert_that(foo.cargo("new")
                    .arg("-v").arg("foo").cwd(&foo.root().join("foo")),
                 execs().with_status(101).with_stderr("\
-[ERROR] Couldn't load Cargo configuration
+[ERROR] Failed to create project `foo` at `[..]`
+
+Caused by:
+  Couldn't load Cargo configuration
 
 Caused by:
   failed to merge key `foo` between files:

tests/version.rs

@@ -56,3 +56,22 @@ fn version_works_without_rustc() {
     assert_that(p.cargo_process("version").env("PATH", ""),
                 execs().with_status(0));
 }
+
+#[test]
+fn version_works_with_bad_config() {
+    let p = project("foo")
+        .file(".cargo/config", "this is not toml");
+    assert_that(p.cargo_process("version"),
+                execs().with_status(0));
+}
+
+#[test]
+fn version_works_with_bad_target_dir() {
+    let p = project("foo")
+        .file(".cargo/config", r#"
+            [build]
+            target-dir = 4
+        "#);
+    assert_that(p.cargo_process("version"),
+                execs().with_status(0));
+}