Cleanup ProcessBuilder, work towards getting tests passing

libs/hamcrest-rust

@@ -1 +1 @@
-Subproject commit de700414aab1aaa4461618ce7a516cb24a8e6665
+Subproject commit 138de49f217604d22e019afd71c529a7d76643f0

src/cargo/core/resolver.rs

@@ -44,9 +44,9 @@ mod test {
     };
 
     use core::{
-        MemRegistry,
         Dependency,
-        Package
+        Package,
+        PackageSet
     };
 
     use super::{
@@ -71,8 +71,8 @@ mod test {
         Dependency::new(name)
     }
 
-    fn registry(pkgs: Vec<Package>) -> MemRegistry {
-        MemRegistry::new(&pkgs)
+    fn registry(pkgs: Vec<Package>) -> PackageSet {
+        PackageSet::new(&pkgs)
     }
 
     #[test]

src/cargo/util/process_builder.rs

@@ -2,88 +2,149 @@ use std::os;
 use std::path::Path;
 use std::io;
 use std::io::process::{Process,ProcessConfig,ProcessOutput,InheritFd};
+use collections::HashMap;
 use ToCargoError;
 use CargoResult;
 
 #[deriving(Clone,Eq)]
 pub struct ProcessBuilder {
-  program: ~str,
-  args: Vec<~str>,
-  path: Vec<~str>,
-  cwd: Path
+    program: ~str,
+    args: Vec<~str>,
+    path: Vec<~str>,
+    env: HashMap<~str, ~str>,
+    cwd: Path
 }
 
 // TODO: Upstream a Windows/Posix branch to Rust proper
 static PATH_SEP : &'static str = ":";
 
 impl ProcessBuilder {
-  pub fn args(mut self, arguments: &[~str]) -> ProcessBuilder {
-    self.args = Vec::from_slice(arguments);
-    self
-  }
+    pub fn args(mut self, arguments: &[~str]) -> ProcessBuilder {
+        self.args = Vec::from_slice(arguments);
+        self
+    }
 
-  pub fn extra_path(mut self, path: Path) -> ProcessBuilder {
-    // For now, just convert to a string, but we should do something better
-    self.path.push(format!("{}", path.display()));
-    self
-  }
+    pub fn extra_path(mut self, path: Path) -> ProcessBuilder {
+        // For now, just convert to a string, but we should do something better
+        self.path.push(format!("{}", path.display()));
+        self
+    }
 
-  pub fn cwd(mut self, path: Path) -> ProcessBuilder {
-    self.cwd = path;
-    self
-  }
+    pub fn cwd(mut self, path: Path) -> ProcessBuilder {
+        self.cwd = path;
+        self
+    }
 
-  // TODO: clean all this up
-  pub fn exec(&self) -> io::IoResult<()> {
-      let mut config = ProcessConfig::new();
+    // TODO: should InheritFd be hardcoded?
+    pub fn exec(&self) -> io::IoResult<()> {
+        let mut config = try!(self.build_config());
+        let env = self.build_env();
 
-      config.program = self.program.as_slice();
-      config.args = self.args.as_slice();
-      config.cwd = Some(&self.cwd);
-      config.stdout = InheritFd(1);
-      config.stderr = InheritFd(2);
+        // Set where the output goes
+        config.env = Some(env.as_slice());
+        config.stdout = InheritFd(1);
+        config.stderr = InheritFd(2);
 
-      let mut process = try!(Process::configure(config));
-      let exit = process.wait();
+        let mut process = try!(Process::configure(config));
+        let exit = process.wait();
 
-      if exit.success() {
-          Ok(())
-      }
-      else {
-          Err(io::IoError {
-              kind: io::OtherIoError,
-              desc: "process did not exit successfully",
-              detail: None
-          })
-      }
-  }
+        if exit.success() {
+            Ok(())
+        }
+        else {
+            Err(io::IoError {
+                kind: io::OtherIoError,
+                desc: "process did not exit successfully",
+                detail: None
+            })
+        }
+    }
 
-  pub fn exec_with_output(&self) -> CargoResult<ProcessOutput> {
-    let mut config = ProcessConfig::new();
+    // TODO: Match exec()
+    pub fn exec_with_output(&self) -> CargoResult<ProcessOutput> {
+        let mut config = ProcessConfig::new();
 
-    println!("cwd: {}", self.cwd.display());
+        config.program = self.program.as_slice();
+        config.args = self.args.as_slice();
+        config.cwd = Some(&self.cwd);
 
-    config.program = self.program.as_slice();
-    config.args = self.args.as_slice();
-    config.cwd = Some(&self.cwd);
+        let os_path = try!(os::getenv("PATH").to_cargo_error("Could not find the PATH environment variable".to_owned(), 1));
+        let path = os_path + PATH_SEP + self.path.connect(PATH_SEP);
 
-    let os_path = try!(os::getenv("PATH").to_cargo_error("Could not find the PATH environment variable".to_owned(), 1));
-    let path = os_path + PATH_SEP + self.path.connect(PATH_SEP);
+        let path = [("PATH".to_owned(), path)];
+        config.env = Some(path.as_slice());
 
-    let path = [("PATH".to_owned(), path)];
-    config.env = Some(path.as_slice());
+        Process::configure(config).map(|mut ok| ok.wait_with_output()).to_cargo_error("Could not spawn process".to_owned(), 1)
+    }
 
-    println!("{:?}", config);
+    fn build_config<'a>(&'a self) -> io::IoResult<ProcessConfig<'a>> {
+        let mut config = ProcessConfig::new();
 
-    Process::configure(config).map(|mut ok| ok.wait_with_output()).to_cargo_error("Could not spawn process".to_owned(), 1)
-  }
+        config.program = self.program.as_slice();
+        config.args = self.args.as_slice();
+        config.cwd = Some(&self.cwd);
+
+        Ok(config)
+    }
+
+    fn build_env(&self) -> ~[(~str, ~str)] {
+        let mut ret = Vec::new();
+
+        for (key, val) in self.env.iter() {
+            // Skip path
+            if key.as_slice() != "PATH" {
+                ret.push((key.clone(), val.clone()));
+            }
+        }
+
+        match self.build_path() {
+            Some(path) => ret.push(("PATH".to_owned(), path)),
+            _ => ()
+        }
+
+        ret.as_slice().to_owned()
+    }
+
+    fn build_path(&self) -> Option<~str> {
+        let path = self.path.connect(PATH_SEP);
+
+        match self.env.find_equiv(&("PATH")) {
+            Some(existing) => {
+                if self.path.is_empty() {
+                    Some(existing.to_owned())
+                }
+                else {
+                    Some(existing.as_slice() + PATH_SEP + path)
+                }
+            }
+            None => {
+                if self.path.is_empty() {
+                    None
+                }
+                else {
+                    Some(path)
+                }
+            }
+        }
+    }
 }
 
 pub fn process(cmd: &str) -> ProcessBuilder {
-  ProcessBuilder {
-    program: cmd.to_owned(),
-    args: vec!(),
-    path: vec!(),
-    cwd: os::getcwd()
-  }
+    ProcessBuilder {
+        program: cmd.to_owned(),
+        args: vec!(),
+        path: vec!(),
+        cwd: os::getcwd(),
+        env: system_env()
+    }
+}
+
+fn system_env() -> HashMap<~str, ~str> {
+    let mut ret = HashMap::new();
+
+    for &(ref key, ref val) in os::env().iter() {
+        ret.insert(key.clone(), val.clone());
+    }
+
+    ret
 }

