Auto merge of #1991 - carols10cents:pipelinify, r=alexcrichton

Hi! This is an attempt to start refactoring some of the internals to be more like a pipeline, and eventually enable the kind of functionality I tried to add in #1968 without having to add as much duplication. Turns out there's a fair bit of duplication in the code today, I think this helps address it!

I may have totally gone against some abstractions... namely I made a way to create `Package`s from a `manifest_path` and a `config`, without needing a `Source`. I think it cleans up the code quite a bit, and I think makes things a bit more pipeliney in that the `Source` isn't updated until we really need it to be (as opposed to having to use `preload` to avoid updating it again). But I'm open to the possibility that I'm moving things around to where no one who knows the code well will be able to find them ;)

This *should* be a Real Refactor in the sense that these changes don't change behavior-- except in one test case, where the same error happens as did before, but it's going through a `chain_error` now so has a slightly different message.

src/cargo/core/package.rs

@@ -4,9 +4,10 @@ use std::slice;
 use std::path::{Path, PathBuf};
 use semver::Version;
 
-use core::{Dependency, Manifest, PackageId, Registry, Target, Summary, Metadata};
+use core::{Dependency, Manifest, PackageId, SourceId, Registry, Target, Summary, Metadata};
+use ops;
 use core::dependency::SerializedDependency;
-use util::{CargoResult, graph};
+use util::{CargoResult, graph, Config};
 use rustc_serialize::{Encoder,Encodable};
 use core::source::Source;
 
@@ -58,6 +59,14 @@ impl Package {
         }
     }
 
+    pub fn for_path(manifest_path: &Path, config: &Config) -> CargoResult<Package> {
+        let path = manifest_path.parent().unwrap();
+        let source_id = try!(SourceId::for_path(path));
+        let (pkg, _) = try!(ops::read_package(&manifest_path, &source_id,
+                                              config));
+        Ok(pkg)
+    }
+
     pub fn dependencies(&self) -> &[Dependency] { self.manifest.dependencies() }
     pub fn manifest(&self) -> &Manifest { &self.manifest }
     pub fn manifest_path(&self) -> &Path { &self.manifest_path }

src/cargo/core/registry.rs

@@ -147,11 +147,6 @@ impl<'cfg> PackageRegistry<'cfg> {
         Ok(())
     }
 
