From c94804bdca5c7c9d6314acf1e1937abff36aa664 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 7 Sep 2018 15:13:57 -0700 Subject: [PATCH 01/15] Move downloading crates higher up in Cargo This commit is intended to lay the groundwork for downloading crates in parallel from crates.io. It starts out by lifting up the download operation from deep within the `Source` trait to the `PackageSet`. This should allow us to maintain a queue of downloads specifically in a `PackageSet` and arbitrate access through that one type, making it easier for us to implement parallel downloads. The `Source` trait's `download` method may now fail saying "go download this data", and then the data is fed back into the trait object once it's complete to actually get the `Package` out. --- src/cargo/core/package.rs | 58 ++++++++++++++++++- src/cargo/core/registry.rs | 4 +- src/cargo/core/source/mod.rs | 65 ++++++++++++++++++++- src/cargo/ops/cargo_install.rs | 24 +++++--- src/cargo/sources/directory.rs | 8 ++- src/cargo/sources/git/source.rs | 8 ++- src/cargo/sources/path.rs | 8 ++- src/cargo/sources/registry/local.rs | 17 ++++-- src/cargo/sources/registry/mod.rs | 77 +++++++++++++++++-------- src/cargo/sources/registry/remote.rs | 84 +++++++++------------------- src/cargo/sources/replaced.rs | 18 +++++- tests/testsuite/registry.rs | 10 +--- 12 files changed, 269 insertions(+), 112 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 09473864586..9536f117beb 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -11,6 +11,7 @@ use lazycell::LazyCell; use core::{Dependency, Manifest, PackageId, SourceId, Target}; use core::{FeatureMap, SourceMap, Summary}; +use core::source::MaybePackage; use core::interning::InternedString; use util::{internal, lev_distance, Config}; use util::errors::{CargoResult, CargoResultExt}; @@ -240,16 +241,22 @@ impl hash::Hash for Package { pub struct PackageSet<'cfg> { packages: HashMap>, sources: RefCell>, + config: &'cfg Config, } impl<'cfg> PackageSet<'cfg> { - pub fn new(package_ids: &[PackageId], sources: SourceMap<'cfg>) -> PackageSet<'cfg> { + pub fn new( + package_ids: &[PackageId], + sources: SourceMap<'cfg>, + config: &'cfg Config, + ) -> PackageSet<'cfg> { PackageSet { packages: package_ids .iter() .map(|id| (id.clone(), LazyCell::new())) .collect(), sources: RefCell::new(sources), + config, } } @@ -271,6 +278,14 @@ impl<'cfg> PackageSet<'cfg> { let pkg = source .download(id) .chain_err(|| format_err!("unable to get packages from source"))?; + let pkg = match pkg { + MaybePackage::Ready(pkg) => pkg, + MaybePackage::Download { url, descriptor } => { + self.config.shell().status("Downloading", descriptor)?; + let data = download(self.config, &url)?; + source.finish_download(id, data)? + } + }; assert!(slot.fill(pkg).is_ok()); Ok(slot.borrow().unwrap()) } @@ -279,3 +294,44 @@ impl<'cfg> PackageSet<'cfg> { self.sources.borrow() } } + +fn download(config: &Config, url: &str) -> CargoResult> { + use util::network; + use util::Progress; + use util::errors::HttpNot200; + + let mut handle = config.http()?.borrow_mut(); + handle.get(true)?; + handle.url(&url)?; + handle.follow_location(true)?; + let mut body = Vec::new(); + network::with_retry(config, || { + body = Vec::new(); + let mut pb = Progress::new("Fetch", config); + { + handle.progress(true)?; + let mut handle = handle.transfer(); + handle.progress_function(|dl_total, dl_cur, _, _| { + pb.tick(dl_cur as usize, dl_total as usize).is_ok() + })?; + handle.write_function(|buf| { + body.extend_from_slice(buf); + Ok(buf.len()) + })?; + handle.perform().chain_err(|| { + format!("failed to download from `{}`", url) + })?; + } + let code = handle.response_code()?; + if code != 200 && code != 0 { + let url = handle.effective_url()?.unwrap_or(&url); + Err(HttpNot200 { + code, + url: url.to_string(), + }.into()) + } else { + Ok(()) + } + })?; + Ok(body) +} diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index 27c617e4817..1c359b39490 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -37,6 +37,7 @@ pub trait Registry { /// a `Source`. Each `Source` in the map has been updated (using network /// operations if necessary) and is ready to be queried for packages. pub struct PackageRegistry<'cfg> { + config: &'cfg Config, sources: SourceMap<'cfg>, // A list of sources which are considered "overrides" which take precedent @@ -81,6 +82,7 @@ impl<'cfg> PackageRegistry<'cfg> { pub fn new(config: &'cfg Config) -> CargoResult> { let source_config = SourceConfigMap::new(config)?; Ok(PackageRegistry { + config, sources: SourceMap::new(), source_ids: HashMap::new(), overrides: Vec::new(), @@ -94,7 +96,7 @@ impl<'cfg> PackageRegistry<'cfg> { pub fn get(self, package_ids: &[PackageId]) -> PackageSet<'cfg> { trace!("getting packages; sources={}", self.sources.len()); - PackageSet::new(package_ids, self.sources) + PackageSet::new(package_ids, self.sources, self.config) } fn ensure_loaded(&mut self, namespace: &SourceId, kind: Kind) -> CargoResult<()> { diff --git a/src/cargo/core/source/mod.rs b/src/cargo/core/source/mod.rs index 6aedfe85b96..71eab5c2857 100644 --- a/src/cargo/core/source/mod.rs +++ b/src/cargo/core/source/mod.rs @@ -49,7 +49,10 @@ pub trait Source { /// The download method fetches the full package for each name and /// version specified. - fn download(&mut self, package: &PackageId) -> CargoResult; + fn download(&mut self, package: &PackageId) -> CargoResult; + + fn finish_download(&mut self, package: &PackageId, contents: Vec) + -> CargoResult; /// Generates a unique string which represents the fingerprint of the /// current state of the source. @@ -74,6 +77,14 @@ pub trait Source { } } +pub enum MaybePackage { + Ready(Package), + Download { + url: String, + descriptor: String, + } +} + impl<'a, T: Source + ?Sized + 'a> Source for Box { /// Forwards to `Source::supports_checksums` fn supports_checksums(&self) -> bool { @@ -111,10 +122,14 @@ impl<'a, T: Source + ?Sized + 'a> Source for Box { } /// Forwards to `Source::download` - fn download(&mut self, id: &PackageId) -> CargoResult { + fn download(&mut self, id: &PackageId) -> CargoResult { (**self).download(id) } + fn finish_download(&mut self, id: &PackageId, data: Vec) -> CargoResult { + (**self).finish_download(id, data) + } + /// Forwards to `Source::fingerprint` fn fingerprint(&self, pkg: &Package) -> CargoResult { (**self).fingerprint(pkg) @@ -126,6 +141,52 @@ impl<'a, T: Source + ?Sized + 'a> Source for Box { } } +impl<'a, T: Source + ?Sized + 'a> Source for &'a mut T { + fn supports_checksums(&self) -> bool { + (**self).supports_checksums() + } + + fn requires_precise(&self) -> bool { + (**self).requires_precise() + } + + fn query(&mut self, dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()> { + (**self).query(dep, f) + } + + fn fuzzy_query(&mut self, dep: &Dependency, f: &mut FnMut(Summary)) -> CargoResult<()> { + (**self).fuzzy_query(dep, f) + } + + fn source_id(&self) -> &SourceId { + (**self).source_id() + } + + fn replaced_source_id(&self) -> &SourceId { + (**self).replaced_source_id() + } + + fn update(&mut self) -> CargoResult<()> { + (**self).update() + } + + fn download(&mut self, id: &PackageId) -> CargoResult { + (**self).download(id) + } + + fn finish_download(&mut self, id: &PackageId, data: Vec) -> CargoResult { + (**self).finish_download(id, data) + } + + fn fingerprint(&self, pkg: &Package) -> CargoResult { + (**self).fingerprint(pkg) + } + + fn verify(&self, pkg: &PackageId) -> CargoResult<()> { + (**self).verify(pkg) + } +} + /// A `HashMap` of `SourceId` -> `Box` #[derive(Default)] pub struct SourceMap<'src> { diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 60e6c30ed8d..0d75bd5ccf8 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -12,6 +12,8 @@ use toml; use core::{Dependency, Edition, Package, PackageIdSpec, Source, SourceId}; use core::{PackageId, Workspace}; +use core::source::SourceMap; +use core::package::PackageSet; use core::compiler::{DefaultExecutor, Executor}; use ops::{self, CompileFilter}; use sources::{GitSource, PathSource, SourceConfigMap}; @@ -499,22 +501,28 @@ where source.source_id(), )?; let deps = source.query_vec(&dep)?; - match deps.iter().map(|p| p.package_id()).max() { - Some(pkgid) => { - let pkg = source.download(pkgid)?; - Ok((pkg, Box::new(source))) - } + let pkgid = match deps.iter().map(|p| p.package_id()).max() { + Some(pkgid) => pkgid, None => { let vers_info = vers.map(|v| format!(" with version `{}`", v)) .unwrap_or_default(); - Err(format_err!( + bail!( "could not find `{}` in {}{}", name, source.source_id(), vers_info - )) + ) } - } + }; + + let pkg = { + let mut map = SourceMap::new(); + map.insert(Box::new(&mut source)); + PackageSet::new(&[pkgid.clone()], map, config) + .get(&pkgid)? + .clone() + }; + Ok((pkg, Box::new(source))) } None => { let candidates = list_all(&mut source)?; diff --git a/src/cargo/sources/directory.rs b/src/cargo/sources/directory.rs index 91c3e50bc7a..c4918e1b82e 100644 --- a/src/cargo/sources/directory.rs +++ b/src/cargo/sources/directory.rs @@ -9,6 +9,7 @@ use hex; use serde_json; use core::{Dependency, Package, PackageId, Source, SourceId, Summary}; +use core::source::MaybePackage; use sources::PathSource; use util::{Config, Sha256}; use util::errors::{CargoResult, CargoResultExt}; @@ -150,14 +151,19 @@ impl<'cfg> Source for DirectorySource<'cfg> { Ok(()) } - fn download(&mut self, id: &PackageId) -> CargoResult { + fn download(&mut self, id: &PackageId) -> CargoResult { self.packages .get(id) .map(|p| &p.0) .cloned() + .map(MaybePackage::Ready) .ok_or_else(|| format_err!("failed to find package with id: {}", id)) } + fn finish_download(&mut self, _id: &PackageId, _data: Vec) -> CargoResult { + panic!("no downloads to do") + } + fn fingerprint(&self, pkg: &Package) -> CargoResult { Ok(pkg.package_id().version().to_string()) } diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index f4c59916467..2633be285e6 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -2,7 +2,7 @@ use std::fmt::{self, Debug, Formatter}; use url::Url; -use core::source::{Source, SourceId}; +use core::source::{Source, SourceId, MaybePackage}; use core::GitReference; use core::{Dependency, Package, PackageId, Summary}; use util::Config; @@ -210,7 +210,7 @@ impl<'cfg> Source for GitSource<'cfg> { self.path_source.as_mut().unwrap().update() } - fn download(&mut self, id: &PackageId) -> CargoResult { + fn download(&mut self, id: &PackageId) -> CargoResult { trace!( "getting packages for package id `{}` from `{:?}`", id, @@ -222,6 +222,10 @@ impl<'cfg> Source for GitSource<'cfg> { .download(id) } + fn finish_download(&mut self, _id: &PackageId, _data: Vec) -> CargoResult { + panic!("no download should have started") + } + fn fingerprint(&self, _pkg: &Package) -> CargoResult { Ok(self.rev.as_ref().unwrap().to_string()) } diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index f2a4f6ef01a..f16201a4f52 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -9,6 +9,7 @@ use ignore::Match; use ignore::gitignore::GitignoreBuilder; use core::{Dependency, Package, PackageId, Source, SourceId, Summary}; +use core::source::MaybePackage; use ops; use util::{self, internal, CargoResult}; use util::paths; @@ -540,14 +541,19 @@ impl<'cfg> Source for PathSource<'cfg> { Ok(()) } - fn download(&mut self, id: &PackageId) -> CargoResult { + fn download(&mut self, id: &PackageId) -> CargoResult { trace!("getting packages; id={}", id); let pkg = self.packages.iter().find(|pkg| pkg.package_id() == id); pkg.cloned() + .map(MaybePackage::Ready) .ok_or_else(|| internal(format!("failed to find {} in path source", id))) } + fn finish_download(&mut self, _id: &PackageId, _data: Vec) -> CargoResult { + panic!("no download should have started") + } + fn fingerprint(&self, pkg: &Package) -> CargoResult { let (max, max_path) = self.last_modified_file(pkg)?; Ok(format!("{} ({})", max, max_path.display())) diff --git a/src/cargo/sources/registry/local.rs b/src/cargo/sources/registry/local.rs index 32d745a88c3..023e955b857 100644 --- a/src/cargo/sources/registry/local.rs +++ b/src/cargo/sources/registry/local.rs @@ -4,10 +4,9 @@ use std::path::Path; use core::PackageId; use hex; -use sources::registry::{RegistryConfig, RegistryData}; -use util::FileLock; +use sources::registry::{RegistryConfig, RegistryData, MaybeLock}; use util::paths; -use util::{Config, Filesystem, Sha256}; +use util::{Config, Filesystem, Sha256, FileLock}; use util::errors::{CargoResult, CargoResultExt}; pub struct LocalRegistry<'cfg> { @@ -70,7 +69,7 @@ impl<'cfg> RegistryData for LocalRegistry<'cfg> { Ok(()) } - fn download(&mut self, pkg: &PackageId, checksum: &str) -> CargoResult { + fn download(&mut self, pkg: &PackageId, checksum: &str) -> CargoResult { let crate_file = format!("{}-{}.crate", pkg.name(), pkg.version()); let mut crate_file = self.root.open_ro(&crate_file, self.config, "crate file")?; @@ -78,7 +77,7 @@ impl<'cfg> RegistryData for LocalRegistry<'cfg> { // checksum below as it is in theory already verified. let dst = format!("{}-{}", pkg.name(), pkg.version()); if self.src_path.join(dst).into_path_unlocked().exists() { - return Ok(crate_file); + return Ok(MaybeLock::Ready(crate_file)); } self.config.shell().status("Unpacking", pkg)?; @@ -102,6 +101,12 @@ impl<'cfg> RegistryData for LocalRegistry<'cfg> { crate_file.seek(SeekFrom::Start(0))?; - Ok(crate_file) + Ok(MaybeLock::Ready(crate_file)) + } + + fn finish_download(&mut self, _pkg: &PackageId, _checksum: &str, _data: &[u8]) + -> CargoResult + { + panic!("this source doesn't download") } } diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index fa36a0c0ce9..d53dcd1080a 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -170,6 +170,7 @@ use serde_json; use tar::Archive; use core::dependency::{Dependency, Kind}; +use core::source::MaybePackage; use core::{Package, PackageId, Source, SourceId, Summary}; use sources::PathSource; use util::errors::CargoResultExt; @@ -347,13 +348,20 @@ pub trait RegistryData { ) -> CargoResult<()>; fn config(&mut self) -> CargoResult>; fn update_index(&mut self) -> CargoResult<()>; - fn download(&mut self, pkg: &PackageId, checksum: &str) -> CargoResult; + fn download(&mut self, pkg: &PackageId, checksum: &str) -> CargoResult; + fn finish_download(&mut self, pkg: &PackageId, checksum: &str, data: &[u8]) + -> CargoResult; fn is_crate_downloaded(&self, _pkg: &PackageId) -> bool { true } } +pub enum MaybeLock { + Ready(FileLock), + Download { url: String, descriptor: String } +} + mod index; mod local; mod remote; @@ -462,6 +470,34 @@ impl<'cfg> RegistrySource<'cfg> { index::RegistryIndex::new(&self.source_id, path, self.config, self.index_locked); Ok(()) } + + fn get_pkg(&mut self, package: &PackageId, path: FileLock) -> CargoResult { + let path = self + .unpack_package(package, &path) + .chain_err(|| internal(format!("failed to unpack package `{}`", package)))?; + let mut src = PathSource::new(&path, &self.source_id, self.config); + src.update()?; + let pkg = match src.download(package)? { + MaybePackage::Ready(pkg) => pkg, + MaybePackage::Download { .. } => unreachable!(), + }; + + // Unfortunately the index and the actual Cargo.toml in the index can + // differ due to historical Cargo bugs. To paper over these we trash the + // *summary* loaded from the Cargo.toml we just downloaded with the one + // we loaded from the index. + let summaries = self + .index + .summaries(package.name().as_str(), &mut *self.ops)?; + let summary = summaries + .iter() + .map(|s| &s.0) + .find(|s| s.package_id() == package) + .expect("summary not found"); + let mut manifest = pkg.manifest().clone(); + manifest.set_summary(summary.clone()); + Ok(Package::new(manifest, pkg.manifest_path())) + } } impl<'cfg> Source for RegistrySource<'cfg> { @@ -526,31 +562,24 @@ impl<'cfg> Source for RegistrySource<'cfg> { Ok(()) } - fn download(&mut self, package: &PackageId) -> CargoResult { + fn download(&mut self, package: &PackageId) -> CargoResult { let hash = self.index.hash(package, &mut *self.ops)?; - let path = self.ops.download(package, &hash)?; - let path = self - .unpack_package(package, &path) - .chain_err(|| internal(format!("failed to unpack package `{}`", package)))?; - let mut src = PathSource::new(&path, &self.source_id, self.config); - src.update()?; - let pkg = src.download(package)?; + match self.ops.download(package, &hash)? { + MaybeLock::Ready(file) => { + self.get_pkg(package, file).map(MaybePackage::Ready) + } + MaybeLock::Download { url, descriptor } => { + Ok(MaybePackage::Download { url, descriptor }) + } + } + } - // Unfortunately the index and the actual Cargo.toml in the index can - // differ due to historical Cargo bugs. To paper over these we trash the - // *summary* loaded from the Cargo.toml we just downloaded with the one - // we loaded from the index. - let summaries = self - .index - .summaries(package.name().as_str(), &mut *self.ops)?; - let summary = summaries - .iter() - .map(|s| &s.0) - .find(|s| s.package_id() == package) - .expect("summary not found"); - let mut manifest = pkg.manifest().clone(); - manifest.set_summary(summary.clone()); - Ok(Package::new(manifest, pkg.manifest_path())) + fn finish_download(&mut self, package: &PackageId, data: Vec) + -> CargoResult + { + let hash = self.index.hash(package, &mut *self.ops)?; + let file = self.ops.finish_download(package, &hash, &data)?; + self.get_pkg(package, file) } fn fingerprint(&self, pkg: &Package) -> CargoResult { diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs index 63b0c3c11ff..854206d0f92 100644 --- a/src/cargo/sources/registry/remote.rs +++ b/src/cargo/sources/registry/remote.rs @@ -14,10 +14,10 @@ use lazycell::LazyCell; use core::{PackageId, SourceId}; use sources::git; use sources::registry::{RegistryConfig, RegistryData, CRATE_TEMPLATE, INDEX_LOCK, VERSION_TEMPLATE}; -use util::network; +use sources::registry::MaybeLock; use util::{FileLock, Filesystem}; -use util::{Config, Progress, Sha256, ToUrl}; -use util::errors::{CargoResult, CargoResultExt, HttpNot200}; +use util::{Config, Sha256}; +use util::errors::{CargoResult, CargoResultExt}; pub struct RemoteRegistry<'cfg> { index_path: Filesystem, @@ -122,6 +122,10 @@ impl<'cfg> RemoteRegistry<'cfg> { *self.tree.borrow_mut() = Some(tree); Ok(Ref::map(self.tree.borrow(), |s| s.as_ref().unwrap())) } + + fn filename(&self, pkg: &PackageId) -> String { + format!("{}-{}.crate", pkg.name(), pkg.version()) + } } impl<'cfg> RegistryData for RemoteRegistry<'cfg> { @@ -206,9 +210,8 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { Ok(()) } - fn download(&mut self, pkg: &PackageId, checksum: &str) -> CargoResult { - let filename = format!("{}-{}.crate", pkg.name(), pkg.version()); - let path = Path::new(&filename); + fn download(&mut self, pkg: &PackageId, _checksum: &str) -> CargoResult { + let filename = self.filename(pkg); // Attempt to open an read-only copy first to avoid an exclusive write // lock and also work with read-only filesystems. Note that we check the @@ -216,18 +219,12 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { // // If this fails then we fall through to the exclusive path where we may // have to redownload the file. - if let Ok(dst) = self.cache_path.open_ro(path, self.config, &filename) { + if let Ok(dst) = self.cache_path.open_ro(&filename, self.config, &filename) { let meta = dst.file().metadata()?; if meta.len() > 0 { - return Ok(dst); + return Ok(MaybeLock::Ready(dst)); } } - let mut dst = self.cache_path.open_rw(path, self.config, &filename)?; - let meta = dst.file().metadata()?; - if meta.len() > 0 { - return Ok(dst); - } - self.config.shell().status("Downloading", pkg)?; let config = self.config()?.unwrap(); let mut url = config.dl.clone(); @@ -235,56 +232,29 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { write!(url, "/{}/{}/download", CRATE_TEMPLATE, VERSION_TEMPLATE).unwrap(); } let url = url.replace(CRATE_TEMPLATE, &*pkg.name()) - .replace(VERSION_TEMPLATE, &pkg.version().to_string()) - .to_url()?; + .replace(VERSION_TEMPLATE, &pkg.version().to_string()); - // TODO: don't download into memory, but ensure that if we ctrl-c a - // download we should resume either from the start or the middle - // on the next time - let url = url.to_string(); - let mut handle = self.config.http()?.borrow_mut(); - handle.get(true)?; - handle.url(&url)?; - handle.follow_location(true)?; - let mut state = Sha256::new(); - let mut body = Vec::new(); - network::with_retry(self.config, || { - state = Sha256::new(); - body = Vec::new(); - let mut pb = Progress::new("Fetch", self.config); - { - handle.progress(true)?; - let mut handle = handle.transfer(); - handle.progress_function(|dl_total, dl_cur, _, _| { - pb.tick(dl_cur as usize, dl_total as usize).is_ok() - })?; - handle.write_function(|buf| { - state.update(buf); - body.extend_from_slice(buf); - Ok(buf.len()) - })?; - handle.perform().chain_err(|| { - format!("failed to download from `{}`", url) - })?; - } - let code = handle.response_code()?; - if code != 200 && code != 0 { - let url = handle.effective_url()?.unwrap_or(&url); - Err(HttpNot200 { - code, - url: url.to_string(), - }.into()) - } else { - Ok(()) - } - })?; + Ok(MaybeLock::Download { url, descriptor: pkg.to_string() }) + } + fn finish_download(&mut self, pkg: &PackageId, checksum: &str, data: &[u8]) + -> CargoResult + { // Verify what we just downloaded + let mut state = Sha256::new(); + state.update(data); if hex::encode(state.finish()) != checksum { bail!("failed to verify the checksum of `{}`", pkg) } - dst.write_all(&body)?; + let filename = self.filename(pkg); + let mut dst = self.cache_path.open_rw(&filename, self.config, &filename)?; + let meta = dst.file().metadata()?; + if meta.len() > 0 { + return Ok(dst); + } + + dst.write_all(data)?; dst.seek(SeekFrom::Start(0))?; Ok(dst) } diff --git a/src/cargo/sources/replaced.rs b/src/cargo/sources/replaced.rs index 451995b2c7c..3575f67ca45 100644 --- a/src/cargo/sources/replaced.rs +++ b/src/cargo/sources/replaced.rs @@ -1,4 +1,5 @@ use core::{Dependency, Package, PackageId, Source, SourceId, Summary}; +use core::source::MaybePackage; use util::errors::{CargoResult, CargoResultExt}; pub struct ReplacedSource<'cfg> { @@ -71,11 +72,26 @@ impl<'cfg> Source for ReplacedSource<'cfg> { Ok(()) } - fn download(&mut self, id: &PackageId) -> CargoResult { + fn download(&mut self, id: &PackageId) -> CargoResult { let id = id.with_source_id(&self.replace_with); let pkg = self.inner .download(&id) .chain_err(|| format!("failed to download replaced source {}", self.to_replace))?; + Ok(match pkg { + MaybePackage::Ready(pkg) => { + MaybePackage::Ready(pkg.map_source(&self.replace_with, &self.to_replace)) + } + other @ MaybePackage::Download { .. } => other, + }) + } + + fn finish_download(&mut self, id: &PackageId, data: Vec) + -> CargoResult + { + let id = id.with_source_id(&self.replace_with); + let pkg = self.inner + .finish_download(&id, data) + .chain_err(|| format!("failed to download replaced source {}", self.to_replace))?; Ok(pkg.map_source(&self.replace_with, &self.to_replace)) } diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 0c4ce1be92c..4d9fd5d8882 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -266,10 +266,7 @@ fn bad_cksum() { "\ [UPDATING] [..] index [DOWNLOADING] bad-cksum [..] -[ERROR] unable to get packages from source - -Caused by: - failed to download replaced source registry `https://[..]` +[ERROR] failed to download replaced source registry `https://[..]` Caused by: failed to verify the checksum of `bad-cksum v0.0.1 (registry `[ROOT][..]`)` @@ -1662,10 +1659,7 @@ fn bad_and_or_malicious_packages_rejected() { "\ [UPDATING] [..] [DOWNLOADING] [..] -error: unable to get packages from source - -Caused by: - failed to download [..] +error: failed to download [..] Caused by: failed to unpack [..] From 7b1a0dc571ba3b5cd281368338cf832ff785de95 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 10 Sep 2018 13:44:11 -0700 Subject: [PATCH 02/15] Rename PackageSet::get to `get_one` This commit removes the old `PackageSet::get` method (used for downloading crates) to `PackageSet::get_one`. The new method explicitly indicates that only one package is being fetched. Additionally this commit also adds support for an API that allows supporting downloading multiple packages in parallel. Existing callers are all updated to use the parallel API where appropriate except for `BuildContext`, which will be updated in the next commit. Also note that no parallel downloads are done yet, they're still synchronously performed one at a time. A later commit will add support for truly downloading in parallel. --- src/cargo/core/compiler/build_context/mod.rs | 2 +- src/cargo/core/package.rs | 52 +++++++++++++++++--- src/cargo/ops/cargo_clean.rs | 2 +- src/cargo/ops/cargo_compile.rs | 25 ++++++---- src/cargo/ops/cargo_doc.rs | 15 +++--- src/cargo/ops/cargo_fetch.rs | 6 ++- src/cargo/ops/cargo_install.rs | 2 +- src/cargo/ops/cargo_output_metadata.rs | 44 ++++++++++------- 8 files changed, 101 insertions(+), 47 deletions(-) diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index 465dd1095b5..7da3a09f38b 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -109,7 +109,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { /// Gets a package for the given package id. pub fn get_package(&self, id: &PackageId) -> CargoResult<&'a Package> { - self.packages.get(id) + self.packages.get_one(id) } /// Get the user-specified linker for a particular host or target diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 9536f117beb..ae6ead8eb42 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -242,6 +242,12 @@ pub struct PackageSet<'cfg> { packages: HashMap>, sources: RefCell>, config: &'cfg Config, + downloads: RefCell, +} + +#[derive(Default, Debug)] +struct Downloads { + pending: Vec<(String, String, PackageId)>, } impl<'cfg> PackageSet<'cfg> { @@ -257,6 +263,7 @@ impl<'cfg> PackageSet<'cfg> { .collect(), sources: RefCell::new(sources), config, + downloads: Default::default(), } } @@ -264,12 +271,13 @@ impl<'cfg> PackageSet<'cfg> { Box::new(self.packages.keys()) } - pub fn get(&self, id: &PackageId) -> CargoResult<&Package> { + pub fn start_download(&self, id: &PackageId) -> CargoResult> { + let mut downloads = self.downloads.borrow_mut(); let slot = self.packages .get(id) .ok_or_else(|| internal(format!("couldn't find `{}` in package set", id)))?; if let Some(pkg) = slot.borrow() { - return Ok(pkg); + return Ok(Some(pkg)); } let mut sources = self.sources.borrow_mut(); let source = sources @@ -278,18 +286,46 @@ impl<'cfg> PackageSet<'cfg> { let pkg = source .download(id) .chain_err(|| format_err!("unable to get packages from source"))?; - let pkg = match pkg { - MaybePackage::Ready(pkg) => pkg, + match pkg { + MaybePackage::Ready(pkg) => { + assert!(slot.fill(pkg).is_ok()); + Ok(Some(slot.borrow().unwrap())) + } MaybePackage::Download { url, descriptor } => { - self.config.shell().status("Downloading", descriptor)?; - let data = download(self.config, &url)?; - source.finish_download(id, data)? + downloads.pending.push((url, descriptor, id.clone())); + Ok(None) } - }; + } + } + + pub fn remaining_downloads(&self) -> usize { + let downloads = self.downloads.borrow(); + downloads.pending.len() + } + + pub fn wait_for_download(&self) -> CargoResult<&Package> { + let mut downloads = self.downloads.borrow_mut(); + let (url, descriptor, id) = downloads.pending.pop().unwrap(); + self.config.shell().status("Downloading", descriptor)?; + let data = download(self.config, &url)?; + let mut sources = self.sources.borrow_mut(); + let source = sources + .get_mut(id.source_id()) + .ok_or_else(|| internal(format!("couldn't find source for `{}`", id)))?; + let pkg = source.finish_download(&id, data)?; + let slot = &self.packages[&id]; assert!(slot.fill(pkg).is_ok()); Ok(slot.borrow().unwrap()) } + pub fn get_one(&self, id: &PackageId) -> CargoResult<&Package> { + assert_eq!(self.remaining_downloads(), 0); + match self.start_download(id)? { + Some(s) => Ok(s), + None => self.wait_for_download(), + } + } + pub fn sources(&self) -> Ref> { self.sources.borrow() } diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index abb3be686a3..bfdf79ea079 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -52,7 +52,7 @@ pub fn clean(ws: &Workspace, opts: &CleanOptions) -> CargoResult<()> { for spec in opts.spec.iter() { // Translate the spec to a Package let pkgid = resolve.query(spec)?; - let pkg = packages.get(pkgid)?; + let pkg = packages.get_one(pkgid)?; // Generate all relevant `Unit` targets for this package for target in pkg.targets() { diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 0c353d9bc8f..ebd3fdb800d 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -243,15 +243,22 @@ pub fn compile_ws<'a>( let resolve = ops::resolve_ws_with_method(ws, source, method, &specs)?; let (packages, resolve_with_overrides) = resolve; - let to_builds = specs - .iter() - .map(|p| { - let pkgid = p.query(resolve_with_overrides.iter())?; - let p = packages.get(pkgid)?; - p.manifest().print_teapot(ws.config()); - Ok(p) - }) - .collect::>>()?; + let mut to_builds = Vec::new(); + for spec in specs.iter() { + let pkgid = spec.query(resolve_with_overrides.iter())?; + to_builds.extend(packages.start_download(pkgid)?); + } + while packages.remaining_downloads() > 0 { + to_builds.push(packages.wait_for_download()?); + } + // The ordering here affects some error messages coming out of cargo, so + // let's be test and CLI friendly by always printing in the same order if + // there's an error. + to_builds.sort_by_key(|p| p.package_id()); + + for pkg in to_builds.iter() { + pkg.manifest().print_teapot(ws.config()); + } let (extra_args, extra_args_name) = match (target_rustc_args, target_rustdoc_args) { (&Some(ref args), _) => (Some(args.clone()), "rustc"), diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs index e12784df9e2..4873ec40983 100644 --- a/src/cargo/ops/cargo_doc.rs +++ b/src/cargo/ops/cargo_doc.rs @@ -31,13 +31,14 @@ pub fn doc(ws: &Workspace, options: &DocOptions) -> CargoResult<()> { )?; let (packages, resolve_with_overrides) = resolve; - let pkgs = specs - .iter() - .map(|p| { - let pkgid = p.query(resolve_with_overrides.iter())?; - packages.get(pkgid) - }) - .collect::>>()?; + let mut pkgs = Vec::new(); + for spec in specs.iter() { + let pkgid = spec.query(resolve_with_overrides.iter())?; + pkgs.extend(packages.start_download(pkgid)?); + } + while packages.remaining_downloads() > 0 { + pkgs.push(packages.wait_for_download()?); + } let mut lib_names = HashMap::new(); let mut bin_names = HashMap::new(); diff --git a/src/cargo/ops/cargo_fetch.rs b/src/cargo/ops/cargo_fetch.rs index ce9a788bc89..08e926d421a 100644 --- a/src/cargo/ops/cargo_fetch.rs +++ b/src/cargo/ops/cargo_fetch.rs @@ -33,7 +33,7 @@ pub fn fetch<'a>( continue; } - packages.get(id)?; + packages.start_download(id)?; let deps = resolve.deps(id) .filter(|&(_id, deps)| { deps.iter() @@ -59,5 +59,9 @@ pub fn fetch<'a>( } } + while packages.remaining_downloads() > 0 { + packages.wait_for_download()?; + } + Ok((resolve, packages)) } diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 0d75bd5ccf8..cc89bb9a6dc 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -519,7 +519,7 @@ where let mut map = SourceMap::new(); map.insert(Box::new(&mut source)); PackageSet::new(&[pkgid.clone()], map, config) - .get(&pkgid)? + .get_one(&pkgid)? .clone() }; Ok((pkg, Box::new(source))) diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index fe2443d403e..466579d636d 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -1,7 +1,9 @@ +use std::collections::HashMap; + use serde::ser; use core::resolver::Resolve; -use core::{Package, PackageId, Workspace, PackageSet}; +use core::{Package, PackageId, Workspace}; use ops::{self, Packages}; use util::CargoResult; @@ -18,7 +20,7 @@ pub struct OutputMetadataOptions { /// Loads the manifest, resolves the dependencies of the project to the concrete /// used versions - considering overrides - and writes all dependencies in a JSON /// format to stdout. -pub fn output_metadata<'a>(ws: &'a Workspace, opt: &OutputMetadataOptions) -> CargoResult> { +pub fn output_metadata(ws: &Workspace, opt: &OutputMetadataOptions) -> CargoResult { if opt.version != VERSION { bail!( "metadata version {} not supported, only {} is currently supported", @@ -33,7 +35,7 @@ pub fn output_metadata<'a>(ws: &'a Workspace, opt: &OutputMetadataOptions) -> Ca } } -fn metadata_no_deps<'a>(ws: &'a Workspace, _opt: &OutputMetadataOptions) -> CargoResult> { +fn metadata_no_deps(ws: &Workspace, _opt: &OutputMetadataOptions) -> CargoResult { Ok(ExportInfo { packages: ws.members().cloned().collect(), workspace_members: ws.members().map(|pkg| pkg.package_id().clone()).collect(), @@ -44,9 +46,9 @@ fn metadata_no_deps<'a>(ws: &'a Workspace, _opt: &OutputMetadataOptions) -> Carg }) } -fn metadata_full<'a>(ws: &'a Workspace, opt: &OutputMetadataOptions) -> CargoResult> { +fn metadata_full(ws: &Workspace, opt: &OutputMetadataOptions) -> CargoResult { let specs = Packages::All.to_package_id_specs(ws)?; - let deps = ops::resolve_ws_precisely( + let (package_set, resolve) = ops::resolve_ws_precisely( ws, None, &opt.features, @@ -54,18 +56,22 @@ fn metadata_full<'a>(ws: &'a Workspace, opt: &OutputMetadataOptions) -> CargoRes opt.no_default_features, &specs, )?; - let (package_set, resolve) = deps; - - let packages = package_set - .package_ids() - .map(|i| package_set.get(i).map(|p| p.clone())) - .collect::>>()?; + let mut packages = HashMap::new(); + for id in package_set.package_ids() { + if let Some(pkg) = package_set.start_download(id)? { + packages.insert(id.clone(), pkg.clone()); + } + } + while package_set.remaining_downloads() > 0 { + let pkg = package_set.wait_for_download()?; + packages.insert(pkg.package_id().clone(), pkg.clone()); + } Ok(ExportInfo { - packages, + packages: packages.values().map(|p| (*p).clone()).collect(), workspace_members: ws.members().map(|pkg| pkg.package_id().clone()).collect(), resolve: Some(MetadataResolve { - resolve: (package_set, resolve), + resolve: (packages, resolve), root: ws.current_opt().map(|pkg| pkg.package_id().clone()), }), target_directory: ws.target_dir().display().to_string(), @@ -75,10 +81,10 @@ fn metadata_full<'a>(ws: &'a Workspace, opt: &OutputMetadataOptions) -> CargoRes } #[derive(Serialize)] -pub struct ExportInfo<'a> { +pub struct ExportInfo { packages: Vec, workspace_members: Vec, - resolve: Option>, + resolve: Option, target_directory: String, version: u32, workspace_root: String, @@ -88,13 +94,13 @@ pub struct ExportInfo<'a> { /// The one from lockfile does not fit because it uses a non-standard /// format for `PackageId`s #[derive(Serialize)] -struct MetadataResolve<'a> { +struct MetadataResolve { #[serde(rename = "nodes", serialize_with = "serialize_resolve")] - resolve: (PackageSet<'a>, Resolve), + resolve: (HashMap, Resolve), root: Option, } -fn serialize_resolve((package_set, resolve): &(PackageSet, Resolve), s: S) -> Result +fn serialize_resolve((packages, resolve): &(HashMap, Resolve), s: S) -> Result where S: ser::Serializer, { @@ -119,7 +125,7 @@ where dependencies: resolve.deps(id).map(|(pkg, _deps)| pkg).collect(), deps: resolve.deps(id) .map(|(pkg, _deps)| { - let name = package_set.get(pkg).ok() + let name = packages.get(pkg) .and_then(|pkg| pkg.targets().iter().find(|t| t.is_lib())) .and_then(|lib_target| { resolve.extern_crate_name(id, pkg, lib_target).ok() From 44a7ee7db8621c33b4ff519be98343f95dbc5ba6 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 10 Sep 2018 16:50:29 -0700 Subject: [PATCH 03/15] Refactor `build_unit_dependencies` for parallel downloads This commit refactors the `build_unit_dependencies` function for leveraging parallel downloads. This function is the primary location that crates are download as part of `cargo build`, which is one of the most critical locations to take advantage of parallel downloads! The problem here is somewhat unusal in that we don't want to download the entire crate graph but rather only the precise slice that we're compiling. This means that we're letting the calculation of unit dependencies entirely drive the decision of what crates to download. While we're calculating dependencies, though, we may need to download more crates! The strategy implemented here is to attempt to build the unit dependency graph. Any missing packages are skipped during this construction and enqueued for download. If any package was enqueued, we throw away the entire graph, wait for a download to finish, and then attempt to construct the unit graph again. The hope here is that we'll be able to incrementally make progress and incrementally add more and more crates to be downloaded in parallel. The worry is that this is a relatively inefficient strategy as we could reconstruct the unit dependency graph N times (where you have N total transitive dependencies). To help alleviate this concern wait for only a small handful of crates to actively be downloading before we continue to try to recreate the dependency graph. It's hoped that this will strike a good balance between ensuring we always have a full pipeline of downloads without re-downloading too much. --- src/cargo/core/compiler/build_context/mod.rs | 19 +- src/cargo/core/compiler/context/mod.rs | 30 +- .../compiler/context/unit_dependencies.rs | 256 ++++++++++++------ src/cargo/core/compiler/job_queue.rs | 2 +- 4 files changed, 204 insertions(+), 103 deletions(-) diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index 7da3a09f38b..358751c7f07 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -5,7 +5,7 @@ use std::str; use core::profiles::Profiles; use core::{Dependency, Workspace}; -use core::{Package, PackageId, PackageSet, Resolve}; +use core::{PackageId, PackageSet, Resolve}; use util::errors::CargoResult; use util::{profile, Cfg, CfgExpr, Config, Rustc}; @@ -107,11 +107,6 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { platform.matches(name, info.cfg()) } - /// Gets a package for the given package id. - pub fn get_package(&self, id: &PackageId) -> CargoResult<&'a Package> { - self.packages.get_one(id) - } - /// Get the user-specified linker for a particular host or target pub fn linker(&self, kind: Kind) -> Option<&Path> { self.target_config(kind).linker.as_ref().map(|s| s.as_ref()) @@ -198,18 +193,6 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { pub fn extra_args_for(&self, unit: &Unit<'a>) -> Option<&Vec> { self.extra_compiler_args.get(unit) } - - /// Return the list of filenames read by cargo to generate the BuildContext - /// (all Cargo.toml, etc). - pub fn inputs(&self) -> CargoResult> { - let mut inputs = Vec::new(); - for id in self.packages.package_ids() { - let pkg = self.get_package(id)?; - inputs.push(pkg.manifest_path().to_path_buf()); - } - inputs.sort(); - Ok(inputs) - } } /// Information required to build for a target diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 197356b2424..d4932c60d01 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -99,6 +99,7 @@ pub struct Context<'a, 'cfg: 'a> { primary_packages: HashSet<&'a PackageId>, unit_dependencies: HashMap, Vec>>, files: Option>, + package_cache: HashMap<&'a PackageId, &'a Package>, } impl<'a, 'cfg> Context<'a, 'cfg> { @@ -133,6 +134,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { primary_packages: HashSet::new(), unit_dependencies: HashMap::new(), files: None, + package_cache: HashMap::new(), }) } @@ -165,7 +167,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { queue.execute(&mut self, &mut plan)?; if build_plan { - plan.set_inputs(self.bcx.inputs()?); + plan.set_inputs(self.inputs()?); plan.output_plan(); } @@ -326,7 +328,12 @@ impl<'a, 'cfg> Context<'a, 'cfg> { }; self.primary_packages.extend(units.iter().map(|u| u.pkg.package_id())); - build_unit_dependencies(units, self.bcx, &mut self.unit_dependencies)?; + build_unit_dependencies( + units, + self.bcx, + &mut self.unit_dependencies, + &mut self.package_cache, + )?; self.build_used_in_plugin_map(units)?; let files = CompilationFiles::new( units, @@ -495,6 +502,25 @@ impl<'a, 'cfg> Context<'a, 'cfg> { pub fn is_primary_package(&self, unit: &Unit<'a>) -> bool { self.primary_packages.contains(unit.pkg.package_id()) } + + /// Gets a package for the given package id. + pub fn get_package(&self, id: &PackageId) -> CargoResult<&'a Package> { + self.package_cache.get(id) + .cloned() + .ok_or_else(|| format_err!("failed to find {}", id)) + } + + /// Return the list of filenames read by cargo to generate the BuildContext + /// (all Cargo.toml, etc). + pub fn inputs(&self) -> CargoResult> { + let mut inputs = Vec::new(); + for id in self.bcx.packages.package_ids() { + let pkg = self.get_package(id)?; + inputs.push(pkg.manifest_path().to_path_buf()); + } + inputs.sort(); + Ok(inputs) + } } #[derive(Default)] diff --git a/src/cargo/core/compiler/context/unit_dependencies.rs b/src/cargo/core/compiler/context/unit_dependencies.rs index e8af68628c3..f8ffcb1ce57 100644 --- a/src/cargo/core/compiler/context/unit_dependencies.rs +++ b/src/cargo/core/compiler/context/unit_dependencies.rs @@ -15,46 +15,72 @@ //! (for example, with and without tests), so we actually build a dependency //! graph of `Unit`s, which capture these properties. +use std::cell::{RefCell, Cell}; use std::collections::{HashMap, HashSet}; use CargoResult; use core::dependency::Kind as DepKind; use core::profiles::ProfileFor; -use core::{Package, Target}; +use core::{Package, Target, PackageId}; use super::{BuildContext, CompileMode, Kind, Unit}; +struct State<'a: 'tmp, 'cfg: 'a, 'tmp> { + bcx: &'tmp BuildContext<'a, 'cfg>, + deps: &'tmp mut HashMap, Vec>>, + pkgs: RefCell<&'tmp mut HashMap<&'a PackageId, &'a Package>>, + waiting_on_downloads: Cell, +} + pub fn build_unit_dependencies<'a, 'cfg>( roots: &[Unit<'a>], bcx: &BuildContext<'a, 'cfg>, deps: &mut HashMap, Vec>>, + pkgs: &mut HashMap<&'a PackageId, &'a Package>, ) -> CargoResult<()> { assert!(deps.is_empty(), "can only build unit deps once"); - for unit in roots.iter() { - // Dependencies of tests/benches should not have `panic` set. - // We check the global test mode to see if we are running in `cargo - // test` in which case we ensure all dependencies have `panic` - // cleared, and avoid building the lib thrice (once with `panic`, once - // without, once for --test). In particular, the lib included for - // doctests and examples are `Build` mode here. - let profile_for = if unit.mode.is_any_test() || bcx.build_config.test() { - ProfileFor::TestDependency + let mut state = State { + bcx, + deps, + pkgs: RefCell::new(pkgs), + waiting_on_downloads: Cell::new(false), + }; + + loop { + for unit in roots.iter() { + state.get(unit.pkg.package_id())?; + + // Dependencies of tests/benches should not have `panic` set. + // We check the global test mode to see if we are running in `cargo + // test` in which case we ensure all dependencies have `panic` + // cleared, and avoid building the lib thrice (once with `panic`, once + // without, once for --test). In particular, the lib included for + // doctests and examples are `Build` mode here. + let profile_for = if unit.mode.is_any_test() || bcx.build_config.test() { + ProfileFor::TestDependency + } else { + ProfileFor::Any + }; + deps_of(unit, &mut state, profile_for)?; + } + + if state.waiting_on_downloads.replace(false) { + state.finish_some_downloads()?; + state.deps.clear(); } else { - ProfileFor::Any - }; - deps_of(unit, bcx, deps, profile_for)?; + break + } } - trace!("ALL UNIT DEPENDENCIES {:#?}", deps); + trace!("ALL UNIT DEPENDENCIES {:#?}", state.deps); - connect_run_custom_build_deps(bcx, deps); + connect_run_custom_build_deps(&mut state); Ok(()) } -fn deps_of<'a, 'cfg>( +fn deps_of<'a, 'cfg, 'tmp>( unit: &Unit<'a>, - bcx: &BuildContext<'a, 'cfg>, - deps: &mut HashMap, Vec>>, + state: &mut State<'a, 'cfg, 'tmp>, profile_for: ProfileFor, ) -> CargoResult<()> { // Currently the `deps` map does not include `profile_for`. This should @@ -63,12 +89,12 @@ fn deps_of<'a, 'cfg>( // `TestDependency`. `CustomBuild` should also be fine since if the // requested unit's settings are the same as `Any`, `CustomBuild` can't // affect anything else in the hierarchy. - if !deps.contains_key(unit) { - let unit_deps = compute_deps(unit, bcx, profile_for)?; + if !state.deps.contains_key(unit) { + let unit_deps = compute_deps(unit, state, profile_for)?; let to_insert: Vec<_> = unit_deps.iter().map(|&(unit, _)| unit).collect(); - deps.insert(*unit, to_insert); + state.deps.insert(*unit, to_insert); for (unit, profile_for) in unit_deps { - deps_of(&unit, bcx, deps, profile_for)?; + deps_of(&unit, state, profile_for)?; } } Ok(()) @@ -78,63 +104,82 @@ fn deps_of<'a, 'cfg>( /// for that package. /// This returns a vec of `(Unit, ProfileFor)` pairs. The `ProfileFor` /// is the profile type that should be used for dependencies of the unit. -fn compute_deps<'a, 'cfg>( +fn compute_deps<'a, 'cfg, 'tmp>( unit: &Unit<'a>, - bcx: &BuildContext<'a, 'cfg>, + state: &mut State<'a, 'cfg, 'tmp>, profile_for: ProfileFor, ) -> CargoResult, ProfileFor)>> { if unit.mode.is_run_custom_build() { - return compute_deps_custom_build(unit, bcx); + return compute_deps_custom_build(unit, state.bcx); } else if unit.mode.is_doc() && !unit.mode.is_any_test() { // Note: This does not include Doctest. - return compute_deps_doc(unit, bcx); + return compute_deps_doc(unit, state); } + let bcx = state.bcx; let id = unit.pkg.package_id(); - let deps = bcx.resolve.deps(id); - let mut ret = deps.filter(|&(_id, deps)| { - assert!(!deps.is_empty()); - deps.iter().any(|dep| { - // If this target is a build command, then we only want build - // dependencies, otherwise we want everything *other than* build - // dependencies. - if unit.target.is_custom_build() != dep.is_build() { - return false; - } + let deps = bcx.resolve.deps(id) + .filter(|&(_id, deps)| { + assert!(!deps.is_empty()); + deps.iter().any(|dep| { + // If this target is a build command, then we only want build + // dependencies, otherwise we want everything *other than* build + // dependencies. + if unit.target.is_custom_build() != dep.is_build() { + return false; + } - // If this dependency is *not* a transitive dependency, then it - // only applies to test/example targets - if !dep.is_transitive() && !unit.target.is_test() && !unit.target.is_example() - && !unit.mode.is_any_test() - { - return false; - } + // If this dependency is *not* a transitive dependency, then it + // only applies to test/example targets + if !dep.is_transitive() && + !unit.target.is_test() && + !unit.target.is_example() && + !unit.mode.is_any_test() + { + return false; + } - // If this dependency is only available for certain platforms, - // make sure we're only enabling it for that platform. - if !bcx.dep_platform_activated(dep, unit.kind) { - return false; - } + // If this dependency is only available for certain platforms, + // make sure we're only enabling it for that platform. + if !bcx.dep_platform_activated(dep, unit.kind) { + return false; + } - // If the dependency is optional, then we're only activating it - // if the corresponding feature was activated - if dep.is_optional() && !bcx.resolve.features(id).contains(&*dep.name_in_toml()) { - return false; - } + // If the dependency is optional, then we're only activating it + // if the corresponding feature was activated + if dep.is_optional() && + !bcx.resolve.features(id).contains(&*dep.name_in_toml()) + { + return false; + } - // If we've gotten past all that, then this dependency is - // actually used! - true - }) - }).filter_map(|(id, _)| match bcx.get_package(id) { - Ok(pkg) => pkg.targets().iter().find(|t| t.is_lib()).map(|t| { - let mode = check_or_build_mode(unit.mode, t); - let unit = new_unit(bcx, pkg, t, profile_for, unit.kind.for_target(t), mode); - Ok((unit, profile_for)) - }), - Err(e) => Some(Err(e)), - }) - .collect::>>()?; + // If we've gotten past all that, then this dependency is + // actually used! + true + }) + }); + + let mut ret = Vec::new(); + for (id, _) in deps { + let pkg = match state.get(id)? { + Some(pkg) => pkg, + None => continue, + }; + let lib = match pkg.targets().iter().find(|t| t.is_lib()) { + Some(t) => t, + None => continue, + }; + let mode = check_or_build_mode(unit.mode, lib); + let unit = new_unit( + bcx, + pkg, + lib, + profile_for, + unit.kind.for_target(lib), + mode, + ); + ret.push((unit, profile_for)); + } // If this target is a build script, then what we've collected so far is // all we need. If this isn't a build script, then it depends on the @@ -221,10 +266,11 @@ fn compute_deps_custom_build<'a, 'cfg>( } /// Returns the dependencies necessary to document a package -fn compute_deps_doc<'a, 'cfg>( +fn compute_deps_doc<'a, 'cfg, 'tmp>( unit: &Unit<'a>, - bcx: &BuildContext<'a, 'cfg>, + state: &mut State<'a, 'cfg, 'tmp>, ) -> CargoResult, ProfileFor)>> { + let bcx = state.bcx; let deps = bcx.resolve .deps(unit.pkg.package_id()) .filter(|&(_id, deps)| { @@ -232,15 +278,17 @@ fn compute_deps_doc<'a, 'cfg>( DepKind::Normal => bcx.dep_platform_activated(dep, unit.kind), _ => false, }) - }) - .map(|(id, _deps)| bcx.get_package(id)); + }); // To document a library, we depend on dependencies actually being // built. If we're documenting *all* libraries, then we also depend on // the documentation of the library being built. let mut ret = Vec::new(); - for dep in deps { - let dep = dep?; + for (id, _deps) in deps { + let dep = match state.get(id)? { + Some(dep) => dep, + None => continue, + }; let lib = match dep.targets().iter().find(|t| t.is_lib()) { Some(lib) => lib, None => continue, @@ -288,7 +336,14 @@ fn maybe_lib<'a>( ) -> Option<(Unit<'a>, ProfileFor)> { unit.pkg.targets().iter().find(|t| t.linkable()).map(|t| { let mode = check_or_build_mode(unit.mode, t); - let unit = new_unit(bcx, unit.pkg, t, profile_for, unit.kind.for_target(t), mode); + let unit = new_unit( + bcx, + unit.pkg, + t, + profile_for, + unit.kind.for_target(t), + mode, + ); (unit, profile_for) }) } @@ -373,10 +428,7 @@ fn new_unit<'a>( /// /// Here we take the entire `deps` map and add more dependencies from execution /// of one build script to execution of another build script. -fn connect_run_custom_build_deps<'a>( - bcx: &BuildContext, - deps: &mut HashMap, Vec>>, -) { +fn connect_run_custom_build_deps(state: &mut State) { let mut new_deps = Vec::new(); { @@ -386,7 +438,7 @@ fn connect_run_custom_build_deps<'a>( // have the build script as the key and the library would be in the // value's set. let mut reverse_deps = HashMap::new(); - for (unit, deps) in deps.iter() { + for (unit, deps) in state.deps.iter() { for dep in deps { if dep.mode == CompileMode::RunCustomBuild { reverse_deps.entry(dep) @@ -405,7 +457,7 @@ fn connect_run_custom_build_deps<'a>( // `links`, then we depend on that package's build script! Here we use // `dep_build_script` to manufacture an appropriate build script unit to // depend on. - for unit in deps.keys().filter(|k| k.mode == CompileMode::RunCustomBuild) { + for unit in state.deps.keys().filter(|k| k.mode == CompileMode::RunCustomBuild) { let reverse_deps = match reverse_deps.get(unit) { Some(set) => set, None => continue, @@ -413,13 +465,13 @@ fn connect_run_custom_build_deps<'a>( let to_add = reverse_deps .iter() - .flat_map(|reverse_dep| deps[reverse_dep].iter()) + .flat_map(|reverse_dep| state.deps[reverse_dep].iter()) .filter(|other| { other.pkg != unit.pkg && other.target.linkable() && other.pkg.manifest().links().is_some() }) - .filter_map(|other| dep_build_script(other, bcx).map(|p| p.0)) + .filter_map(|other| dep_build_script(other, state.bcx).map(|p| p.0)) .collect::>(); if !to_add.is_empty() { @@ -430,6 +482,46 @@ fn connect_run_custom_build_deps<'a>( // And finally, add in all the missing dependencies! for (unit, new_deps) in new_deps { - deps.get_mut(&unit).unwrap().extend(new_deps); + state.deps.get_mut(&unit).unwrap().extend(new_deps); + } +} + +impl<'a, 'cfg, 'tmp> State<'a, 'cfg, 'tmp> { + fn get(&mut self, id: &'a PackageId) -> CargoResult> { + let mut pkgs = self.pkgs.borrow_mut(); + if let Some(pkg) = pkgs.get(id) { + return Ok(Some(pkg)) + } + if let Some(pkg) = self.bcx.packages.start_download(id)? { + pkgs.insert(id, pkg); + return Ok(Some(pkg)) + } + self.waiting_on_downloads.set(true); + Ok(None) + } + + /// Completes at least one downloading, maybe waiting for more to complete. + /// + /// This function will block the current thread waiting for at least one + /// create to finish downloading. The function may continue to download more + /// crates if it looks like there's a long enough queue of crates to keep + /// downloading. When only a handful of packages remain this function + /// returns, and it's hoped that by returning we'll be able to push more + /// packages to download into the queu. + fn finish_some_downloads(&mut self) -> CargoResult<()> { + assert!(self.bcx.packages.remaining_downloads() > 0); + loop { + let pkg = self.bcx.packages.wait_for_download()?; + self.pkgs.borrow_mut().insert(pkg.package_id(), pkg); + + // Arbitrarily choose that 5 or more packages concurrently download + // is a good enough number to "fill the network pipe". If we have + // less than this let's recompute the whole unit dependency graph + // again and try to find some more packages to download. + if self.bcx.packages.remaining_downloads() < 5 { + break + } + } + Ok(()) } } diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index ba177b0f4b1..c2681c892b6 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -535,7 +535,7 @@ impl<'a> Key<'a> { fn dependencies<'cfg>(&self, cx: &Context<'a, 'cfg>) -> CargoResult>> { let unit = Unit { - pkg: cx.bcx.get_package(self.pkg)?, + pkg: cx.get_package(self.pkg)?, target: self.target, profile: self.profile, kind: self.kind, From 468f243e0ece85397250684a3fe885d9c44cad24 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 10 Sep 2018 17:28:18 -0700 Subject: [PATCH 04/15] Parallelize downloads with HTTP/2 This commit implements parallel downloads using `libcurl` powered by `libnghttp2` over HTTP/2. Using all of the previous refactorings this actually implements usage of `Multi` to download crates in parallel. This achieves some large wins locally, taking download times from 30s to 2s in the best case. The standard output of Cargo is also changed as a result of this commit. It's no longer useful for Cargo to print "Downloading ..." for each crate really as they all start instantaneously. Instead Cargo now no longer prints `Downloading` by default (unless attached to a pipe) and instead only has one progress bar for all downloads. Currently this progress bar is discrete and based on the total number of downloads, no longer specifying how much of one particular download has happened. This provides a less granular view into what Cargo is doing but it's hoped that it looks reasonable from an outside perspective as there's still a progress bar indicating what's happening. --- Cargo.toml | 2 +- .../compiler/context/unit_dependencies.rs | 14 +- src/cargo/core/package.rs | 323 ++++++++++++++---- src/cargo/core/registry.rs | 2 +- src/cargo/ops/cargo_install.rs | 2 +- src/cargo/ops/resolve.rs | 6 +- src/cargo/util/network.rs | 49 ++- src/cargo/util/progress.rs | 4 + 8 files changed, 315 insertions(+), 87 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 47760255b41..147b38e55c5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ atty = "0.2" crates-io = { path = "src/crates-io", version = "0.20" } crossbeam-utils = "0.5" crypto-hash = "0.3.1" -curl = "0.4.13" +curl = { version = "0.4.15", features = ['http2'] } env_logger = "0.5.11" failure = "0.1.2" filetime = "0.2" diff --git a/src/cargo/core/compiler/context/unit_dependencies.rs b/src/cargo/core/compiler/context/unit_dependencies.rs index f8ffcb1ce57..7b40ee0d856 100644 --- a/src/cargo/core/compiler/context/unit_dependencies.rs +++ b/src/cargo/core/compiler/context/unit_dependencies.rs @@ -15,7 +15,7 @@ //! (for example, with and without tests), so we actually build a dependency //! graph of `Unit`s, which capture these properties. -use std::cell::{RefCell, Cell}; +use std::cell::RefCell; use std::collections::{HashMap, HashSet}; use CargoResult; @@ -28,7 +28,7 @@ struct State<'a: 'tmp, 'cfg: 'a, 'tmp> { bcx: &'tmp BuildContext<'a, 'cfg>, deps: &'tmp mut HashMap, Vec>>, pkgs: RefCell<&'tmp mut HashMap<&'a PackageId, &'a Package>>, - waiting_on_downloads: Cell, + waiting_on_download: HashSet<&'a PackageId>, } pub fn build_unit_dependencies<'a, 'cfg>( @@ -43,7 +43,7 @@ pub fn build_unit_dependencies<'a, 'cfg>( bcx, deps, pkgs: RefCell::new(pkgs), - waiting_on_downloads: Cell::new(false), + waiting_on_download: HashSet::new(), }; loop { @@ -64,7 +64,7 @@ pub fn build_unit_dependencies<'a, 'cfg>( deps_of(unit, &mut state, profile_for)?; } - if state.waiting_on_downloads.replace(false) { + if state.waiting_on_download.len() > 0 { state.finish_some_downloads()?; state.deps.clear(); } else { @@ -492,11 +492,14 @@ impl<'a, 'cfg, 'tmp> State<'a, 'cfg, 'tmp> { if let Some(pkg) = pkgs.get(id) { return Ok(Some(pkg)) } + if !self.waiting_on_download.insert(id) { + return Ok(None) + } if let Some(pkg) = self.bcx.packages.start_download(id)? { pkgs.insert(id, pkg); + self.waiting_on_download.remove(id); return Ok(Some(pkg)) } - self.waiting_on_downloads.set(true); Ok(None) } @@ -512,6 +515,7 @@ impl<'a, 'cfg, 'tmp> State<'a, 'cfg, 'tmp> { assert!(self.bcx.packages.remaining_downloads() > 0); loop { let pkg = self.bcx.packages.wait_for_download()?; + self.waiting_on_download.remove(pkg.package_id()); self.pkgs.borrow_mut().insert(pkg.package_id(), pkg); // Arbitrarily choose that 5 or more packages concurrently download diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index ae6ead8eb42..28b3533b904 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -1,20 +1,26 @@ use std::cell::{Ref, RefCell}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt; use std::hash; +use std::mem; use std::path::{Path, PathBuf}; +use std::time::Duration; use semver::Version; use serde::ser; use toml; use lazycell::LazyCell; +use curl::easy::{Easy, HttpVersion}; +use curl::multi::{Multi, EasyHandle}; use core::{Dependency, Manifest, PackageId, SourceId, Target}; use core::{FeatureMap, SourceMap, Summary}; use core::source::MaybePackage; use core::interning::InternedString; -use util::{internal, lev_distance, Config}; -use util::errors::{CargoResult, CargoResultExt}; +use ops; +use util::{internal, lev_distance, Config, Progress, ProgressStyle}; +use util::errors::{CargoResult, CargoResultExt, HttpNot200}; +use util::network::Retry; /// Information about a package that is available somewhere in the file system. /// @@ -237,17 +243,30 @@ impl hash::Hash for Package { } } -#[derive(Debug)] pub struct PackageSet<'cfg> { packages: HashMap>, sources: RefCell>, config: &'cfg Config, - downloads: RefCell, + downloads: RefCell>, } -#[derive(Default, Debug)] -struct Downloads { - pending: Vec<(String, String, PackageId)>, +pub(crate) struct Downloads<'cfg> { + pending: HashMap, + pending_ids: HashSet, + results: Vec<(usize, CargoResult<()>)>, + next: usize, + multi: Multi, + retry: Retry<'cfg>, + progress: Progress<'cfg>, + downloads_finished: usize, +} + +struct Download { + token: usize, + id: PackageId, + data: RefCell>, + url: String, + descriptor: String, } impl<'cfg> PackageSet<'cfg> { @@ -255,30 +274,61 @@ impl<'cfg> PackageSet<'cfg> { package_ids: &[PackageId], sources: SourceMap<'cfg>, config: &'cfg Config, - ) -> PackageSet<'cfg> { - PackageSet { + ) -> CargoResult> { + // We've enabled the `http2` feature of `curl` in Cargo, so treat + // failures here as fatal as it would indicate a build-time problem. + let mut multi = Multi::new(); + multi.pipelining(true, true) + .chain_err(|| "failed to enable multiplexing/pipelining in curl")?; + + Ok(PackageSet { packages: package_ids .iter() .map(|id| (id.clone(), LazyCell::new())) .collect(), sources: RefCell::new(sources), config, - downloads: Default::default(), - } + downloads: RefCell::new(Downloads { + multi, + next: 0, + pending: HashMap::new(), + pending_ids: HashSet::new(), + results: Vec::new(), + retry: Retry::new(config)?, + progress: Progress::with_style( + "Downloading", + ProgressStyle::Ratio, + config, + ), + downloads_finished: 0, + }), + }) } pub fn package_ids<'a>(&'a self) -> Box + 'a> { Box::new(self.packages.keys()) } + /// Starts to download the package for the `id` specified. + /// + /// Returns `None` if the package is queued up for download and will + /// eventually be returned from `wait_for_download`. Returns `Some(pkg)` if + /// the package is ready and doesn't need to be downloaded. pub fn start_download(&self, id: &PackageId) -> CargoResult> { let mut downloads = self.downloads.borrow_mut(); + + // First up see if we've already cached this package, in which case + // there's nothing to do. let slot = self.packages .get(id) .ok_or_else(|| internal(format!("couldn't find `{}` in package set", id)))?; if let Some(pkg) = slot.borrow() { return Ok(Some(pkg)); } + + // Ask the original source fo this `PackageId` for the corresponding + // package. That may immediately come back and tell us that the package + // is ready, or it could tell us that it needs to be downloaded. let mut sources = self.sources.borrow_mut(); let source = sources .get_mut(id.source_id()) @@ -286,34 +336,135 @@ impl<'cfg> PackageSet<'cfg> { let pkg = source .download(id) .chain_err(|| format_err!("unable to get packages from source"))?; - match pkg { + let (url, descriptor) = match pkg { MaybePackage::Ready(pkg) => { + debug!("{} doesn't need a download", id); assert!(slot.fill(pkg).is_ok()); - Ok(Some(slot.borrow().unwrap())) + return Ok(Some(slot.borrow().unwrap())) } - MaybePackage::Download { url, descriptor } => { - downloads.pending.push((url, descriptor, id.clone())); - Ok(None) - } - } + MaybePackage::Download { url, descriptor } => (url, descriptor), + }; + + // Ok we're going to download this crate, so let's set up all our + // internal state and hand off an `Easy` handle to our libcurl `Multi` + // handle. This won't actually start the transfer, but later it'll + // hapen during `wait_for_download` + let token = downloads.next; + downloads.next += 1; + debug!("downloading {} as {}", id, token); + assert!(downloads.pending_ids.insert(id.clone())); + + let mut handle = ops::http_handle(self.config)?; + handle.get(true)?; + handle.url(&url)?; + handle.follow_location(true)?; // follow redirects + + // Enable HTTP/2 to be used as it'll allow true multiplexing which makes + // downloads much faster. Currently Cargo requests the `http2` feature + // of the `curl` crate which means it should always be built in, so + // treat it as a fatal error of http/2 support isn't found. + handle.http_version(HttpVersion::V2) + .chain_err(|| "failed to enable HTTP2, is curl not built right?")?; + + // This is an option to `libcurl` which indicates that if there's a + // bunch of parallel requests to the same host they all wait until the + // pipelining status of the host is known. This means that we won't + // initiate dozens of connections to crates.io, but rather only one. + // Once the main one is opened we realized that pipelining is possible + // and multiplexing is possible with static.crates.io. All in all this + // reduces the number of connections done to a more manageable state. + handle.pipewait(true)?; + + handle.write_function(move |buf| { + debug!("{} - {} bytes of data", token, buf.len()); + tls::with(|downloads| { + downloads.pending[&token].0.data.borrow_mut().extend_from_slice(buf); + }); + Ok(buf.len()) + })?; + + let dl = Download { + token, + data: RefCell::new(Vec::new()), + id: id.clone(), + url, + descriptor, + }; + downloads.enqueue(dl, handle)?; + + Ok(None) } + /// Returns the number of crates that are still downloading pub fn remaining_downloads(&self) -> usize { let downloads = self.downloads.borrow(); downloads.pending.len() } + /// Blocks the current thread waiting for a package to finish downloading. + /// + /// This method will wait for a previously enqueued package to finish + /// downloading and return a reference to it after it's done downloading. + /// + /// # Panics + /// + /// This function will panic if there are no remaining downloads. pub fn wait_for_download(&self) -> CargoResult<&Package> { let mut downloads = self.downloads.borrow_mut(); - let (url, descriptor, id) = downloads.pending.pop().unwrap(); - self.config.shell().status("Downloading", descriptor)?; - let data = download(self.config, &url)?; + let downloads = &mut *downloads; + downloads.tick_now()?; + + let (dl, data) = loop { + assert_eq!(downloads.pending.len(), downloads.pending_ids.len()); + let (token, result) = downloads.wait_for_curl()?; + debug!("{} finished with {:?}", token, result); + + let (mut dl, handle) = downloads.pending.remove(&token) + .expect("got a token for a non-in-progress transfer"); + let data = mem::replace(&mut *dl.data.borrow_mut(), Vec::new()); + let mut handle = downloads.multi.remove(handle)?; + + // Check if this was a spurious error. If it was a spurious error + // then we want to re-enqueue our request for another attempt and + // then we wait for another request to finish. + let ret = { + downloads.retry.try(|| { + result.chain_err(|| { + format!("failed to download from `{}`", dl.url) + })?; + + let code = handle.response_code()?; + if code != 200 && code != 0 { + handle.url(&dl.url)?; + let url = handle.effective_url()?.unwrap_or(&dl.url); + return Err(HttpNot200 { + code, + url: url.to_string(), + }.into()) + } + Ok(()) + })? + }; + if ret.is_some() { + break (dl, data) + } + downloads.enqueue(dl, handle)?; + }; + downloads.pending_ids.remove(&dl.id); + if !downloads.progress.is_enabled() { + self.config.shell().status("Downloading", &dl.descriptor)?; + } + downloads.downloads_finished += 1; + downloads.tick_now()?; + + // Inform the original source that the download is finished which + // should allow us to actually get the package and fill it in now. let mut sources = self.sources.borrow_mut(); let source = sources - .get_mut(id.source_id()) - .ok_or_else(|| internal(format!("couldn't find source for `{}`", id)))?; - let pkg = source.finish_download(&id, data)?; - let slot = &self.packages[&id]; + .get_mut(dl.id.source_id()) + .ok_or_else(|| internal(format!("couldn't find source for `{}`", dl.id)))?; + let pkg = source.finish_download(&dl.id, data)?; + let slot = &self.packages[&dl.id]; assert!(slot.fill(pkg).is_ok()); Ok(slot.borrow().unwrap()) } @@ -331,43 +482,91 @@ impl<'cfg> PackageSet<'cfg> { } } -fn download(config: &Config, url: &str) -> CargoResult> { - use util::network; - use util::Progress; - use util::errors::HttpNot200; - - let mut handle = config.http()?.borrow_mut(); - handle.get(true)?; - handle.url(&url)?; - handle.follow_location(true)?; - let mut body = Vec::new(); - network::with_retry(config, || { - body = Vec::new(); - let mut pb = Progress::new("Fetch", config); - { - handle.progress(true)?; - let mut handle = handle.transfer(); - handle.progress_function(|dl_total, dl_cur, _, _| { - pb.tick(dl_cur as usize, dl_total as usize).is_ok() - })?; - handle.write_function(|buf| { - body.extend_from_slice(buf); - Ok(buf.len()) - })?; - handle.perform().chain_err(|| { - format!("failed to download from `{}`", url) +impl<'cfg> Downloads<'cfg> { + fn enqueue(&mut self, dl: Download, handle: Easy) -> CargoResult<()> { + let mut handle = self.multi.add(handle)?; + handle.set_token(dl.token)?; + self.pending.insert(dl.token, (dl, handle)); + Ok(()) + } + + fn wait_for_curl(&mut self) -> CargoResult<(usize, CargoResult<()>)> { + // This is the main workhorse loop. We use libcurl's portable `wait` + // method to actually perform blocking. This isn't necessarily too + // efficient in terms of fd management, but we should only be juggling + // a few anyway. + // + // Here we start off by asking the `multi` handle to do some work via + // the `perform` method. This will actually do I/O work (nonblocking) + // and attempt to make progress. Afterwards we ask about the `messages` + // contained in the handle which will inform us if anything has finished + // transferring. + // + // If we've got a finished transfer after all that work we break out + // and process the finished transfer at the end. Otherwise we need to + // actually block waiting for I/O to happen, which we achieve with the + // `wait` method on `multi`. + loop { + let n = tls::set(self, || { + self.multi.perform() + .chain_err(|| "failed to perform http requests") })?; + debug!("handles remaining: {}", n); + let results = &mut self.results; + self.multi.messages(|msg| { + let token = msg.token().expect("failed to read token"); + if let Some(result) = msg.result() { + results.push((token, result.map_err(|e| e.into()))); + } else { + debug!("message without a result (?)"); + } + }); + + if let Some(pair) = results.pop() { + break Ok(pair) + } + assert!(self.pending.len() > 0); + self.multi.wait(&mut [], Duration::new(60, 0)) + .chain_err(|| "failed to wait on curl `Multi`")?; + } + } + + fn tick_now(&mut self) -> CargoResult<()> { + self.progress.tick( + self.downloads_finished, + self.downloads_finished + self.pending.len(), + ) + } +} + +mod tls { + use std::cell::Cell; + + use super::Downloads; + + thread_local!(static PTR: Cell = Cell::new(0)); + + pub(crate) fn with(f: impl FnOnce(&Downloads) -> R) -> R { + let ptr = PTR.with(|p| p.get()); + assert!(ptr != 0); + unsafe { + f(&*(ptr as *const Downloads)) } - let code = handle.response_code()?; - if code != 200 && code != 0 { - let url = handle.effective_url()?.unwrap_or(&url); - Err(HttpNot200 { - code, - url: url.to_string(), - }.into()) - } else { - Ok(()) + } + + pub(crate) fn set(dl: &Downloads, f: impl FnOnce() -> R) -> R { + struct Reset<'a, T: Copy + 'a>(&'a Cell, T); + + impl<'a, T: Copy> Drop for Reset<'a, T> { + fn drop(&mut self) { + self.0.set(self.1); + } } - })?; - Ok(body) + + PTR.with(|p| { + let _reset = Reset(p, p.get()); + p.set(dl as *const Downloads as usize); + f() + }) + } } diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index 1c359b39490..86cf40dee9b 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -94,7 +94,7 @@ impl<'cfg> PackageRegistry<'cfg> { }) } - pub fn get(self, package_ids: &[PackageId]) -> PackageSet<'cfg> { + pub fn get(self, package_ids: &[PackageId]) -> CargoResult> { trace!("getting packages; sources={}", self.sources.len()); PackageSet::new(package_ids, self.sources, self.config) } diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index cc89bb9a6dc..3f4593291d8 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -518,7 +518,7 @@ where let pkg = { let mut map = SourceMap::new(); map.insert(Box::new(&mut source)); - PackageSet::new(&[pkgid.clone()], map, config) + PackageSet::new(&[pkgid.clone()], map, config)? .get_one(&pkgid)? .clone() }; diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 729d748f880..da01a776318 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -16,7 +16,7 @@ use util::profile; pub fn resolve_ws<'a>(ws: &Workspace<'a>) -> CargoResult<(PackageSet<'a>, Resolve)> { let mut registry = PackageRegistry::new(ws.config())?; let resolve = resolve_with_registry(ws, &mut registry, true)?; - let packages = get_resolved_packages(&resolve, registry); + let packages = get_resolved_packages(&resolve, registry)?; Ok((packages, resolve)) } @@ -96,7 +96,7 @@ pub fn resolve_ws_with_method<'a>( true, )?; - let packages = get_resolved_packages(&resolved_with_overrides, registry); + let packages = get_resolved_packages(&resolved_with_overrides, registry)?; Ok((packages, resolved_with_overrides)) } @@ -374,7 +374,7 @@ pub fn add_overrides<'a>( pub fn get_resolved_packages<'a>( resolve: &Resolve, registry: PackageRegistry<'a>, -) -> PackageSet<'a> { +) -> CargoResult> { let ids: Vec = resolve.iter().cloned().collect(); registry.get(&ids) } diff --git a/src/cargo/util/network.rs b/src/cargo/util/network.rs index 7cdbcbe714d..60a629ea4dc 100644 --- a/src/cargo/util/network.rs +++ b/src/cargo/util/network.rs @@ -6,6 +6,38 @@ use failure::Error; use util::Config; use util::errors::{CargoResult, HttpNot200}; +pub struct Retry<'a> { + config: &'a Config, + remaining: u32, +} + +impl<'a> Retry<'a> { + pub fn new(config: &'a Config) -> CargoResult> { + Ok(Retry { + config, + remaining: config.get::>("net.retry")?.unwrap_or(2), + }) + } + + pub fn try(&mut self, f: impl FnOnce() -> CargoResult) + -> CargoResult> + { + match f() { + Err(ref e) if maybe_spurious(e) && self.remaining > 0 => { + let msg = format!( + "spurious network error ({} tries \ + remaining): {}", + self.remaining, e + ); + self.config.shell().warn(msg)?; + self.remaining -= 1; + Ok(None) + } + other => other.map(Some), + } + } +} + fn maybe_spurious(err: &Error) -> bool { for e in err.iter_chain() { if let Some(git_err) = e.downcast_ref::() { @@ -48,21 +80,10 @@ pub fn with_retry(config: &Config, mut callback: F) -> CargoResult where F: FnMut() -> CargoResult, { - let mut remaining = config.get::>("net.retry")?.unwrap_or(2); + let mut retry = Retry::new(config)?; loop { - match callback() { - Ok(ret) => return Ok(ret), - Err(ref e) if maybe_spurious(e) && remaining > 0 => { - let msg = format!( - "spurious network error ({} tries \ - remaining): {}", - remaining, e - ); - config.shell().warn(msg)?; - remaining -= 1; - } - //todo impl from - Err(e) => return Err(e), + if let Some(ret) = retry.try(&mut callback)? { + return Ok(ret) } } } diff --git a/src/cargo/util/progress.rs b/src/cargo/util/progress.rs index 9b419120a3f..15ca758afaf 100644 --- a/src/cargo/util/progress.rs +++ b/src/cargo/util/progress.rs @@ -62,6 +62,10 @@ impl<'cfg> Progress<'cfg> { self.state = None; } + pub fn is_enabled(&self) -> bool { + self.state.is_some() + } + pub fn new(name: &str, cfg: &'cfg Config) -> Progress<'cfg> { Self::with_style(name, ProgressStyle::Percentage, cfg) } From 0ea0c8ba5c81a32fd33eba1c996c2e41e0788926 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 12 Sep 2018 19:24:33 -0700 Subject: [PATCH 05/15] Only enable downloads through an RAII structure This should help us scope downloads to ensure that we only ever have one download session at once and it's explicitly scoped so we can handle the progress bar and everything. --- .../compiler/context/unit_dependencies.rs | 11 +- src/cargo/core/package.rs | 163 ++++++++++-------- src/cargo/ops/cargo_compile.rs | 9 +- src/cargo/ops/cargo_doc.rs | 8 +- src/cargo/ops/cargo_fetch.rs | 10 +- src/cargo/ops/cargo_output_metadata.rs | 8 +- 6 files changed, 121 insertions(+), 88 deletions(-) diff --git a/src/cargo/core/compiler/context/unit_dependencies.rs b/src/cargo/core/compiler/context/unit_dependencies.rs index 7b40ee0d856..668ddcb0fc0 100644 --- a/src/cargo/core/compiler/context/unit_dependencies.rs +++ b/src/cargo/core/compiler/context/unit_dependencies.rs @@ -22,6 +22,7 @@ use CargoResult; use core::dependency::Kind as DepKind; use core::profiles::ProfileFor; use core::{Package, Target, PackageId}; +use core::package::Downloads; use super::{BuildContext, CompileMode, Kind, Unit}; struct State<'a: 'tmp, 'cfg: 'a, 'tmp> { @@ -29,6 +30,7 @@ struct State<'a: 'tmp, 'cfg: 'a, 'tmp> { deps: &'tmp mut HashMap, Vec>>, pkgs: RefCell<&'tmp mut HashMap<&'a PackageId, &'a Package>>, waiting_on_download: HashSet<&'a PackageId>, + downloads: Downloads<'a, 'cfg>, } pub fn build_unit_dependencies<'a, 'cfg>( @@ -44,6 +46,7 @@ pub fn build_unit_dependencies<'a, 'cfg>( deps, pkgs: RefCell::new(pkgs), waiting_on_download: HashSet::new(), + downloads: bcx.packages.enable_download()?, }; loop { @@ -495,7 +498,7 @@ impl<'a, 'cfg, 'tmp> State<'a, 'cfg, 'tmp> { if !self.waiting_on_download.insert(id) { return Ok(None) } - if let Some(pkg) = self.bcx.packages.start_download(id)? { + if let Some(pkg) = self.downloads.start(id)? { pkgs.insert(id, pkg); self.waiting_on_download.remove(id); return Ok(Some(pkg)) @@ -512,9 +515,9 @@ impl<'a, 'cfg, 'tmp> State<'a, 'cfg, 'tmp> { /// returns, and it's hoped that by returning we'll be able to push more /// packages to download into the queu. fn finish_some_downloads(&mut self) -> CargoResult<()> { - assert!(self.bcx.packages.remaining_downloads() > 0); + assert!(self.downloads.remaining() > 0); loop { - let pkg = self.bcx.packages.wait_for_download()?; + let pkg = self.downloads.wait()?; self.waiting_on_download.remove(pkg.package_id()); self.pkgs.borrow_mut().insert(pkg.package_id(), pkg); @@ -522,7 +525,7 @@ impl<'a, 'cfg, 'tmp> State<'a, 'cfg, 'tmp> { // is a good enough number to "fill the network pipe". If we have // less than this let's recompute the whole unit dependency graph // again and try to find some more packages to download. - if self.bcx.packages.remaining_downloads() < 5 { + if self.downloads.remaining() < 5 { break } } diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 28b3533b904..2c6707aae94 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -1,4 +1,4 @@ -use std::cell::{Ref, RefCell}; +use std::cell::{Ref, RefCell, Cell}; use std::collections::{HashMap, HashSet}; use std::fmt; use std::hash; @@ -247,18 +247,20 @@ pub struct PackageSet<'cfg> { packages: HashMap>, sources: RefCell>, config: &'cfg Config, - downloads: RefCell>, + multi: Multi, + downloading: Cell, } -pub(crate) struct Downloads<'cfg> { +pub struct Downloads<'a, 'cfg: 'a> { + set: &'a PackageSet<'cfg>, pending: HashMap, pending_ids: HashSet, results: Vec<(usize, CargoResult<()>)>, next: usize, - multi: Multi, retry: Retry<'cfg>, - progress: Progress<'cfg>, + progress: RefCell>, downloads_finished: usize, + downloaded_bytes: usize, } struct Download { @@ -267,6 +269,8 @@ struct Download { data: RefCell>, url: String, descriptor: String, + total: Cell, + current: Cell, } impl<'cfg> PackageSet<'cfg> { @@ -288,20 +292,8 @@ impl<'cfg> PackageSet<'cfg> { .collect(), sources: RefCell::new(sources), config, - downloads: RefCell::new(Downloads { - multi, - next: 0, - pending: HashMap::new(), - pending_ids: HashSet::new(), - results: Vec::new(), - retry: Retry::new(config)?, - progress: Progress::with_style( - "Downloading", - ProgressStyle::Ratio, - config, - ), - downloads_finished: 0, - }), + multi, + downloading: Cell::new(false), }) } @@ -309,17 +301,47 @@ impl<'cfg> PackageSet<'cfg> { Box::new(self.packages.keys()) } + pub fn enable_download<'a>(&'a self) -> CargoResult> { + assert!(!self.downloading.replace(true)); + Ok(Downloads { + set: self, + next: 0, + pending: HashMap::new(), + pending_ids: HashSet::new(), + results: Vec::new(), + retry: Retry::new(self.config)?, + progress: RefCell::new(Progress::with_style( + "Downloading", + ProgressStyle::Ratio, + self.config, + )), + downloads_finished: 0, + downloaded_bytes: 0, + }) + } + pub fn get_one(&self, id: &PackageId) -> CargoResult<&Package> { + let mut e = self.enable_download()?; + match e.start(id)? { + Some(s) => Ok(s), + None => e.wait(), + } + } + + pub fn sources(&self) -> Ref> { + self.sources.borrow() + } +} + +impl<'a, 'cfg> Downloads<'a, 'cfg> { /// Starts to download the package for the `id` specified. /// /// Returns `None` if the package is queued up for download and will /// eventually be returned from `wait_for_download`. Returns `Some(pkg)` if /// the package is ready and doesn't need to be downloaded. - pub fn start_download(&self, id: &PackageId) -> CargoResult> { - let mut downloads = self.downloads.borrow_mut(); - + pub fn start(&mut self, id: &PackageId) -> CargoResult> { // First up see if we've already cached this package, in which case // there's nothing to do. - let slot = self.packages + let slot = self.set.packages .get(id) .ok_or_else(|| internal(format!("couldn't find `{}` in package set", id)))?; if let Some(pkg) = slot.borrow() { @@ -329,7 +351,7 @@ impl<'cfg> PackageSet<'cfg> { // Ask the original source fo this `PackageId` for the corresponding // package. That may immediately come back and tell us that the package // is ready, or it could tell us that it needs to be downloaded. - let mut sources = self.sources.borrow_mut(); + let mut sources = self.set.sources.borrow_mut(); let source = sources .get_mut(id.source_id()) .ok_or_else(|| internal(format!("couldn't find source for `{}`", id)))?; @@ -349,12 +371,12 @@ impl<'cfg> PackageSet<'cfg> { // internal state and hand off an `Easy` handle to our libcurl `Multi` // handle. This won't actually start the transfer, but later it'll // hapen during `wait_for_download` - let token = downloads.next; - downloads.next += 1; + let token = self.next; + self.next += 1; debug!("downloading {} as {}", id, token); - assert!(downloads.pending_ids.insert(id.clone())); + assert!(self.pending_ids.insert(id.clone())); - let mut handle = ops::http_handle(self.config)?; + let mut handle = ops::http_handle(self.set.config)?; handle.get(true)?; handle.url(&url)?; handle.follow_location(true)?; // follow redirects @@ -383,22 +405,33 @@ impl<'cfg> PackageSet<'cfg> { Ok(buf.len()) })?; + handle.progress(true)?; + handle.progress_function(move |dl_total, dl_cur, _, _| { + tls::with(|downloads| { + let dl = &downloads.pending[&token].0; + dl.total.set(dl_total as usize); + dl.current.set(dl_cur as usize); + downloads.tick(false).is_ok() + }) + })?; + let dl = Download { token, data: RefCell::new(Vec::new()), id: id.clone(), url, descriptor, + total: Cell::new(0), + current: Cell::new(0), }; - downloads.enqueue(dl, handle)?; + self.enqueue(dl, handle)?; Ok(None) } /// Returns the number of crates that are still downloading - pub fn remaining_downloads(&self) -> usize { - let downloads = self.downloads.borrow(); - downloads.pending.len() + pub fn remaining(&self) -> usize { + self.pending.len() } /// Blocks the current thread waiting for a package to finish downloading. @@ -409,33 +442,30 @@ impl<'cfg> PackageSet<'cfg> { /// # Panics /// /// This function will panic if there are no remaining downloads. - pub fn wait_for_download(&self) -> CargoResult<&Package> { - let mut downloads = self.downloads.borrow_mut(); - let downloads = &mut *downloads; - downloads.tick_now()?; + pub fn wait(&mut self) -> CargoResult<&'a Package> { + self.tick(true)?; let (dl, data) = loop { - assert_eq!(downloads.pending.len(), downloads.pending_ids.len()); - let (token, result) = downloads.wait_for_curl()?; + assert_eq!(self.pending.len(), self.pending_ids.len()); + let (token, result) = self.wait_for_curl()?; debug!("{} finished with {:?}", token, result); - let (mut dl, handle) = downloads.pending.remove(&token) + let (mut dl, handle) = self.pending.remove(&token) .expect("got a token for a non-in-progress transfer"); let data = mem::replace(&mut *dl.data.borrow_mut(), Vec::new()); - let mut handle = downloads.multi.remove(handle)?; + let mut handle = self.set.multi.remove(handle)?; // Check if this was a spurious error. If it was a spurious error // then we want to re-enqueue our request for another attempt and // then we wait for another request to finish. let ret = { - downloads.retry.try(|| { + self.retry.try(|| { result.chain_err(|| { format!("failed to download from `{}`", dl.url) })?; let code = handle.response_code()?; if code != 200 && code != 0 { - handle.url(&dl.url)?; let url = handle.effective_url()?.unwrap_or(&dl.url); return Err(HttpNot200 { code, @@ -448,43 +478,30 @@ impl<'cfg> PackageSet<'cfg> { if ret.is_some() { break (dl, data) } - downloads.enqueue(dl, handle)?; + self.enqueue(dl, handle)?; }; - downloads.pending_ids.remove(&dl.id); - if !downloads.progress.is_enabled() { - self.config.shell().status("Downloading", &dl.descriptor)?; + self.pending_ids.remove(&dl.id); + if !self.progress.borrow().is_enabled() { + self.set.config.shell().status("Downloading", &dl.descriptor)?; } - downloads.downloads_finished += 1; - downloads.tick_now()?; + self.downloads_finished += 1; + self.downloaded_bytes += dl.total.get(); + self.tick(true)?; // Inform the original source that the download is finished which // should allow us to actually get the package and fill it in now. - let mut sources = self.sources.borrow_mut(); + let mut sources = self.set.sources.borrow_mut(); let source = sources .get_mut(dl.id.source_id()) .ok_or_else(|| internal(format!("couldn't find source for `{}`", dl.id)))?; let pkg = source.finish_download(&dl.id, data)?; - let slot = &self.packages[&dl.id]; + let slot = &self.set.packages[&dl.id]; assert!(slot.fill(pkg).is_ok()); Ok(slot.borrow().unwrap()) } - pub fn get_one(&self, id: &PackageId) -> CargoResult<&Package> { - assert_eq!(self.remaining_downloads(), 0); - match self.start_download(id)? { - Some(s) => Ok(s), - None => self.wait_for_download(), - } - } - - pub fn sources(&self) -> Ref> { - self.sources.borrow() - } -} - -impl<'cfg> Downloads<'cfg> { fn enqueue(&mut self, dl: Download, handle: Easy) -> CargoResult<()> { - let mut handle = self.multi.add(handle)?; + let mut handle = self.set.multi.add(handle)?; handle.set_token(dl.token)?; self.pending.insert(dl.token, (dl, handle)); Ok(()) @@ -508,12 +525,12 @@ impl<'cfg> Downloads<'cfg> { // `wait` method on `multi`. loop { let n = tls::set(self, || { - self.multi.perform() + self.set.multi.perform() .chain_err(|| "failed to perform http requests") })?; debug!("handles remaining: {}", n); let results = &mut self.results; - self.multi.messages(|msg| { + self.set.multi.messages(|msg| { let token = msg.token().expect("failed to read token"); if let Some(result) = msg.result() { results.push((token, result.map_err(|e| e.into()))); @@ -526,19 +543,25 @@ impl<'cfg> Downloads<'cfg> { break Ok(pair) } assert!(self.pending.len() > 0); - self.multi.wait(&mut [], Duration::new(60, 0)) + self.set.multi.wait(&mut [], Duration::new(60, 0)) .chain_err(|| "failed to wait on curl `Multi`")?; } } - fn tick_now(&mut self) -> CargoResult<()> { - self.progress.tick( + fn tick(&self, now: bool) -> CargoResult<()> { + self.progress.borrow_mut().tick( self.downloads_finished, self.downloads_finished + self.pending.len(), ) } } +impl<'a, 'cfg> Drop for Downloads<'a, 'cfg> { + fn drop(&mut self) { + self.set.downloading.set(false); + } +} + mod tls { use std::cell::Cell; diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index ebd3fdb800d..f55f61b884a 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -244,13 +244,16 @@ pub fn compile_ws<'a>( let (packages, resolve_with_overrides) = resolve; let mut to_builds = Vec::new(); + let mut downloads = packages.enable_download()?; for spec in specs.iter() { let pkgid = spec.query(resolve_with_overrides.iter())?; - to_builds.extend(packages.start_download(pkgid)?); + to_builds.extend(downloads.start(pkgid)?); } - while packages.remaining_downloads() > 0 { - to_builds.push(packages.wait_for_download()?); + while downloads.remaining() > 0 { + to_builds.push(downloads.wait()?); } + drop(downloads); + // The ordering here affects some error messages coming out of cargo, so // let's be test and CLI friendly by always printing in the same order if // there's an error. diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs index 4873ec40983..5e680606cd5 100644 --- a/src/cargo/ops/cargo_doc.rs +++ b/src/cargo/ops/cargo_doc.rs @@ -32,13 +32,15 @@ pub fn doc(ws: &Workspace, options: &DocOptions) -> CargoResult<()> { let (packages, resolve_with_overrides) = resolve; let mut pkgs = Vec::new(); + let mut downloads = packages.enable_download()?; for spec in specs.iter() { let pkgid = spec.query(resolve_with_overrides.iter())?; - pkgs.extend(packages.start_download(pkgid)?); + pkgs.extend(downloads.start(pkgid)?); } - while packages.remaining_downloads() > 0 { - pkgs.push(packages.wait_for_download()?); + while downloads.remaining() > 0 { + pkgs.push(downloads.wait()?); } + drop(downloads); let mut lib_names = HashMap::new(); let mut bin_names = HashMap::new(); diff --git a/src/cargo/ops/cargo_fetch.rs b/src/cargo/ops/cargo_fetch.rs index 08e926d421a..61de8468bb8 100644 --- a/src/cargo/ops/cargo_fetch.rs +++ b/src/cargo/ops/cargo_fetch.rs @@ -25,6 +25,7 @@ pub fn fetch<'a>( let target_info = TargetInfo::new(config, &build_config.requested_target, &rustc, Kind::Target)?; { + let mut downloads = packages.enable_download()?; let mut fetched_packages = HashSet::new(); let mut deps_to_fetch = ws.members().map(|p| p.package_id()).collect::>(); @@ -33,7 +34,7 @@ pub fn fetch<'a>( continue; } - packages.start_download(id)?; + downloads.start(id)?; let deps = resolve.deps(id) .filter(|&(_id, deps)| { deps.iter() @@ -57,10 +58,9 @@ pub fn fetch<'a>( .map(|(id, _deps)| id); deps_to_fetch.extend(deps); } - } - - while packages.remaining_downloads() > 0 { - packages.wait_for_download()?; + while downloads.remaining() > 0 { + downloads.wait()?; + } } Ok((resolve, packages)) diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index 466579d636d..406b09cadb2 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -57,15 +57,17 @@ fn metadata_full(ws: &Workspace, opt: &OutputMetadataOptions) -> CargoResult 0 { - let pkg = package_set.wait_for_download()?; + while downloads.remaining() > 0 { + let pkg = downloads.wait()?; packages.insert(pkg.package_id().clone(), pkg.clone()); } + drop(downloads); Ok(ExportInfo { packages: packages.values().map(|p| (*p).clone()).collect(), From 775b4ebca17bdbac8d945e5513b770c76711b590 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 12 Sep 2018 19:31:12 -0700 Subject: [PATCH 06/15] Comment about how we print downloads --- src/cargo/core/package.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 2c6707aae94..a35f5ad3c50 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -481,9 +481,25 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { self.enqueue(dl, handle)?; }; self.pending_ids.remove(&dl.id); + + // If the progress bar isn't enabled then we still want to provide some + // semblance of progress of how we're downloading crates. Historically + // we did this by printing `Downloading ...` as we downloaded each crate + // one at a time, but now we download them all in parallel so there's no + // sense of progress if they get printed all at once. + // + // As a sort of weird compromise/hack we print `Downloading ...` when a + // crate is *finished* rather than when it starts. This should give a + // good sense of progress as we're printing as things complete (as + // opposed to all at once at the beginning). It's a bit of a lie because + // the crate is already downloaded, though. + // + // We can continue to iterate on this, but this is hopefully good enough + // for now. if !self.progress.borrow().is_enabled() { self.set.config.shell().status("Downloading", &dl.descriptor)?; } + self.downloads_finished += 1; self.downloaded_bytes += dl.total.get(); self.tick(true)?; From a46df8fe7d7e3988943115ae78ad9b9cc694ce18 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 12 Sep 2018 20:57:01 -0700 Subject: [PATCH 07/15] Update the progress bar for parallel downloads This is actually a super tricky problem. We don't really have the capacity for more than one line of update-able information in Cargo right now, so we need to squeeze a lot of information into one line of output for Cargo. The main constraints this tries to satisfy are: * At all times it should be clear what's happening. Cargo shouldn't just hang with no output when downloading a crate for a long time, a counter ideally needs to be decreasing while the download progresses. * If a progress bar is shown, it shouldn't jump around. This ends up just being a surprising user experience for most. Progress bars should only ever increase, but they may increase at different speeds. * Cargo has, currently, at most one line of output (as mentioned above) to pack information into. We haven't delved into fancier terminal features that involve multiple lines of update-able output. * When downloading crates as part of `cargo build` (the norm) we don't actually know ahead of time how many crates are being downloaded. We rely on the calculation of unit dependencies to naturally feed into downloading more crates. * Furthermore, once we decide to download a crate, we don't actually know how big it is! We have to wait for the server to tell us how big it is. There doesn't really seem to be a great solution that satisfies all of these constraints unfortunately. As a result this commit implements a relatively conservative solution which should hopefully get us most of the way there. There isn't actually a progress bar but rather Cargo prints that it's got N crates left to download, and if it takes awhile it prints out that there are M bytes remaining. Unfortunately the progress is pretty choppy and jerky, not providing a smooth UI. This appears to largely be because Cargo will synchronously extract tarballs, which for large crates can cause a noticeable pause. Cargo's not really prepared internally to perform this work on helper threads, but ideally if it could do so it would improve the output quite a bit! (making it much smoother and also able to account for the time tarball extraction takes). --- Cargo.toml | 1 + src/cargo/core/compiler/job_queue.rs | 12 +- src/cargo/core/package.rs | 115 +++++++++++++---- src/cargo/lib.rs | 1 + src/cargo/util/mod.rs | 12 ++ src/cargo/util/progress.rs | 182 ++++++++++++++++++--------- 6 files changed, 232 insertions(+), 91 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 147b38e55c5..51e5c8b4ca4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ path = "src/cargo/lib.rs" [dependencies] atty = "0.2" +bytesize = "1.0" crates-io = { path = "src/crates-io", version = "0.20" } crossbeam-utils = "0.5" crypto-hash = "0.3.1" diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index c2681c892b6..1323e393c90 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -14,6 +14,7 @@ use jobserver::{Acquired, HelperThread}; use core::profiles::Profile; use core::{PackageId, Target, TargetKind}; use handle_error; +use util; use util::{internal, profile, CargoResult, CargoResultExt, ProcessBuilder}; use util::{Config, DependencyQueue, Dirty, Fresh, Freshness}; use util::{Progress, ProgressStyle}; @@ -368,16 +369,7 @@ impl<'a> JobQueue<'a> { opt_type += " + debuginfo"; } - let time_elapsed = { - let duration = cx.bcx.config.creation_time().elapsed(); - let secs = duration.as_secs(); - - if secs >= 60 { - format!("{}m {:02}s", secs / 60, secs % 60) - } else { - format!("{}.{:02}s", secs, duration.subsec_nanos() / 10_000_000) - } - }; + let time_elapsed = util::elapsed(cx.bcx.config.creation_time().elapsed()); if self.queue.is_empty() { let message = format!( diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index a35f5ad3c50..a5769c2af10 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -4,21 +4,22 @@ use std::fmt; use std::hash; use std::mem; use std::path::{Path, PathBuf}; -use std::time::Duration; +use std::time::{Instant, Duration}; +use bytesize::ByteSize; +use curl::easy::{Easy, HttpVersion}; +use curl::multi::{Multi, EasyHandle}; +use lazycell::LazyCell; use semver::Version; use serde::ser; use toml; -use lazycell::LazyCell; -use curl::easy::{Easy, HttpVersion}; -use curl::multi::{Multi, EasyHandle}; use core::{Dependency, Manifest, PackageId, SourceId, Target}; use core::{FeatureMap, SourceMap, Summary}; use core::source::MaybePackage; use core::interning::InternedString; use ops; -use util::{internal, lev_distance, Config, Progress, ProgressStyle}; +use util::{self, internal, lev_distance, Config, Progress, ProgressStyle}; use util::errors::{CargoResult, CargoResultExt, HttpNot200}; use util::network::Retry; @@ -258,9 +259,11 @@ pub struct Downloads<'a, 'cfg: 'a> { results: Vec<(usize, CargoResult<()>)>, next: usize, retry: Retry<'cfg>, - progress: RefCell>, + progress: RefCell>>, downloads_finished: usize, - downloaded_bytes: usize, + downloaded_bytes: u64, + largest: (u64, String), + start: Instant, } struct Download { @@ -269,8 +272,9 @@ struct Download { data: RefCell>, url: String, descriptor: String, - total: Cell, - current: Cell, + total: Cell, + current: Cell, + start: Instant, } impl<'cfg> PackageSet<'cfg> { @@ -304,19 +308,21 @@ impl<'cfg> PackageSet<'cfg> { pub fn enable_download<'a>(&'a self) -> CargoResult> { assert!(!self.downloading.replace(true)); Ok(Downloads { + start: Instant::now(), set: self, next: 0, pending: HashMap::new(), pending_ids: HashSet::new(), results: Vec::new(), retry: Retry::new(self.config)?, - progress: RefCell::new(Progress::with_style( + progress: RefCell::new(Some(Progress::with_style( "Downloading", ProgressStyle::Ratio, self.config, - )), + ))), downloads_finished: 0, downloaded_bytes: 0, + largest: (0, String::new()), }) } pub fn get_one(&self, id: &PackageId) -> CargoResult<&Package> { @@ -409,9 +415,9 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { handle.progress_function(move |dl_total, dl_cur, _, _| { tls::with(|downloads| { let dl = &downloads.pending[&token].0; - dl.total.set(dl_total as usize); - dl.current.set(dl_cur as usize); - downloads.tick(false).is_ok() + dl.total.set(dl_total as u64); + dl.current.set(dl_cur as u64); + downloads.tick(WhyTick::DownloadUpdate).is_ok() }) })?; @@ -423,8 +429,10 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { descriptor, total: Cell::new(0), current: Cell::new(0), + start: Instant::now(), }; self.enqueue(dl, handle)?; + self.tick(WhyTick::DownloadStarted)?; Ok(None) } @@ -443,8 +451,6 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { /// /// This function will panic if there are no remaining downloads. pub fn wait(&mut self) -> CargoResult<&'a Package> { - self.tick(true)?; - let (dl, data) = loop { assert_eq!(self.pending.len(), self.pending_ids.len()); let (token, result) = self.wait_for_curl()?; @@ -496,13 +502,26 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { // // We can continue to iterate on this, but this is hopefully good enough // for now. - if !self.progress.borrow().is_enabled() { + if !self.progress.borrow().as_ref().unwrap().is_enabled() { self.set.config.shell().status("Downloading", &dl.descriptor)?; } self.downloads_finished += 1; self.downloaded_bytes += dl.total.get(); - self.tick(true)?; + if dl.total.get() > self.largest.0 { + self.largest = (dl.total.get(), dl.id.name().to_string()); + } + + // We're about to synchronously extract the crate below. While we're + // doing that our download progress won't actually be updated, nor do we + // have a great view into the progress of the extraction. Let's prepare + // the user for this CPU-heavy step if it looks like it'll take some + // time to do so. + if dl.total.get() < ByteSize::kb(400).0 { + self.tick(WhyTick::DownloadFinished)?; + } else { + self.tick(WhyTick::Extracting(&dl.id.name()))?; + } // Inform the original source that the download is finished which // should allow us to actually get the package and fill it in now. @@ -564,17 +583,67 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { } } - fn tick(&self, now: bool) -> CargoResult<()> { - self.progress.borrow_mut().tick( - self.downloads_finished, - self.downloads_finished + self.pending.len(), - ) + fn tick(&self, why: WhyTick) -> CargoResult<()> { + let mut progress = self.progress.borrow_mut(); + let progress = progress.as_mut().unwrap(); + + if let WhyTick::DownloadUpdate = why { + if !progress.update_allowed() { + return Ok(()) + } + } + let mut msg = format!("{} crates", self.pending.len()); + match why { + WhyTick::Extracting(krate) => { + msg.push_str(&format!(", extracting {} ...", krate)); + } + _ => { + let mut dur = Duration::new(0, 0); + let mut remaining = 0; + for (dl, _) in self.pending.values() { + dur += dl.start.elapsed(); + remaining += dl.total.get() - dl.current.get(); + } + if remaining > 0 && dur > Duration::from_millis(500) { + msg.push_str(&format!(", remaining bytes: {}", ByteSize(remaining))); + } + } + } + progress.print_now(&msg) } } +enum WhyTick<'a> { + DownloadStarted, + DownloadUpdate, + DownloadFinished, + Extracting(&'a str), +} + impl<'a, 'cfg> Drop for Downloads<'a, 'cfg> { fn drop(&mut self) { self.set.downloading.set(false); + let progress = self.progress.get_mut().take().unwrap(); + // Don't print a download summary if we're not using a progress bar, + // we've already printed lots of `Downloading...` items. + if !progress.is_enabled() { + return + } + if self.downloads_finished == 0 { + return + } + let mut status = format!("{} crates ({}) in {}", + self.downloads_finished, + ByteSize(self.downloaded_bytes), + util::elapsed(self.start.elapsed())); + if self.largest.0 > ByteSize::mb(1).0 { + status.push_str(&format!( + " (largest was `{}` at {})", + self.largest.1, + ByteSize(self.largest.0), + )); + } + drop(self.set.config.shell().status("Downloaded", status)); } } diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index 6792ba12c38..d511238f4ce 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -15,6 +15,7 @@ #![cfg_attr(feature = "cargo-clippy", allow(wrong_self_convention))] // perhaps Rc should be special cased in Clippy? extern crate atty; +extern crate bytesize; extern crate clap; #[cfg(target_os = "macos")] extern crate core_foundation; diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index aeb78b295e3..c18914954f6 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -1,3 +1,5 @@ +use std::time::Duration; + pub use self::cfg::{Cfg, CfgExpr}; pub use self::config::{homedir, Config, ConfigValue}; pub use self::dependency_queue::{DependencyQueue, Dirty, Fresh, Freshness}; @@ -46,3 +48,13 @@ mod read2; mod progress; mod lockserver; pub mod diagnostic_server; + +pub fn elapsed(duration: Duration) -> String { + let secs = duration.as_secs(); + + if secs >= 60 { + format!("{}m {:02}s", secs / 60, secs % 60) + } else { + format!("{}.{:02}s", secs, duration.subsec_nanos() / 10_000_000) + } +} diff --git a/src/cargo/util/progress.rs b/src/cargo/util/progress.rs index 15ca758afaf..c6cedf62191 100644 --- a/src/cargo/util/progress.rs +++ b/src/cargo/util/progress.rs @@ -16,13 +16,17 @@ pub enum ProgressStyle { Ratio, } +struct Throttle { + first: bool, + last_update: Instant, +} + struct State<'cfg> { config: &'cfg Config, format: Format, - first: bool, - last_update: Instant, name: String, done: bool, + throttle: Throttle, } struct Format { @@ -50,10 +54,9 @@ impl<'cfg> Progress<'cfg> { max_width: n, max_print: 80, }, - first: true, - last_update: Instant::now(), name: name.to_string(), done: false, + throttle: Throttle::new(), }), } } @@ -71,8 +74,47 @@ impl<'cfg> Progress<'cfg> { } pub fn tick(&mut self, cur: usize, max: usize) -> CargoResult<()> { + let s = match &mut self.state { + Some(s) => s, + None => return Ok(()), + }; + + // Don't update too often as it can cause excessive performance loss + // just putting stuff onto the terminal. We also want to avoid + // flickering by not drawing anything that goes away too quickly. As a + // result we've got two branches here: + // + // 1. If we haven't drawn anything, we wait for a period of time to + // actually start drawing to the console. This ensures that + // short-lived operations don't flicker on the console. Currently + // there's a 500ms delay to when we first draw something. + // 2. If we've drawn something, then we rate limit ourselves to only + // draw to the console every so often. Currently there's a 100ms + // delay between updates. + if !s.throttle.allowed() { + return Ok(()) + } + + s.tick(cur, max, "") + } + + pub fn tick_now(&mut self, cur: usize, max: usize, msg: &str) -> CargoResult<()> { match self.state { - Some(ref mut s) => s.tick(cur, max, "", true), + Some(ref mut s) => s.tick(cur, max, msg), + None => Ok(()), + } + } + + pub fn update_allowed(&mut self) -> bool { + match &mut self.state { + Some(s) => s.throttle.allowed(), + None => false, + } + } + + pub fn print_now(&mut self, msg: &str) -> CargoResult<()> { + match &mut self.state { + Some(s) => s.print("", msg), None => Ok(()), } } @@ -82,60 +124,74 @@ impl<'cfg> Progress<'cfg> { s.clear(); } } +} - pub fn tick_now(&mut self, cur: usize, max: usize, msg: &str) -> CargoResult<()> { - match self.state { - Some(ref mut s) => s.tick(cur, max, msg, false), - None => Ok(()), +impl Throttle { + fn new() -> Throttle { + Throttle { + first: true, + last_update: Instant::now(), } } + + fn allowed(&mut self) -> bool { + if self.first { + let delay = Duration::from_millis(500); + if self.last_update.elapsed() < delay { + return false + } + } else { + let interval = Duration::from_millis(100); + if self.last_update.elapsed() < interval { + return false + } + } + self.update(); + true + } + + fn update(&mut self) { + self.first = false; + self.last_update = Instant::now(); + } } impl<'cfg> State<'cfg> { - fn tick(&mut self, cur: usize, max: usize, msg: &str, throttle: bool) -> CargoResult<()> { + fn tick(&mut self, cur: usize, max: usize, msg: &str) -> CargoResult<()> { if self.done { return Ok(()); } - // Don't update too often as it can cause excessive performance loss - // just putting stuff onto the terminal. We also want to avoid - // flickering by not drawing anything that goes away too quickly. As a - // result we've got two branches here: - // - // 1. If we haven't drawn anything, we wait for a period of time to - // actually start drawing to the console. This ensures that - // short-lived operations don't flicker on the console. Currently - // there's a 500ms delay to when we first draw something. - // 2. If we've drawn something, then we rate limit ourselves to only - // draw to the console every so often. Currently there's a 100ms - // delay between updates. - if throttle { - if self.first { - let delay = Duration::from_millis(500); - if self.last_update.elapsed() < delay { - return Ok(()); - } - self.first = false; - } else { - let interval = Duration::from_millis(100); - if self.last_update.elapsed() < interval { - return Ok(()); - } - } - self.last_update = Instant::now(); - } - - if cur == max { + if max > 0 && cur == max { self.done = true; } // Write out a pretty header, then the progress bar itself, and then // return back to the beginning of the line for the next print. self.try_update_max_width(); - if let Some(string) = self.format.progress_status(cur, max, msg) { - self.config.shell().status_header(&self.name)?; - write!(self.config.shell().err(), "{}\r", string)?; + if let Some(pbar) = self.format.progress(cur, max) { + self.print(&pbar, msg)?; + } + Ok(()) + } + + fn print(&mut self, prefix: &str, msg: &str) -> CargoResult<()> { + self.throttle.update(); + self.try_update_max_width(); + + // make sure we have enough room for the header + if self.format.max_width < 15 { + return Ok(()) } + self.config.shell().status_header(&self.name)?; + let mut line = prefix.to_string(); + self.format.render(&mut line, msg); + + while line.len() < self.format.max_width - 15 { + line.push(' '); + } + + write!(self.config.shell().err(), "{}\r", line)?; Ok(()) } @@ -153,7 +209,7 @@ impl<'cfg> State<'cfg> { } impl Format { - fn progress_status(&self, cur: usize, max: usize, msg: &str) -> Option { + fn progress(&self, cur: usize, max: usize) -> Option { // Render the percentage at the far right and then figure how long the // progress bar is let pct = (cur as f64) / (max as f64); @@ -192,26 +248,36 @@ impl Format { string.push_str("]"); string.push_str(&stats); - let mut avail_msg_len = self.max_width - self.width(); + Some(string) + } + + fn render(&self, string: &mut String, msg: &str) { + let mut avail_msg_len = self.max_width - string.len() - 15; let mut ellipsis_pos = 0; - if avail_msg_len > 3 { - for c in msg.chars() { - let display_width = c.width().unwrap_or(0); - if avail_msg_len >= display_width { - avail_msg_len -= display_width; - string.push(c); - if avail_msg_len >= 3 { - ellipsis_pos = string.len(); - } - } else { - string.truncate(ellipsis_pos); - string.push_str("..."); - break; + if avail_msg_len <= 3 { + return + } + for c in msg.chars() { + let display_width = c.width().unwrap_or(0); + if avail_msg_len >= display_width { + avail_msg_len -= display_width; + string.push(c); + if avail_msg_len >= 3 { + ellipsis_pos = string.len(); } + } else { + string.truncate(ellipsis_pos); + string.push_str("..."); + break; } } + } - Some(string) + #[cfg(test)] + fn progress_status(&self, cur: usize, max: usize, msg: &str) -> Option { + let mut ret = self.progress(cur, max)?; + self.render(&mut ret, msg); + Some(ret) } fn width(&self) -> usize { From e2637b6599b36725d65c130945a9225b4093a81f Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 14 Sep 2018 13:33:18 -0700 Subject: [PATCH 08/15] Review comments! --- .../compiler/context/unit_dependencies.rs | 4 +- src/cargo/core/package.rs | 45 ++++++----- src/cargo/ops/cargo_compile.rs | 14 +--- src/cargo/ops/cargo_doc.rs | 14 +--- src/cargo/ops/cargo_fetch.rs | 8 +- src/cargo/ops/cargo_output_metadata.rs | 10 +-- tests/testsuite/alt_registry.rs | 22 +++--- tests/testsuite/build.rs | 11 +-- tests/testsuite/build_script.rs | 6 +- tests/testsuite/cfg.rs | 8 +- tests/testsuite/directory.rs | 3 +- tests/testsuite/fetch.rs | 12 +-- tests/testsuite/git.rs | 4 +- tests/testsuite/install.rs | 16 ++-- tests/testsuite/overrides.rs | 14 ++-- tests/testsuite/patch.rs | 18 +++-- tests/testsuite/registry.rs | 78 ++++++++++++------- tests/testsuite/rename_deps.rs | 3 +- tests/testsuite/support/mod.rs | 1 + tests/testsuite/warn_on_failure.rs | 7 +- tests/testsuite/workspaces.rs | 12 ++- 21 files changed, 175 insertions(+), 135 deletions(-) diff --git a/src/cargo/core/compiler/context/unit_dependencies.rs b/src/cargo/core/compiler/context/unit_dependencies.rs index 668ddcb0fc0..c7050e348ec 100644 --- a/src/cargo/core/compiler/context/unit_dependencies.rs +++ b/src/cargo/core/compiler/context/unit_dependencies.rs @@ -509,11 +509,11 @@ impl<'a, 'cfg, 'tmp> State<'a, 'cfg, 'tmp> { /// Completes at least one downloading, maybe waiting for more to complete. /// /// This function will block the current thread waiting for at least one - /// create to finish downloading. The function may continue to download more + /// crate to finish downloading. The function may continue to download more /// crates if it looks like there's a long enough queue of crates to keep /// downloading. When only a handful of packages remain this function /// returns, and it's hoped that by returning we'll be able to push more - /// packages to download into the queu. + /// packages to download into the queue. fn finish_some_downloads(&mut self) -> CargoResult<()> { assert!(self.downloads.remaining() > 0); loop { diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index a5769c2af10..f4ccc000bcd 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -325,12 +325,23 @@ impl<'cfg> PackageSet<'cfg> { largest: (0, String::new()), }) } + pub fn get_one(&self, id: &PackageId) -> CargoResult<&Package> { - let mut e = self.enable_download()?; - match e.start(id)? { - Some(s) => Ok(s), - None => e.wait(), + Ok(self.get_many(Some(id))?.remove(0)) + } + + pub fn get_many<'a>(&self, ids: impl IntoIterator) + -> CargoResult> + { + let mut pkgs = Vec::new(); + let mut downloads = self.enable_download()?; + for id in ids { + pkgs.extend(downloads.start(id)?); + } + while downloads.remaining() > 0 { + pkgs.push(downloads.wait()?); } + Ok(pkgs) } pub fn sources(&self) -> Ref> { @@ -421,6 +432,16 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { }) })?; + // If the progress bar isn't enabled then it may be awhile before the + // first crate finishes downloading so we inform immediately that we're + // downloading crates here. + if self.downloads_finished == 0 && + self.pending.len() == 0 && + !self.progress.borrow().as_ref().unwrap().is_enabled() + { + self.set.config.shell().status("Downloading", "crates ...")?; + } + let dl = Download { token, data: RefCell::new(Vec::new()), @@ -489,21 +510,9 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { self.pending_ids.remove(&dl.id); // If the progress bar isn't enabled then we still want to provide some - // semblance of progress of how we're downloading crates. Historically - // we did this by printing `Downloading ...` as we downloaded each crate - // one at a time, but now we download them all in parallel so there's no - // sense of progress if they get printed all at once. - // - // As a sort of weird compromise/hack we print `Downloading ...` when a - // crate is *finished* rather than when it starts. This should give a - // good sense of progress as we're printing as things complete (as - // opposed to all at once at the beginning). It's a bit of a lie because - // the crate is already downloaded, though. - // - // We can continue to iterate on this, but this is hopefully good enough - // for now. + // semblance of progress of how we're downloading crates. if !self.progress.borrow().as_ref().unwrap().is_enabled() { - self.set.config.shell().status("Downloading", &dl.descriptor)?; + self.set.config.shell().status("Downloaded", &dl.descriptor)?; } self.downloads_finished += 1; diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index f55f61b884a..0f9264e990e 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -243,16 +243,10 @@ pub fn compile_ws<'a>( let resolve = ops::resolve_ws_with_method(ws, source, method, &specs)?; let (packages, resolve_with_overrides) = resolve; - let mut to_builds = Vec::new(); - let mut downloads = packages.enable_download()?; - for spec in specs.iter() { - let pkgid = spec.query(resolve_with_overrides.iter())?; - to_builds.extend(downloads.start(pkgid)?); - } - while downloads.remaining() > 0 { - to_builds.push(downloads.wait()?); - } - drop(downloads); + let to_build_ids = specs.iter() + .map(|s| s.query(resolve_with_overrides.iter())) + .collect::>>()?; + let mut to_builds = packages.get_many(to_build_ids)?; // The ordering here affects some error messages coming out of cargo, so // let's be test and CLI friendly by always printing in the same order if diff --git a/src/cargo/ops/cargo_doc.rs b/src/cargo/ops/cargo_doc.rs index 5e680606cd5..3e84120f504 100644 --- a/src/cargo/ops/cargo_doc.rs +++ b/src/cargo/ops/cargo_doc.rs @@ -31,16 +31,10 @@ pub fn doc(ws: &Workspace, options: &DocOptions) -> CargoResult<()> { )?; let (packages, resolve_with_overrides) = resolve; - let mut pkgs = Vec::new(); - let mut downloads = packages.enable_download()?; - for spec in specs.iter() { - let pkgid = spec.query(resolve_with_overrides.iter())?; - pkgs.extend(downloads.start(pkgid)?); - } - while downloads.remaining() > 0 { - pkgs.push(downloads.wait()?); - } - drop(downloads); + let ids = specs.iter() + .map(|s| s.query(resolve_with_overrides.iter())) + .collect::>>()?; + let pkgs = packages.get_many(ids)?; let mut lib_names = HashMap::new(); let mut bin_names = HashMap::new(); diff --git a/src/cargo/ops/cargo_fetch.rs b/src/cargo/ops/cargo_fetch.rs index 61de8468bb8..c9d25534f8a 100644 --- a/src/cargo/ops/cargo_fetch.rs +++ b/src/cargo/ops/cargo_fetch.rs @@ -25,16 +25,16 @@ pub fn fetch<'a>( let target_info = TargetInfo::new(config, &build_config.requested_target, &rustc, Kind::Target)?; { - let mut downloads = packages.enable_download()?; let mut fetched_packages = HashSet::new(); let mut deps_to_fetch = ws.members().map(|p| p.package_id()).collect::>(); + let mut to_download = Vec::new(); while let Some(id) = deps_to_fetch.pop() { if !fetched_packages.insert(id) { continue; } - downloads.start(id)?; + to_download.push(id.clone()); let deps = resolve.deps(id) .filter(|&(_id, deps)| { deps.iter() @@ -58,9 +58,7 @@ pub fn fetch<'a>( .map(|(id, _deps)| id); deps_to_fetch.extend(deps); } - while downloads.remaining() > 0 { - downloads.wait()?; - } + packages.get_many(&to_download)?; } Ok((resolve, packages)) diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index 406b09cadb2..89cb252128b 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -57,17 +57,9 @@ fn metadata_full(ws: &Workspace, opt: &OutputMetadataOptions) -> CargoResult 0 { - let pkg = downloads.wait()?; + for pkg in package_set.get_many(package_set.package_ids())? { packages.insert(pkg.package_id().clone(), pkg.clone()); } - drop(downloads); Ok(ExportInfo { packages: packages.values().map(|p| (*p).clone()).collect(), diff --git a/tests/testsuite/alt_registry.rs b/tests/testsuite/alt_registry.rs index fa006305b2f..b1669fff4db 100644 --- a/tests/testsuite/alt_registry.rs +++ b/tests/testsuite/alt_registry.rs @@ -57,7 +57,8 @@ fn depend_on_alt_registry() { .with_stderr(&format!( "\ [UPDATING] `{reg}` index -[DOWNLOADING] bar v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.1 (registry `[ROOT][..]`) [COMPILING] bar v0.0.1 (registry `[ROOT][..]`) [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -110,8 +111,9 @@ fn depend_on_alt_registry_depends_on_same_registry_no_index() { .with_stderr(&format!( "\ [UPDATING] `{reg}` index -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) [COMPILING] baz v0.0.1 (registry `[ROOT][..]`) [COMPILING] bar v0.0.1 (registry `[ROOT][..]`) [COMPILING] foo v0.0.1 ([CWD]) @@ -152,8 +154,9 @@ fn depend_on_alt_registry_depends_on_same_registry() { .with_stderr(&format!( "\ [UPDATING] `{reg}` index -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) [COMPILING] baz v0.0.1 (registry `[ROOT][..]`) [COMPILING] bar v0.0.1 (registry `[ROOT][..]`) [COMPILING] foo v0.0.1 ([CWD]) @@ -195,8 +198,9 @@ fn depend_on_alt_registry_depends_on_crates_io() { "\ [UPDATING] `{alt_reg}` index [UPDATING] `{reg}` index -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) [COMPILING] baz v0.0.1 (registry `[ROOT][..]`) [COMPILING] bar v0.0.1 (registry `[ROOT][..]`) [COMPILING] foo v0.0.1 ([CWD]) @@ -363,8 +367,8 @@ fn alt_registry_and_crates_io_deps() { )).with_stderr_contains(&format!( "[UPDATING] `{}` index", registry::registry_path().to_str().unwrap())) - .with_stderr_contains("[DOWNLOADING] crates_io_dep v0.0.1 (registry `[ROOT][..]`)") - .with_stderr_contains("[DOWNLOADING] alt_reg_dep v0.1.0 (registry `[ROOT][..]`)") + .with_stderr_contains("[DOWNLOADED] crates_io_dep v0.0.1 (registry `[ROOT][..]`)") + .with_stderr_contains("[DOWNLOADED] alt_reg_dep v0.1.0 (registry `[ROOT][..]`)") .with_stderr_contains("[COMPILING] alt_reg_dep v0.1.0 (registry `[ROOT][..]`)") .with_stderr_contains("[COMPILING] crates_io_dep v0.0.1") .with_stderr_contains("[COMPILING] foo v0.0.1 ([CWD])") diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index a422ec68206..aba5dbf09bc 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -3569,11 +3569,12 @@ fn build_all_member_dependency_same_name() { p.cargo("build --all") .with_stderr( - "[..] Updating `[..]` index\n\ - [..] Downloading a v0.1.0 ([..])\n\ - [..] Compiling a v0.1.0\n\ - [..] Compiling a v0.1.0 ([..])\n\ - [..] Finished dev [unoptimized + debuginfo] target(s) in [..]\n", + "[UPDATING] `[..]` index\n\ + [DOWNLOADING] crates ...\n\ + [DOWNLOADED] a v0.1.0 ([..])\n\ + [COMPILING] a v0.1.0\n\ + [COMPILING] a v0.1.0 ([..])\n\ + [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\n", ).run(); } diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index 470f922c741..22052d3c26d 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -2707,7 +2707,8 @@ fn warnings_hidden_for_upstream() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] bar v0.1.0 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.1.0 ([..]) [COMPILING] bar v0.1.0 [RUNNING] `rustc [..]` [RUNNING] `[..]` @@ -2761,7 +2762,8 @@ fn warnings_printed_on_vv() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] bar v0.1.0 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.1.0 ([..]) [COMPILING] bar v0.1.0 [RUNNING] `rustc [..]` [RUNNING] `[..]` diff --git a/tests/testsuite/cfg.rs b/tests/testsuite/cfg.rs index 3c6a6577411..c332438eff2 100644 --- a/tests/testsuite/cfg.rs +++ b/tests/testsuite/cfg.rs @@ -223,8 +223,9 @@ fn works_through_the_registry() { .with_stderr( "\ [UPDATING] [..] index -[DOWNLOADING] [..] -[DOWNLOADING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] [..] +[DOWNLOADED] [..] [COMPILING] baz v0.1.0 [COMPILING] bar v0.1.0 [COMPILING] foo v0.0.1 ([..]) @@ -267,7 +268,8 @@ fn ignore_version_from_other_platform() { .with_stderr( "\ [UPDATING] [..] index -[DOWNLOADING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] [..] [COMPILING] bar v0.1.0 [COMPILING] foo v0.0.1 ([..]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] diff --git a/tests/testsuite/directory.rs b/tests/testsuite/directory.rs index 549210b38e3..32cd776b095 100644 --- a/tests/testsuite/directory.rs +++ b/tests/testsuite/directory.rs @@ -331,7 +331,8 @@ fn crates_io_then_directory() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] bar v0.1.0 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.1.0 ([..]) [COMPILING] bar v0.1.0 [COMPILING] foo v0.1.0 ([CWD]) [FINISHED] [..] diff --git a/tests/testsuite/fetch.rs b/tests/testsuite/fetch.rs index 455b1390a52..a76a4c52b2c 100644 --- a/tests/testsuite/fetch.rs +++ b/tests/testsuite/fetch.rs @@ -53,8 +53,8 @@ fn fetch_all_platform_dependencies_when_no_target_is_given() { .build(); p.cargo("fetch") - .with_stderr_contains("[..] Downloading d1 v1.2.3 [..]") - .with_stderr_contains("[..] Downloading d2 v0.1.2 [..]") + .with_stderr_contains("[DOWNLOADED] d1 v1.2.3 [..]") + .with_stderr_contains("[DOWNLOADED] d2 v0.1.2 [..]") .run(); } @@ -100,13 +100,13 @@ fn fetch_platform_specific_dependencies() { p.cargo("fetch --target") .arg(&host) - .with_stderr_contains("[..] Downloading d1 v1.2.3 [..]") - .with_stderr_does_not_contain("[..] Downloading d2 v0.1.2 [..]") + .with_stderr_contains("[DOWNLOADED] d1 v1.2.3 [..]") + .with_stderr_does_not_contain("[DOWNLOADED] d2 v0.1.2 [..]") .run(); p.cargo("fetch --target") .arg(&target) - .with_stderr_contains("[..] Downloading d2 v0.1.2[..]") - .with_stderr_does_not_contain("[..] Downloading d1 v1.2.3 [..]") + .with_stderr_contains("[DOWNLOADED] d2 v0.1.2[..]") + .with_stderr_does_not_contain("[DOWNLOADED] d1 v1.2.3 [..]") .run(); } diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index 64df7cb1e1d..2e4d4633903 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -2359,8 +2359,8 @@ fn include_overrides_gitignore() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] filetime [..] -[DOWNLOADING] libc [..] +[DOWNLOADED] filetime [..] +[DOWNLOADED] libc [..] [COMPILING] libc [..] [RUNNING] `rustc --crate-name libc [..]` [COMPILING] filetime [..] diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index df3d44d714b..968065d4368 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -27,7 +27,8 @@ fn simple() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] foo v0.0.1 (registry [..]) +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.0.1 (registry [..]) [INSTALLING] foo v0.0.1 [COMPILING] foo v0.0.1 [FINISHED] release [optimized] target(s) in [..] @@ -53,12 +54,14 @@ fn multiple_pkgs() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] foo v0.0.1 (registry `[CWD]/registry`) +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.0.1 (registry `[CWD]/registry`) [INSTALLING] foo v0.0.1 [COMPILING] foo v0.0.1 [FINISHED] release [optimized] target(s) in [..] [INSTALLING] [CWD]/home/.cargo/bin/foo[EXE] -[DOWNLOADING] bar v0.0.2 (registry `[CWD]/registry`) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.2 (registry `[CWD]/registry`) [INSTALLING] bar v0.0.2 [COMPILING] bar v0.0.2 [FINISHED] release [optimized] target(s) in [..] @@ -97,7 +100,8 @@ fn pick_max_version() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] foo v0.2.1 (registry [..]) +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.2.1 (registry [..]) [INSTALLING] foo v0.2.1 [COMPILING] foo v0.2.1 [FINISHED] release [optimized] target(s) in [..] @@ -1004,7 +1008,7 @@ fn vers_precise() { pkg("foo", "0.1.2"); cargo_process("install foo --vers 0.1.1") - .with_stderr_contains("[DOWNLOADING] foo v0.1.1 (registry [..])") + .with_stderr_contains("[DOWNLOADED] foo v0.1.1 (registry [..])") .run(); } @@ -1014,7 +1018,7 @@ fn version_too() { pkg("foo", "0.1.2"); cargo_process("install foo --version 0.1.1") - .with_stderr_contains("[DOWNLOADING] foo v0.1.1 (registry [..])") + .with_stderr_contains("[DOWNLOADED] foo v0.1.1 (registry [..])") .run(); } diff --git a/tests/testsuite/overrides.rs b/tests/testsuite/overrides.rs index 9958204da02..bb5e88fc760 100644 --- a/tests/testsuite/overrides.rs +++ b/tests/testsuite/overrides.rs @@ -185,7 +185,8 @@ fn transitive() { "\ [UPDATING] `[ROOT][..]` index [UPDATING] git repository `[..]` -[DOWNLOADING] baz v0.2.0 (registry [..]) +[DOWNLOADING] crates ... +[DOWNLOADED] baz v0.2.0 (registry [..]) [COMPILING] bar v0.1.0 (file://[..]) [COMPILING] baz v0.2.0 [COMPILING] foo v0.0.1 ([CWD]) @@ -338,8 +339,9 @@ fn use_a_spec_to_select() { "\ [UPDATING] `[ROOT][..]` index [UPDATING] git repository `[..]` -[DOWNLOADING] [..] -[DOWNLOADING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] [..] +[DOWNLOADED] [..] [COMPILING] [..] [COMPILING] [..] [COMPILING] [..] @@ -395,7 +397,8 @@ fn override_adds_some_deps() { "\ [UPDATING] `[ROOT][..]` index [UPDATING] git repository `[..]` -[DOWNLOADING] baz v0.1.1 (registry [..]) +[DOWNLOADING] crates ... +[DOWNLOADED] baz v0.1.1 (registry [..]) [COMPILING] baz v0.1.1 [COMPILING] bar v0.1.0 ([..]) [COMPILING] foo v0.0.1 ([CWD]) @@ -832,7 +835,8 @@ documented online at the url below for more information. https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#overriding-dependencies -[DOWNLOADING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] [..] [COMPILING] [..] [COMPILING] [..] [COMPILING] [..] diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index 930c9926748..82988034472 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -51,7 +51,8 @@ fn replace() { .with_stderr( "\ [UPDATING] `[ROOT][..]` index -[DOWNLOADING] baz v0.1.0 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] baz v0.1.0 ([..]) [COMPILING] bar v0.1.0 ([CWD]/bar) [COMPILING] baz v0.1.0 [COMPILING] foo v0.0.1 ([CWD]) @@ -217,7 +218,8 @@ fn unused() { .with_stderr( "\ [UPDATING] `[ROOT][..]` index -[DOWNLOADING] bar v0.1.0 [..] +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.1.0 [..] [COMPILING] bar v0.1.0 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] @@ -275,7 +277,8 @@ fn unused_git() { "\ [UPDATING] git repository `file://[..]` [UPDATING] `[ROOT][..]` index -[DOWNLOADING] bar v0.1.0 [..] +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.1.0 [..] [COMPILING] bar v0.1.0 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] @@ -309,7 +312,8 @@ fn add_patch() { .with_stderr( "\ [UPDATING] `[ROOT][..]` index -[DOWNLOADING] bar v0.1.0 [..] +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.1.0 [..] [COMPILING] bar v0.1.0 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] @@ -368,7 +372,8 @@ fn add_ignored_patch() { .with_stderr( "\ [UPDATING] `[ROOT][..]` index -[DOWNLOADING] bar v0.1.0 [..] +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.1.0 [..] [COMPILING] bar v0.1.0 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] @@ -535,7 +540,8 @@ fn new_major() { .with_stderr( "\ [UPDATING] `[ROOT][..]` index -[DOWNLOADING] bar v0.2.0 [..] +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.2.0 [..] [COMPILING] bar v0.2.0 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 4d9fd5d8882..0e0222504f0 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -40,7 +40,8 @@ fn simple() { .with_stderr(&format!( "\ [UPDATING] `{reg}` index -[DOWNLOADING] bar v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.1 (registry `[ROOT][..]`) [COMPILING] bar v0.0.1 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -85,8 +86,9 @@ fn deps() { .with_stderr(&format!( "\ [UPDATING] `{reg}` index -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) [COMPILING] baz v0.0.1 [COMPILING] bar v0.0.1 [COMPILING] foo v0.0.1 ([CWD]) @@ -265,7 +267,8 @@ fn bad_cksum() { .with_stderr( "\ [UPDATING] [..] index -[DOWNLOADING] bad-cksum [..] +[DOWNLOADING] crates ... +[DOWNLOADED] bad-cksum [..] [ERROR] failed to download replaced source registry `https://[..]` Caused by: @@ -309,7 +312,8 @@ required by package `foo v0.0.1 ([..])` .with_stderr(format!( "\ [UPDATING] `{reg}` index -[DOWNLOADING] notyet v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] notyet v0.0.1 (registry `[ROOT][..]`) [COMPILING] notyet v0.0.1 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -364,7 +368,8 @@ required by package `foo v0.0.1 ([..])` [PACKAGING] foo v0.0.1 ([CWD]) [VERIFYING] foo v0.0.1 ([CWD]) [UPDATING] `[..]` index -[DOWNLOADING] notyet v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] notyet v0.0.1 (registry `[ROOT][..]`) [COMPILING] notyet v0.0.1 [COMPILING] foo v0.0.1 ([CWD][..]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -395,7 +400,8 @@ fn lockfile_locks() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] bar v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.1 (registry `[ROOT][..]`) [COMPILING] bar v0.0.1 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -432,8 +438,9 @@ fn lockfile_locks_transitively() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) [COMPILING] baz v0.0.1 [COMPILING] bar v0.0.1 [COMPILING] foo v0.0.1 ([CWD]) @@ -477,8 +484,9 @@ fn yanks_are_not_used() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) [COMPILING] baz v0.0.1 [COMPILING] bar v0.0.1 [COMPILING] foo v0.0.1 ([CWD]) @@ -584,7 +592,8 @@ fn update_with_lockfile_if_packages_missing() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] bar v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.1 (registry `[ROOT][..]`) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s ", ).run(); @@ -627,7 +636,8 @@ fn update_lockfile() { p.cargo("build") .with_stderr( "\ -[DOWNLOADING] [..] v0.0.2 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.2 (registry `[ROOT][..]`) [COMPILING] bar v0.0.2 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -647,7 +657,8 @@ fn update_lockfile() { p.cargo("build") .with_stderr( "\ -[DOWNLOADING] [..] v0.0.3 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.3 (registry `[ROOT][..]`) [COMPILING] bar v0.0.3 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -725,7 +736,8 @@ fn dev_dependency_not_used() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] [..] v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.1 (registry `[ROOT][..]`) [COMPILING] bar v0.0.1 [COMPILING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -809,7 +821,8 @@ fn updating_a_dep() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] bar v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.1 (registry `[ROOT][..]`) [COMPILING] bar v0.0.1 [COMPILING] a v0.0.1 ([CWD]/a) [COMPILING] foo v0.0.1 ([CWD]) @@ -835,7 +848,8 @@ fn updating_a_dep() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] bar v0.1.0 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.1.0 (registry `[ROOT][..]`) [COMPILING] bar v0.1.0 [COMPILING] a v0.0.1 ([CWD]/a) [COMPILING] foo v0.0.1 ([CWD]) @@ -889,7 +903,8 @@ fn git_and_registry_dep() { "\ [UPDATING] [..] [UPDATING] [..] -[DOWNLOADING] a v0.0.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] a v0.0.1 (registry `[ROOT][..]`) [COMPILING] a v0.0.1 [COMPILING] b v0.0.1 ([..]) [COMPILING] foo v0.0.1 ([CWD]) @@ -962,7 +977,8 @@ fn update_publish_then_update() { .with_stderr( "\ [UPDATING] [..] -[DOWNLOADING] a v0.1.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] a v0.1.1 (registry `[ROOT][..]`) [COMPILING] a v0.1.1 [COMPILING] foo v0.5.0 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -993,7 +1009,8 @@ fn fetch_downloads() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] a v0.1.0 (registry [..]) +[DOWNLOADING] crates ... +[DOWNLOADED] a v0.1.0 (registry [..]) ", ).run(); } @@ -1033,7 +1050,8 @@ fn update_transitive_dependency() { p.cargo("build") .with_stderr( "\ -[DOWNLOADING] b v0.1.1 (registry `[ROOT][..]`) +[DOWNLOADING] crates ... +[DOWNLOADED] b v0.1.1 (registry `[ROOT][..]`) [COMPILING] b v0.1.1 [COMPILING] a v0.1.0 [COMPILING] foo v0.5.0 ([..]) @@ -1136,9 +1154,9 @@ fn update_multiple_packages() { ).run(); p.cargo("build") - .with_stderr_contains("[DOWNLOADING] a v0.1.1 (registry `[ROOT][..]`)") - .with_stderr_contains("[DOWNLOADING] b v0.1.1 (registry `[ROOT][..]`)") - .with_stderr_contains("[DOWNLOADING] c v0.1.1 (registry `[ROOT][..]`)") + .with_stderr_contains("[DOWNLOADED] a v0.1.1 (registry `[ROOT][..]`)") + .with_stderr_contains("[DOWNLOADED] b v0.1.1 (registry `[ROOT][..]`)") + .with_stderr_contains("[DOWNLOADED] c v0.1.1 (registry `[ROOT][..]`)") .with_stderr_contains("[COMPILING] a v0.1.1") .with_stderr_contains("[COMPILING] b v0.1.1") .with_stderr_contains("[COMPILING] c v0.1.1") @@ -1263,7 +1281,8 @@ fn only_download_relevant() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] baz v0.1.0 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] baz v0.1.0 ([..]) [COMPILING] baz v0.1.0 [COMPILING] bar v0.5.0 ([..]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s @@ -1506,7 +1525,8 @@ update to a fixed version or contact the upstream maintainer about this warning. [UPDATING] [..] -[DOWNLOADING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] [..] [COMPILING] [..] [COMPILING] [..] [FINISHED] [..] @@ -1551,7 +1571,8 @@ fn old_version_req_upstream() { .with_stderr( "\ [UPDATING] [..] -[DOWNLOADING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] [..] warning: parsed version requirement `0.2*` is no longer valid Previous versions of Cargo accepted this malformed requirement, @@ -1658,7 +1679,8 @@ fn bad_and_or_malicious_packages_rejected() { .with_stderr( "\ [UPDATING] [..] -[DOWNLOADING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] [..] error: failed to download [..] Caused by: diff --git a/tests/testsuite/rename_deps.rs b/tests/testsuite/rename_deps.rs index fce7f2249e2..7f996f696f1 100644 --- a/tests/testsuite/rename_deps.rs +++ b/tests/testsuite/rename_deps.rs @@ -248,7 +248,8 @@ fn rename_twice() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] foo v0.1.0 (registry [..]) +[DOWNLOADING] crates ... +[DOWNLOADED] foo v0.1.0 (registry [..]) error: multiple dependencies listed for the same crate must all have the same \ name, but the dependency on `foo v0.1.0` is listed as having different names ", diff --git a/tests/testsuite/support/mod.rs b/tests/testsuite/support/mod.rs index b217527b8b8..497610caf67 100644 --- a/tests/testsuite/support/mod.rs +++ b/tests/testsuite/support/mod.rs @@ -1389,6 +1389,7 @@ fn substitute_macros(input: &str) -> String { ("[DOCTEST]", " Doc-tests"), ("[PACKAGING]", " Packaging"), ("[DOWNLOADING]", " Downloading"), + ("[DOWNLOADED]", " Downloaded"), ("[UPLOADING]", " Uploading"), ("[VERIFYING]", " Verifying"), ("[ARCHIVING]", " Archiving"), diff --git a/tests/testsuite/warn_on_failure.rs b/tests/testsuite/warn_on_failure.rs index b8019e69290..f4f8abae093 100644 --- a/tests/testsuite/warn_on_failure.rs +++ b/tests/testsuite/warn_on_failure.rs @@ -59,7 +59,8 @@ fn no_warning_on_success() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] bar v0.0.1 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.1 ([..]) [COMPILING] bar v0.0.1 [COMPILING] foo v0.0.1 ([..]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] @@ -79,7 +80,7 @@ fn no_warning_on_bin_failure() { .with_stderr_does_not_contain(&format!("[WARNING] {}", WARNING1)) .with_stderr_does_not_contain(&format!("[WARNING] {}", WARNING2)) .with_stderr_contains("[UPDATING] `[..]` index") - .with_stderr_contains("[DOWNLOADING] bar v0.0.1 ([..])") + .with_stderr_contains("[DOWNLOADED] bar v0.0.1 ([..])") .with_stderr_contains("[COMPILING] bar v0.0.1") .with_stderr_contains("[COMPILING] foo v0.0.1 ([..])") .run(); @@ -96,7 +97,7 @@ fn warning_on_lib_failure() { .with_stderr_does_not_contain("hidden stderr") .with_stderr_does_not_contain("[COMPILING] foo v0.0.1 ([..])") .with_stderr_contains("[UPDATING] `[..]` index") - .with_stderr_contains("[DOWNLOADING] bar v0.0.1 ([..])") + .with_stderr_contains("[DOWNLOADED] bar v0.0.1 ([..])") .with_stderr_contains("[COMPILING] bar v0.0.1") .with_stderr_contains(&format!("[WARNING] {}", WARNING1)) .with_stderr_contains(&format!("[WARNING] {}", WARNING2)) diff --git a/tests/testsuite/workspaces.rs b/tests/testsuite/workspaces.rs index 4e46aa0693a..9e6674698a6 100644 --- a/tests/testsuite/workspaces.rs +++ b/tests/testsuite/workspaces.rs @@ -557,7 +557,8 @@ fn share_dependencies() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] dep1 v0.1.3 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] dep1 v0.1.3 ([..]) [COMPILING] dep1 v0.1.3 [COMPILING] foo v0.1.0 ([..]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] @@ -600,7 +601,8 @@ fn fetch_fetches_all() { .with_stderr( "\ [UPDATING] `[..]` index -[DOWNLOADING] dep1 v0.1.3 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] dep1 v0.1.3 ([..]) ", ).run(); } @@ -650,7 +652,8 @@ fn lock_works_for_everyone() { p.cargo("build") .with_stderr( "\ -[DOWNLOADING] dep2 v0.1.0 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] dep2 v0.1.0 ([..]) [COMPILING] dep2 v0.1.0 [COMPILING] foo v0.1.0 ([..]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] @@ -661,7 +664,8 @@ fn lock_works_for_everyone() { .cwd(p.root().join("bar")) .with_stderr( "\ -[DOWNLOADING] dep1 v0.1.0 ([..]) +[DOWNLOADING] crates ... +[DOWNLOADED] dep1 v0.1.0 ([..]) [COMPILING] dep1 v0.1.0 [COMPILING] bar v0.1.0 ([..]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] From 78922ca84f18e26f3564b9e6e481bfa62cae550b Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 17 Sep 2018 10:37:44 -0700 Subject: [PATCH 09/15] Handle callbacks invoked outside of `tls::set` This'll happen apparently when we drop items, and no need to keep tracking information there! --- src/cargo/core/package.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index f4ccc000bcd..c2c39ea598f 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -417,7 +417,11 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { handle.write_function(move |buf| { debug!("{} - {} bytes of data", token, buf.len()); tls::with(|downloads| { - downloads.pending[&token].0.data.borrow_mut().extend_from_slice(buf); + if let Some(downloads) = downloads { + downloads.pending[&token].0.data + .borrow_mut() + .extend_from_slice(buf); + } }); Ok(buf.len()) })?; @@ -425,6 +429,10 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { handle.progress(true)?; handle.progress_function(move |dl_total, dl_cur, _, _| { tls::with(|downloads| { + let downloads = match downloads { + Some(d) => d, + None => return false, + }; let dl = &downloads.pending[&token].0; dl.total.set(dl_total as u64); dl.current.set(dl_cur as u64); @@ -663,11 +671,14 @@ mod tls { thread_local!(static PTR: Cell = Cell::new(0)); - pub(crate) fn with(f: impl FnOnce(&Downloads) -> R) -> R { + pub(crate) fn with(f: impl FnOnce(Option<&Downloads>) -> R) -> R { let ptr = PTR.with(|p| p.get()); - assert!(ptr != 0); - unsafe { - f(&*(ptr as *const Downloads)) + if ptr == 0 { + f(None) + } else { + unsafe { + f(Some(&*(ptr as *const Downloads))) + } } } From 304871c79346b878b276ddfda4dc854dd70e7afe Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 17 Sep 2018 11:42:45 -0700 Subject: [PATCH 10/15] Tweaks to retry logic * Don't use `chain_err` internally inside `try` because then the spurious error printed internally is bland and doesn't contain the right error information. Instead place the `chain_err` on the outside so it propagates up to the user still but doesn't get printed for spurious errors. * Handle `pending_ids` management in the case of an error on the connection. --- src/cargo/core/package.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index c2c39ea598f..3e9585d2838 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -489,15 +489,14 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { .expect("got a token for a non-in-progress transfer"); let data = mem::replace(&mut *dl.data.borrow_mut(), Vec::new()); let mut handle = self.set.multi.remove(handle)?; + self.pending_ids.remove(&dl.id); // Check if this was a spurious error. If it was a spurious error // then we want to re-enqueue our request for another attempt and // then we wait for another request to finish. let ret = { self.retry.try(|| { - result.chain_err(|| { - format!("failed to download from `{}`", dl.url) - })?; + result?; let code = handle.response_code()?; if code != 200 && code != 0 { @@ -508,14 +507,18 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { }.into()) } Ok(()) + }).chain_err(|| { + format!("failed to download from `{}`", dl.url) })? }; - if ret.is_some() { - break (dl, data) + match ret { + Some(()) => break (dl, data), + None => { + self.pending_ids.insert(dl.id.clone()); + self.enqueue(dl, handle)? + } } - self.enqueue(dl, handle)?; }; - self.pending_ids.remove(&dl.id); // If the progress bar isn't enabled then we still want to provide some // semblance of progress of how we're downloading crates. From bc942919e358f870b7dd9db15dab70df5a93f09e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 17 Sep 2018 11:44:48 -0700 Subject: [PATCH 11/15] Use `Message::result_for` for better error messages Recently fixed in the `curl` crate, this'll allow getting the full and complete error message for each transfer instead of just the error code. --- Cargo.toml | 2 +- src/cargo/core/package.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 51e5c8b4ca4..93eb7c29053 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ bytesize = "1.0" crates-io = { path = "src/crates-io", version = "0.20" } crossbeam-utils = "0.5" crypto-hash = "0.3.1" -curl = { version = "0.4.15", features = ['http2'] } +curl = { version = "0.4.17", features = ['http2'] } env_logger = "0.5.11" failure = "0.1.2" filetime = "0.2" diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 3e9585d2838..0f14b12972b 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -585,9 +585,11 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { })?; debug!("handles remaining: {}", n); let results = &mut self.results; + let pending = &self.pending; self.set.multi.messages(|msg| { let token = msg.token().expect("failed to read token"); - if let Some(result) = msg.result() { + let handle = &pending[&token].1; + if let Some(result) = msg.result_for(&handle) { results.push((token, result.map_err(|e| e.into()))); } else { debug!("message without a result (?)"); From 5a5b611aa25c3a84e40d8bbebee60ffc3458575e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 17 Sep 2018 11:59:10 -0700 Subject: [PATCH 12/15] Don't print a download summary if an error happens Otherwise it sort of just clutters things up! --- src/cargo/core/package.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 0f14b12972b..0f6f9dae95f 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -264,6 +264,7 @@ pub struct Downloads<'a, 'cfg: 'a> { downloaded_bytes: u64, largest: (u64, String), start: Instant, + success: bool, } struct Download { @@ -323,6 +324,7 @@ impl<'cfg> PackageSet<'cfg> { downloads_finished: 0, downloaded_bytes: 0, largest: (0, String::new()), + success: false, }) } @@ -341,6 +343,7 @@ impl<'cfg> PackageSet<'cfg> { while downloads.remaining() > 0 { pkgs.push(downloads.wait()?); } + downloads.success = true; Ok(pkgs) } @@ -651,9 +654,14 @@ impl<'a, 'cfg> Drop for Downloads<'a, 'cfg> { if !progress.is_enabled() { return } + // If we didn't download anything, no need for a summary if self.downloads_finished == 0 { return } + // If an error happened, let's not clutter up the output + if !self.success { + return + } let mut status = format!("{} crates ({}) in {}", self.downloads_finished, ByteSize(self.downloaded_bytes), From 2245fc40934f175385c97e60634d1f8c32c84b23 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 17 Sep 2018 11:59:37 -0700 Subject: [PATCH 13/15] Avoid a debug overflow from curl Curl doesn't guarantee that we'll get the total/current progress bars in a "as we expect" fashion, so throw out weird data points (such as we've downloaded more bytes than curl thinks we'll be downloading in total). --- src/cargo/core/package.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 0f6f9dae95f..cee7c709112 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -627,7 +627,12 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { let mut remaining = 0; for (dl, _) in self.pending.values() { dur += dl.start.elapsed(); - remaining += dl.total.get() - dl.current.get(); + // If the total/current look weird just throw out the data + // point, sounds like curl has more to learn before we have + // the true information. + if dl.total.get() >= dl.current.get() { + remaining += dl.total.get() - dl.current.get(); + } } if remaining > 0 && dur > Duration::from_millis(500) { msg.push_str(&format!(", remaining bytes: {}", ByteSize(remaining))); From 9da2e70820fc8f67517adf284b6fbda2662aaac9 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 18 Sep 2018 11:37:55 -0700 Subject: [PATCH 14/15] Don't enable HTTP1 pipelining as apparently it's flaky It seems to fix some issues perhaps! --- src/cargo/core/package.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index cee7c709112..ab20af44ae8 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -287,7 +287,7 @@ impl<'cfg> PackageSet<'cfg> { // We've enabled the `http2` feature of `curl` in Cargo, so treat // failures here as fatal as it would indicate a build-time problem. let mut multi = Multi::new(); - multi.pipelining(true, true) + multi.pipelining(false, true) .chain_err(|| "failed to enable multiplexing/pipelining in curl")?; Ok(PackageSet { From f4dcb5c1bb0188b539632106503c559c3b532637 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 18 Sep 2018 14:57:10 -0700 Subject: [PATCH 15/15] Disable multiplexing by default for now Let's hopefully weed out any configuration issues before turning it on by default! --- src/cargo/core/package.rs | 22 +++++++++++++++++++--- src/doc/src/reference/config.md | 1 + 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index ab20af44ae8..c6ec57c3137 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -250,6 +250,7 @@ pub struct PackageSet<'cfg> { config: &'cfg Config, multi: Multi, downloading: Cell, + multiplexing: bool, } pub struct Downloads<'a, 'cfg: 'a> { @@ -286,10 +287,22 @@ impl<'cfg> PackageSet<'cfg> { ) -> CargoResult> { // We've enabled the `http2` feature of `curl` in Cargo, so treat // failures here as fatal as it would indicate a build-time problem. + // + // Note that the multiplexing support is pretty new so we're having it + // off-by-default temporarily. + // + // Also note that pipelining is disabled as curl authors have indicated + // that it's buggy, and we've empirically seen that it's buggy with HTTP + // proxies. let mut multi = Multi::new(); - multi.pipelining(false, true) + let multiplexing = config.get::>("http.multiplexing")? + .unwrap_or(false); + multi.pipelining(false, multiplexing) .chain_err(|| "failed to enable multiplexing/pipelining in curl")?; + // let's not flood crates.io with connections + multi.set_max_host_connections(2)?; + Ok(PackageSet { packages: package_ids .iter() @@ -299,6 +312,7 @@ impl<'cfg> PackageSet<'cfg> { config, multi, downloading: Cell::new(false), + multiplexing, }) } @@ -405,8 +419,10 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { // downloads much faster. Currently Cargo requests the `http2` feature // of the `curl` crate which means it should always be built in, so // treat it as a fatal error of http/2 support isn't found. - handle.http_version(HttpVersion::V2) - .chain_err(|| "failed to enable HTTP2, is curl not built right?")?; + if self.set.multiplexing { + handle.http_version(HttpVersion::V2) + .chain_err(|| "failed to enable HTTP2, is curl not built right?")?; + } // This is an option to `libcurl` which indicates that if there's a // bunch of parallel requests to the same host they all wait until the diff --git a/src/doc/src/reference/config.md b/src/doc/src/reference/config.md index 91425164b0c..c539d4f6a86 100644 --- a/src/doc/src/reference/config.md +++ b/src/doc/src/reference/config.md @@ -101,6 +101,7 @@ timeout = 30 # Timeout for each HTTP request, in seconds cainfo = "cert.pem" # Path to Certificate Authority (CA) bundle (optional) check-revoke = true # Indicates whether SSL certs are checked for revocation low-speed-limit = 5 # Lower threshold for bytes/sec (10 = default, 0 = disabled) +multiplexing = false # whether or not to use HTTP/2 multiplexing where possible [build] jobs = 1 # number of parallel jobs, defaults to # of CPUs