tests/support.rs

@@ -187,7 +187,7 @@ impl Execs {
       None => ham::success(),
       Some(out) => {
         match str::from_utf8(actual.as_slice()) {
-          None => Err(~"stdout was not utf8 encoded"),
+          None => Err("stdout was not utf8 encoded".to_owned()),
           Some(actual) => {
             ham::expect(actual == out, format!("stdout was `{}`", actual))
           }
@@ -199,7 +199,7 @@ impl Execs {
 
 impl ham::SelfDescribing for Execs {
   fn describe(&self) -> ~str {
-    ~"execs"
+    "execs".to_owned()
   }
 }
 
@@ -209,7 +209,7 @@ impl ham::Matcher<ProcessBuilder> for Execs {
 
     match res {
       Ok(out) => self.match_output(&out),
-      Err(_) => Err(~"could not exec process")
+      Err(_) => Err("could not exec process".to_owned())
     }
   }
 }

tests/test_cargo_compile.rs

@@ -24,10 +24,12 @@ test!(cargo_compile_with_explicit_manifest_path {
             }"#)
         .build();
 
-    p.cargo_process("cargo-compile")
-      .args([~"--manifest-path", ~"Cargo.toml"])
-      .exec_with_output()
+    println!("~~~~~~~");
+    p.cargo_process("cargo")
+      .args(["compile".to_owned(), "--manifest-path".to_owned(), "Cargo.toml".to_owned()])
+      .exec()
       .unwrap();
+    println!("~~~~~~~");
 
     assert_that(&p.root().join("target/foo"), existing_file());