Auto merge of #341 - pietroalbini:refactor-runcommand, r=pietroalbini

Refactor RunCommand and add SandboxedRunCommand

Fixes #101

Author: bors

src/docker.rs

@@ -13,11 +13,10 @@ pub static IMAGE_NAME: &'static str = "crater";
 /// to exist in the `docker` directory, at runtime.
 pub fn build_container(docker_env: &str) -> Result<()> {
     let dockerfile = format!("docker/Dockerfile.{}", docker_env);
-    RunCommand::new(
-        "docker",
-        &["build", "-f", &dockerfile, "-t", IMAGE_NAME, "docker"],
-    ).enable_timeout(false)
-    .run()
+    RunCommand::new("docker")
+        .args(&["build", "-f", &dockerfile, "-t", IMAGE_NAME, "docker"])
+        .enable_timeout(false)
+        .run()
 }
 
 #[derive(Copy, Clone)]
@@ -26,67 +25,72 @@ pub enum MountPerms {
     ReadOnly,
 }
 
-struct MountConfig<'a> {
+struct MountConfig {
     host_path: PathBuf,
-    container_path: &'a str,
+    container_path: PathBuf,
     perm: MountPerms,
 }
 
-impl<'a> MountConfig<'a> {
+impl MountConfig {
     fn to_arg(&self) -> String {
         let perm = match self.perm {
             MountPerms::ReadWrite => "rw",
             MountPerms::ReadOnly => "ro",
         };
         format!(
             "{}:{}:{},Z",
-            absolute(&self.host_path).display(),
-            self.container_path,
+            absolute(&self.host_path).to_string_lossy(),
+            self.container_path.to_string_lossy(),
             perm
         )
     }
 }
 
-pub struct ContainerBuilder<'a> {
-    image: &'a str,
-    mounts: Vec<MountConfig<'a>>,
-    env: Vec<(&'static str, String)>,
+pub struct ContainerBuilder {
+    image: String,
+    mounts: Vec<MountConfig>,
+    env: Vec<(String, String)>,
     memory_limit: Option<Size>,
-    networking_disabled: bool,
+    enable_networking: bool,
 }
 
-impl<'a> ContainerBuilder<'a> {
-    pub fn new(image: &'a str) -> Self {
+impl ContainerBuilder {
+    pub fn new<S: Into<String>>(image: S) -> Self {
         ContainerBuilder {
-            image,
+            image: image.into(),
             mounts: Vec::new(),
             env: Vec::new(),
             memory_limit: None,
-            networking_disabled: false,
+            enable_networking: true,
         }
     }
 
-    pub fn mount(mut self, host_path: PathBuf, container_path: &'a str, perm: MountPerms) -> Self {
+    pub fn mount<P1: Into<PathBuf>, P2: Into<PathBuf>>(
+        mut self,
+        host_path: P1,
+        container_path: P2,
+        perm: MountPerms,
+    ) -> Self {
         self.mounts.push(MountConfig {
-            host_path,
-            container_path,
+            host_path: host_path.into(),
+            container_path: container_path.into(),
             perm,
         });
         self
     }
 
-    pub fn env(mut self, key: &'static str, value: String) -> Self {
-        self.env.push((key, value));
+    pub fn env<S1: Into<String>, S2: Into<String>>(mut self, key: S1, value: S2) -> Self {
+        self.env.push((key.into(), value.into()));
         self
     }
 
-    pub fn memory_limit(mut self, limit: Size) -> Self {
-        self.memory_limit = Some(limit);
+    pub fn memory_limit(mut self, limit: Option<Size>) -> Self {
+        self.memory_limit = limit;
         self
     }
 
-    pub fn disable_networking(mut self) -> Self {
-        self.networking_disabled = true;
+    pub fn enable_networking(mut self, enable: bool) -> Self {
+        self.enable_networking = enable;
         self
     }
 
@@ -99,7 +103,7 @@ impl<'a> ContainerBuilder<'a> {
             args.push(mount.to_arg())
         }
 
-        for &(var, ref value) in &self.env {
+        for &(ref var, ref value) in &self.env {
             args.push("-e".into());
             args.push(format!{"{}={}", var, value})
         }
@@ -109,14 +113,14 @@ impl<'a> ContainerBuilder<'a> {
             args.push(limit.to_string());
         }
 
-        if self.networking_disabled {
+        if !self.enable_networking {
             args.push("--network".into());
             args.push("none".into());
         }
 
-        args.push(self.image.into());
+        args.push(self.image);
 
-        let (out, _) = RunCommand::new("docker", &*args).run_capture()?;
+        let (out, _) = RunCommand::new("docker").args(&*args).run_capture()?;
         Ok(Container { id: out[0].clone() })
     }
 
@@ -158,12 +162,15 @@ impl Display for Container {
 
 impl Container {
     pub fn run(&self, quiet: bool) -> Result<()> {
-        RunCommand::new("docker", &["start", "-a", &self.id])
+        RunCommand::new("docker")
+            .args(&["start", "-a", &self.id])
             .quiet(quiet)
             .run()
     }
 
     pub fn delete(&self) -> Result<()> {
-        RunCommand::new("docker", &["rm", "-f", &self.id]).run()
+        RunCommand::new("docker")
+            .args(&["rm", "-f", &self.id])
+            .run()
     }
 }

src/ex_prepare.rs

@@ -8,7 +8,7 @@ use run::RunCommand;
 use std::fs;
 use std::path::{Path, PathBuf};
 use toml_frobber::TomlFrobber;
-use toolchain::{CargoState, Toolchain};
+use toolchain::Toolchain;
 
 fn froml_dir(ex_name: &str) -> PathBuf {
     EXPERIMENT_DIR.join(ex_name).join("fromls")
@@ -37,7 +37,8 @@ pub fn capture_shas<DB: WriteResults>(ex: &Experiment, crates: &[Crate], db: &DB
     for krate in crates {
         if let Crate::GitHub(ref repo) = *krate {
             let dir = repo.mirror_dir();
-            let r = RunCommand::new("git", &["rev-parse", "HEAD"])
+            let r = RunCommand::new("git")
+                .args(&["rev-parse", "HEAD"])
                 .cd(&dir)
                 .run_capture();
 
@@ -148,36 +149,26 @@ pub fn capture_lockfile(
 
     with_work_crate(ex, toolchain, krate, |path| {
         with_frobbed_toml(ex, krate, path)?;
-        capture_lockfile_inner(config, ex, krate, path, toolchain)
+        capture_lockfile_inner(ex, krate, path, toolchain)
     }).chain_err(|| format!("failed to generate lockfile for {}", krate))?;
 
     Ok(())
 }
 
 fn capture_lockfile_inner(
-    config: &Config,
     ex: &Experiment,
     krate: &Crate,
     path: &Path,
     toolchain: &Toolchain,
 ) -> Result<()> {
-    let args = &[
-        "generate-lockfile",
-        "--manifest-path",
-        "Cargo.toml",
-        "-Zno-index-update",
-    ];
-    toolchain
-        .run_cargo(
-            config,
-            ex,
-            path,
-            args,
-            CargoState::Unlocked,
-            false,
-            false,
-            false,
-        ).chain_err(|| format!("unable to generate lockfile for {}", krate))?;
+    RunCommand::new(toolchain.cargo().unstable_features(true))
+        .args(&[
+            "generate-lockfile",
+            "--manifest-path",
+            "Cargo.toml",
+            "-Zno-index-update",
+        ]).cd(path)
+        .run()?;
 
     let src_lockfile = &path.join("Cargo.lock");
     let dst_lockfile = &lockfile(&ex.name, krate)?;
@@ -236,18 +227,10 @@ pub fn fetch_crate_deps(
         with_frobbed_toml(ex, krate, path)?;
         with_captured_lockfile(config, ex, krate, path)?;
 
-        let args = &["fetch", "--locked", "--manifest-path", "Cargo.toml"];
-        toolchain
-            .run_cargo(
-                config,
-                ex,
-                path,
-                args,
-                CargoState::Unlocked,
-                false,
-                true,
-                false,
-            ).chain_err(|| format!("unable to fetch deps for {}", krate))?;
+        RunCommand::new(toolchain.cargo())
+            .args(&["fetch", "--locked", "--manifest-path", "Cargo.toml"])
+            .cd(path)
+            .run()?;
 
         Ok(())
     })

src/ex_run.rs

@@ -1,12 +1,45 @@
 use config::Config;
 use crates::Crate;
+use docker::MountPerms;
 use errors::*;
 use ex_prepare::{with_captured_lockfile, with_frobbed_toml, with_work_crate};
 use experiments::Experiment;
 use results::{TestResult, WriteResults};
+use run::RunCommand;
 use std::collections::HashSet;
 use std::path::Path;
-use toolchain::{CargoState, Toolchain};
+use toolchain::Toolchain;
+
+fn run_cargo(
+    config: &Config,
+    ex: &Experiment,
+    source_path: &Path,
+    toolchain: &Toolchain,
+    quiet: bool,
+    args: &[&str],
+) -> Result<()> {
+    let target_dir = toolchain.target_dir(&ex.name);
+    ::std::fs::create_dir_all(&target_dir)?;
+
+    let mut rustflags = format!("--cap-lints={}", ex.cap_lints.to_str());
+    if let Some(ref tc_rustflags) = toolchain.rustflags {
+        rustflags.push(' ');
+        rustflags.push_str(tc_rustflags);
+    }
+
+    RunCommand::new(toolchain.cargo())
+        .args(args)
+        .quiet(quiet)
+        .cd(source_path)
+        .env("CARGO_TARGET_DIR", "/target")
+        .env("CARGO_INCREMENTAL", "0")
+        .env("RUST_BACKTRACE", "full")
+        .env("RUSTFLAGS", rustflags)
+        .sandboxed()
+        .mount(target_dir, "/target", MountPerms::ReadWrite)
+        .memory_limit(Some(config.sandbox.memory_limit))
+        .run()
+}
 
 pub struct RunTestResult {
     pub result: TestResult,
@@ -59,25 +92,21 @@ fn build(
     toolchain: &Toolchain,
     quiet: bool,
 ) -> Result<()> {
-    toolchain.run_cargo(
+    run_cargo(
         config,
         ex,
         source_path,
-        &["build", "--frozen"],
-        CargoState::Locked,
+        toolchain,
         quiet,
-        false,
-        true,
+        &["build", "--frozen"],
     )?;
-    toolchain.run_cargo(
+    run_cargo(
         config,
         ex,
         source_path,
-        &["test", "--frozen", "--no-run"],
-        CargoState::Locked,
+        toolchain,
         quiet,
-        false,
-        true,
+        &["test", "--frozen", "--no-run"],
     )?;
     Ok(())
 }
@@ -89,15 +118,13 @@ fn test(
     toolchain: &Toolchain,
     quiet: bool,
 ) -> Result<()> {
-    toolchain.run_cargo(
+    run_cargo(
         config,
         ex,
         source_path,
-        &["test", "--frozen"],
-        CargoState::Locked,
+        toolchain,
         quiet,
-        false,
-        true,
+        &["test", "--frozen"],
     )
 }
 
@@ -145,18 +172,15 @@ pub fn test_check_only(
     toolchain: &Toolchain,
     quiet: bool,
 ) -> Result<TestResult> {
-    let r = toolchain.run_cargo(
+    if run_cargo(
         config,
         ex,
         source_path,
-        &["check", "--frozen", "--all", "--all-targets"],
-        CargoState::Locked,
+        toolchain,
         quiet,
-        false,
-        true,
-    );
-
-    if r.is_ok() {
+        &["check", "--frozen", "--all", "--all-targets"],
+    ).is_ok()
+    {
         Ok(TestResult::TestPass)
     } else {
         Ok(TestResult::BuildFail)

src/git.rs

@@ -8,11 +8,10 @@ pub fn shallow_clone_or_pull(url: &str, dir: &Path) -> Result<()> {
 
     if !dir.exists() {
         info!("cloning {} into {}", url, dir.display());
-        let r = RunCommand::new(
-            "git",
-            &["clone", "--depth", "1", &url, &dir.to_string_lossy()],
-        ).run()
-        .chain_err(|| format!("unable to clone {}", url));
+        let r = RunCommand::new("git")
+            .args(&["clone", "--depth", "1", &url, &dir.to_string_lossy()])
+            .run()
+            .chain_err(|| format!("unable to clone {}", url));
 
         if r.is_err() && dir.exists() {
             fs::remove_dir_all(dir)?;
@@ -21,8 +20,12 @@ pub fn shallow_clone_or_pull(url: &str, dir: &Path) -> Result<()> {
         r
     } else {
         info!("pulling existing url {} into {}", url, dir.display());
-        RunCommand::new("git", &["fetch", "--all"]).cd(dir).run()?;
-        RunCommand::new("git", &["reset", "--hard", "@{upstream}"])
+        RunCommand::new("git")
+            .args(&["fetch", "--all"])
+            .cd(dir)
+            .run()?;
+        RunCommand::new("git")
+            .args(&["reset", "--hard", "@{upstream}"])
             .cd(dir)
             .run()
             .chain_err(|| format!("unable to pull {}", url))