-    pub fn preload(&mut self, id: &SourceId, source: Box<Source + 'cfg>) {
-        self.sources.insert(id, source);
-        self.source_ids.insert(id.clone(), (id.clone(), Kind::Locked));
-    }
-
     pub fn add_sources(&mut self, ids: &[SourceId]) -> CargoResult<()> {
         for id in ids.iter() {
             try!(self.load(id, Kind::Locked));

src/cargo/ops/cargo_clean.rs

@@ -3,9 +3,8 @@ use std::fs;
 use std::io::prelude::*;
 use std::path::Path;
 
-use core::{PackageSet, Profiles, Profile};
+use core::{Package, PackageSet, Profiles, Profile};
 use core::source::{Source, SourceMap};
-use sources::PathSource;
 use util::{CargoResult, human, ChainError, Config};
 use ops::{self, Layout, Context, BuildConfig, Kind};
 
@@ -17,10 +16,7 @@ pub struct CleanOptions<'a> {
 
 /// Cleans the project from build artifacts.
 pub fn clean(manifest_path: &Path, opts: &CleanOptions) -> CargoResult<()> {
-    let mut src = try!(PathSource::for_path(manifest_path.parent().unwrap(),
-                                            opts.config));
-    try!(src.update());
-    let root = try!(src.root_package());
+    let root = try!(Package::for_path(manifest_path, opts.config));
     let target_dir = opts.config.target_dir(&root);
 
     // If we have a spec, then we need to delete some packages, otherwise, just

src/cargo/ops/cargo_compile.rs

@@ -28,11 +28,10 @@ use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
 use core::registry::PackageRegistry;
-use core::{Source, SourceId, PackageSet, Package, Target, PackageId};
+use core::{Source, SourceId, PackageSet, Package, Target};
 use core::{Profile, TargetKind};
 use core::resolver::Method;
 use ops::{self, BuildOutput, ExecEngine};
-use sources::{PathSource};
 use util::config::{ConfigValue, Config};
 use util::{CargoResult, internal, human, ChainError, profile};
 
@@ -87,22 +86,16 @@ pub fn compile<'a>(manifest_path: &Path,
                    -> CargoResult<ops::Compilation<'a>> {
     debug!("compile; manifest-path={}", manifest_path.display());
 
-    let mut source = try!(PathSource::for_path(manifest_path.parent().unwrap(),
-                                               options.config));
-    try!(source.update());
-
-    // TODO: Move this into PathSource
-    let package = try!(source.root_package());
+    let package = try!(Package::for_path(manifest_path, options.config));
     debug!("loaded package; package={}", package);
 
     for key in package.manifest().warnings().iter() {
         try!(options.config.shell().warn(key))
     }
-    compile_pkg(&package, Some(Box::new(source)), options)
+    compile_pkg(&package, options)
 }
 
 pub fn compile_pkg<'a>(package: &Package,
-                       source: Option<Box<Source + 'a>>,
                        options: &CompileOptions<'a>)
                        -> CargoResult<ops::Compilation<'a>> {
     let CompileOptions { config, jobs, target, spec, features,
@@ -127,12 +120,6 @@ pub fn compile_pkg<'a>(package: &Package,
 
     let (packages, resolve_with_overrides, sources) = {
         let mut registry = PackageRegistry::new(config);
-        if let Some(source) = source {
-            registry.preload(package.package_id().source_id(), source);
-        } else {
-            try!(registry.add_sources(&[package.package_id().source_id()
-                                               .clone()]));
-        }
 
         // First, resolve the package's *listed* dependencies, as well as
         // downloading and updating all remotes and such.
@@ -155,12 +142,7 @@ pub fn compile_pkg<'a>(package: &Package,
                 try!(ops::resolve_with_previous(&mut registry, package, method,
                                                 Some(&resolve), None));
 
-        let req: Vec<PackageId> = resolved_with_overrides.iter().map(|r| {
-            r.clone()
-        }).collect();
-        let packages = try!(registry.get(&req).chain_error(|| {
-            human("Unable to get packages from source")
-        }));
+        let packages = try!(ops::get_resolved_packages(&resolved_with_overrides, &mut registry));
 
         (packages, resolved_with_overrides, registry.move_sources())
     };

src/cargo/ops/cargo_doc.rs

@@ -3,10 +3,8 @@ use std::fs;
 use std::path::Path;
 use std::process::Command;
 
-use core::PackageIdSpec;
-use core::source::Source;
+use core::{Package, PackageIdSpec};
 use ops;
-use sources::PathSource;
 use util::{CargoResult, human};
 
 pub struct DocOptions<'a> {
@@ -16,10 +14,7 @@ pub struct DocOptions<'a> {
 
 pub fn doc(manifest_path: &Path,
            options: &DocOptions) -> CargoResult<()> {
-    let mut source = try!(PathSource::for_path(manifest_path.parent().unwrap(),
-                                               options.compile_opts.config));
-    try!(source.update());
-    let package = try!(source.root_package());
+    let package = try!(Package::for_path(manifest_path, options.compile_opts.config));
 
     let mut lib_names = HashSet::new();
     let mut bin_names = HashSet::new();

src/cargo/ops/cargo_fetch.rs

@@ -1,25 +1,23 @@
 use std::path::Path;
 
 use core::registry::PackageRegistry;
-use core::{Source, PackageId};
+use core::{Package, PackageId, Resolve};
 use ops;
-use sources::PathSource;
 use util::{CargoResult, Config, human, ChainError};
 
 /// Executes `cargo fetch`.
 pub fn fetch(manifest_path: &Path, config: &Config) -> CargoResult<()> {
-    let mut source = try!(PathSource::for_path(manifest_path.parent().unwrap(),
-                                               config));
-    try!(source.update());
-    let package = try!(source.root_package());
-
+    let package = try!(Package::for_path(manifest_path, config));
     let mut registry = PackageRegistry::new(config);
-    registry.preload(package.package_id().source_id(), Box::new(source));
     let resolve = try!(ops::resolve_pkg(&mut registry, &package));
+    let _ = try!(get_resolved_packages(&resolve, &mut registry));
+    Ok(())
+}
 
+pub fn get_resolved_packages(resolve: &Resolve, registry: &mut PackageRegistry)
+                             -> CargoResult<Vec<Package>> {
     let ids: Vec<PackageId> = resolve.iter().cloned().collect();
-    try!(registry.get(&ids).chain_error(|| {
+    registry.get(&ids).chain_error(|| {
         human("unable to get packages from source")
-    }));
-    Ok(())
+    })
 }

src/cargo/ops/cargo_generate_lockfile.rs

@@ -3,10 +3,9 @@ use std::path::Path;
 
 use core::PackageId;
 use core::registry::PackageRegistry;
-use core::{Source, Resolve, SourceId};
+use core::{Resolve, SourceId, Package};
 use core::resolver::Method;
 use ops;
-use sources::{PathSource};
 use util::config::{Config};
 use util::{CargoResult, human};
 
@@ -19,12 +18,8 @@ pub struct UpdateOptions<'a> {
 
 pub fn generate_lockfile(manifest_path: &Path, config: &Config)
                          -> CargoResult<()> {
-    let mut source = try!(PathSource::for_path(manifest_path.parent().unwrap(),
-                                               config));
-    try!(source.update());
-    let package = try!(source.root_package());
+    let package = try!(Package::for_path(manifest_path, config));
     let mut registry = PackageRegistry::new(config);
-    registry.preload(package.package_id().source_id(), Box::new(source));
     let resolve = try!(ops::resolve_with_previous(&mut registry, &package,
                                                   Method::Everything,
                                                   None, None));
@@ -34,10 +29,7 @@ pub fn generate_lockfile(manifest_path: &Path, config: &Config)
 
 pub fn update_lockfile(manifest_path: &Path,
                        opts: &UpdateOptions) -> CargoResult<()> {
-    let mut source = try!(PathSource::for_path(manifest_path.parent().unwrap(),
-                                               opts.config));
-    try!(source.update());
-    let package = try!(source.root_package());
+    let package = try!(Package::for_path(manifest_path, opts.config));
 
     let previous_resolve = match try!(ops::load_pkg_lockfile(&package)) {
         Some(resolve) => resolve,
@@ -85,7 +77,6 @@ pub fn update_lockfile(manifest_path: &Path,
         None => to_avoid.extend(previous_resolve.iter()),
     }
 
-    registry.preload(package.package_id().source_id(), Box::new(source));
     let resolve = try!(ops::resolve_with_previous(&mut registry,
                                                   &package,
                                                   Method::Everything,

src/cargo/ops/cargo_package.rs

@@ -6,7 +6,7 @@ use tar::Archive;
 use flate2::{GzBuilder, Compression};
 use flate2::read::GzDecoder;
 
-use core::{Source, SourceId, Package, PackageId};
+use core::{SourceId, Package, PackageId};
 use sources::PathSource;
 use util::{self, CargoResult, human, internal, ChainError, Config};
 use ops;
@@ -29,7 +29,6 @@ pub fn package(manifest_path: &Path,
                metadata: bool) -> CargoResult<Option<PathBuf>> {
     let mut src = try!(PathSource::for_path(manifest_path.parent().unwrap(),
                                             config));
-    try!(src.update());
     let pkg = try!(src.root_package());
 
     if metadata {
@@ -179,7 +178,7 @@ fn run_verify(config: &Config, pkg: &Package, tar: &Path)
     let new_pkg = Package::new(new_manifest, &manifest_path);
 
     // Now that we've rewritten all our path dependencies, compile it!
-    try!(ops::compile_pkg(&new_pkg, None, &ops::CompileOptions {
+    try!(ops::compile_pkg(&new_pkg, &ops::CompileOptions {
         config: config,
         jobs: None,
         target: None,

src/cargo/ops/cargo_pkgid.rs

@@ -1,17 +1,13 @@
 use std::path::Path;
 
 use ops;
-use core::{Source, PackageIdSpec};
-use sources::{PathSource};
+use core::{PackageIdSpec, Package};
 use util::{CargoResult, human, Config};
 
 pub fn pkgid(manifest_path: &Path,
              spec: Option<&str>,
              config: &Config) -> CargoResult<PackageIdSpec> {
-    let mut source = try!(PathSource::for_path(&manifest_path.parent().unwrap(),
-                                               config));
-    try!(source.update());
-    let package = try!(source.root_package());
+    let package = try!(Package::for_path(manifest_path, config));
 
     let lockfile = package.root().join("Cargo.lock");
     let source_id = package.package_id().source_id();

src/cargo/ops/cargo_run.rs

@@ -2,17 +2,13 @@ use std::path::Path;
 
 use ops::{self, ExecEngine, CompileFilter};
 use util::{self, CargoResult, human, process, ProcessError};
-use core::source::Source;
-use sources::PathSource;
+use core::Package;
 
 pub fn run(manifest_path: &Path,
            options: &ops::CompileOptions,
            args: &[String]) -> CargoResult<Option<ProcessError>> {
     let config = options.config;
-    let mut src = try!(PathSource::for_path(&manifest_path.parent().unwrap(),
-                                            config));
-    try!(src.update());
-    let root = try!(src.root_package());
+    let root = try!(Package::for_path(manifest_path, config));
 
     let mut bins = root.manifest().targets().iter().filter(|a| {
         !a.is_lib() && !a.is_custom_build() && match options.filter {

src/cargo/ops/mod.rs

@@ -20,7 +20,7 @@ pub use self::cargo_package::package;
 pub use self::registry::{publish, registry_configuration, RegistryConfig};
 pub use self::registry::{registry_login, search, http_proxy_exists, http_handle};
 pub use self::registry::{modify_owners, yank, OwnersOptions};
-pub use self::cargo_fetch::{fetch};
+pub use self::cargo_fetch::{fetch, get_resolved_packages};
 pub use self::cargo_pkgid::pkgid;
 pub use self::resolve::{resolve_pkg, resolve_with_previous};
 

src/cargo/ops/registry.rs

@@ -15,7 +15,7 @@ use core::{Package, SourceId};
 use core::dependency::Kind;
 use core::manifest::ManifestMetadata;
 use ops;
-use sources::{PathSource, RegistrySource};
+use sources::{RegistrySource};
 use util::config;
 use util::{CargoResult, human, ChainError, ToUrl};
 use util::config::{Config, ConfigValue, Location};
@@ -31,10 +31,7 @@ pub fn publish(manifest_path: &Path,
                token: Option<String>,
                index: Option<String>,
                verify: bool) -> CargoResult<()> {
-    let mut src = try!(PathSource::for_path(manifest_path.parent().unwrap(),
-                                            config));
-    try!(src.update());
-    let pkg = try!(src.root_package());
+    let pkg = try!(Package::for_path(&manifest_path, config));
 
     let (mut registry, reg_id) = try!(registry(config, token, index));
     try!(verify_dependencies(&pkg, &reg_id));
@@ -264,10 +261,7 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> {
         Some(ref name) => name.clone(),
         None => {
             let manifest_path = try!(find_root_manifest_for_cwd(None));
-            let mut src = try!(PathSource::for_path(manifest_path.parent().unwrap(),
-                                                    config));
-            try!(src.update());
-            let pkg = try!(src.root_package());
+            let pkg = try!(Package::for_path(&manifest_path, config));
             pkg.name().to_string()
         }
     };
@@ -327,10 +321,7 @@ pub fn yank(config: &Config,
         Some(name) => name,
         None => {
             let manifest_path = try!(find_root_manifest_for_cwd(None));
-            let mut src = try!(PathSource::for_path(manifest_path.parent().unwrap(),
-                                                    config));
-            try!(src.update());
-            let pkg = try!(src.root_package());
+            let pkg = try!(Package::for_path(&manifest_path, config));
             pkg.name().to_string()
         }
     };

src/cargo/ops/resolve.rs

@@ -37,6 +37,9 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
                                  to_avoid: Option<&HashSet<&'a PackageId>>)
                                  -> CargoResult<Resolve> {
 
+    try!(registry.add_sources(&[package.package_id().source_id()
+                                       .clone()]));
+
     // Here we place an artificial limitation that all non-registry sources
     // cannot be locked at more than one revision. This means that if a git
     // repository provides more than one package, they must all be updated in

src/cargo/sources/path.rs

@@ -46,12 +46,10 @@ impl<'cfg> PathSource<'cfg> {
         }
     }
 
-    pub fn root_package(&self) -> CargoResult<Package> {
+    pub fn root_package(&mut self) -> CargoResult<Package> {
         trace!("root_package; source={:?}", self);
 
-        if !self.updated {
-            return Err(internal("source has not been updated"))
-        }
+        try!(self.update());
 
         match self.packages.iter().find(|p| p.root() == &*self.path) {
             Some(pkg) => Ok(pkg.clone()),

tests/test_cargo_compile_path_deps.rs

@@ -513,8 +513,12 @@ test!(error_message_for_missing_manifest {
     assert_that(p.cargo_process("build"),
                 execs()
                 .with_status(101)
-                .with_stderr(&format!("Could not find `Cargo.toml` in `{}`\n",
-                                      p.root().join("src").join("bar").display())));
+                .with_stderr(&format!("\
+Unable to update file://[..]
+
+Caused by:
+  Could not find `Cargo.toml` in `{}`
+", p.root().join("src").join("bar").display())));
 
 });