Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Abandon cURL #1657

Closed
wants to merge 1 commit into from
Closed

Abandon cURL #1657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,208 changes: 634 additions & 574 deletions Cargo.lock
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 0 additions & 4 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@ build = "build.rs"

[features]

default = ["curl-backend", "reqwest-backend"]

curl-backend = ["download/curl-backend"]
reqwest-backend = ["download/reqwest-backend"]
vendored-openssl = ['openssl/vendored']

# Include in the default set to disable self-update and uninstall.
Expand Down
14 changes: 3 additions & 11 deletions src/download/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,12 @@ authors = [ "Brian Anderson <banderson@mozilla.com>" ]

license = "MIT/Apache-2.0"

[features]

default = ["curl-backend"]

curl-backend = ["curl"]
reqwest-backend = ["reqwest", "env_proxy", "lazy_static"]

[dependencies]
error-chain = "0.12"
url = "1.7"
curl = { version = "0.4.11", optional = true }
env_proxy = { version = "0.2.0", optional = true }
lazy_static = { version = "1.0", optional = true }
reqwest = { version = "0.9", optional = true }
env_proxy = "0.2.0"
lazy_static = "1.0"
reqwest = "0.9"

[dev-dependencies]
futures = "0.1"
Expand Down
4 changes: 0 additions & 4 deletions src/download/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,5 @@ error_chain! {
FileNotFound {
description("file not found")
}
BackendUnavailable(be: &'static str) {
description("download backend unavailable")
display("download backend '{}' unavailable", be)
}
}
}
190 changes: 4 additions & 186 deletions src/download/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,6 @@ use url::Url;
mod errors;
pub use crate::errors::*;

#[derive(Debug, Copy, Clone)]
pub enum Backend {
Curl,
Reqwest,
}

#[derive(Debug, Copy, Clone)]
pub enum Event<'a> {
ResumingPartialDownload,
Expand All @@ -21,20 +15,7 @@ pub enum Event<'a> {
DownloadDataReceived(&'a [u8]),
}

fn download_with_backend(
backend: Backend,
url: &Url,
resume_from: u64,
callback: &dyn Fn(Event<'_>) -> Result<()>,
) -> Result<()> {
match backend {
Backend::Curl => curl::download(url, resume_from, callback),
Backend::Reqwest => reqwest_be::download(url, resume_from, callback),
}
}

pub fn download_to_path_with_backend(
backend: Backend,
pub fn download_to_path(
url: &Url,
path: &Path,
resume_from_partial: bool,
Expand Down Expand Up @@ -94,7 +75,7 @@ pub fn download_to_path_with_backend(

let file = RefCell::new(file);

download_with_backend(backend, url, resume_from, &|event| {
reqwest_be::download(url, resume_from, &|event| {
if let Event::DownloadDataReceived(data) = event {
file.borrow_mut()
.write_all(data)
Expand All @@ -118,133 +99,6 @@ pub fn download_to_path_with_backend(
})
}

/// Download via libcurl; encrypt with the native (or OpenSSl) TLS
/// stack via libcurl
#[cfg(feature = "curl-backend")]
pub mod curl {

use curl;

use self::curl::easy::Easy;
use super::Event;
use crate::errors::*;
use std::cell::RefCell;
use std::str;
use std::time::Duration;
use url::Url;

pub fn download(
url: &Url,
resume_from: u64,
callback: &dyn Fn(Event<'_>) -> Result<()>,
) -> Result<()> {
// Fetch either a cached libcurl handle (which will preserve open
// connections) or create a new one if it isn't listed.
//
// Once we've acquired it, reset the lifetime from 'static to our local
// scope.
thread_local!(static EASY: RefCell<Easy> = RefCell::new(Easy::new()));
EASY.with(|handle| {
let mut handle = handle.borrow_mut();

handle
.url(&url.to_string())
.chain_err(|| "failed to set url")?;
handle
.follow_location(true)
.chain_err(|| "failed to set follow redirects")?;

if resume_from > 0 {
handle
.resume_from(resume_from)
.chain_err(|| "setting the range header for download resumption")?;
} else {
// an error here indicates that the range header isn't supported by underlying curl,
// so there's nothing to "clear" - safe to ignore this error.
let _ = handle.resume_from(0);
}

// Take at most 30s to connect
handle
.connect_timeout(Duration::new(30, 0))
.chain_err(|| "failed to set connect timeout")?;

{
let cberr = RefCell::new(None);
let mut transfer = handle.transfer();

// Data callback for libcurl which is called with data that's
// downloaded. We just feed it into our hasher and also write it out
// to disk.
transfer
.write_function(|data| match callback(Event::DownloadDataReceived(data)) {
Ok(()) => Ok(data.len()),
Err(e) => {
*cberr.borrow_mut() = Some(e);
Ok(0)
}
})
.chain_err(|| "failed to set write")?;

// Listen for headers and parse out a `Content-Length` (case-insensitive) if it
// comes so we know how much we're downloading.
transfer
.header_function(|header| {
if let Ok(data) = str::from_utf8(header) {
let prefix = "content-length: ";
if data.to_ascii_lowercase().starts_with(prefix) {
if let Ok(s) = data[prefix.len()..].trim().parse::<u64>() {
let msg = Event::DownloadContentLengthReceived(s + resume_from);
match callback(msg) {
Ok(()) => (),
Err(e) => {
*cberr.borrow_mut() = Some(e);
return false;
}
}
}
}
}
true
})
.chain_err(|| "failed to set header")?;

// If an error happens check to see if we had a filesystem error up
// in `cberr`, but we always want to punt it up.
transfer.perform().or_else(|e| {
// If the original error was generated by one of our
// callbacks, return it.
match cberr.borrow_mut().take() {
Some(cberr) => Err(cberr),
None => {
// Otherwise, return the error from curl
if e.is_file_couldnt_read_file() {
Err(e).chain_err(|| ErrorKind::FileNotFound)
} else {
Err(e).chain_err(|| "error during download")
}
}
}
})?;
}

// If we didn't get a 20x or 0 ("OK" for files) then return an error
let code = handle
.response_code()
.chain_err(|| "failed to get response code")?;
match code {
0 | 200..=299 => {}
_ => {
return Err(ErrorKind::HttpStatus(code).into());
}
};

Ok(())
})
}
}

#[cfg(feature = "reqwest-backend")]
pub mod reqwest_be {
use super::Event;
use crate::errors::*;
Expand Down Expand Up @@ -302,12 +156,8 @@ pub mod reqwest_be {
.build()
};

// woah, an unwrap?!
// It's OK. This is the same as what is happening in curl.
//
// The curl::Easy::new() internally assert!s that the initialized
// Easy is not null. Inside reqwest, the errors here would be from
// the TLS library returning a null pointer as well.
// Inside reqwest, the errors here would be from the TLS library
// returning a null pointer.
catcher().unwrap()
};
}
Expand Down Expand Up @@ -366,35 +216,3 @@ pub mod reqwest_be {
}
}
}

#[cfg(not(feature = "curl-backend"))]
pub mod curl {

use super::Event;
use errors::*;
use url::Url;

pub fn download(
_url: &Url,
_resume_from: u64,
_callback: &Fn(Event) -> Result<()>,
) -> Result<()> {
Err(ErrorKind::BackendUnavailable("curl").into())
}
}

#[cfg(not(feature = "reqwest-backend"))]
pub mod reqwest_be {

use super::Event;
use errors::*;
use url::Url;

pub fn download(
_url: &Url,
_resume_from: u64,
_callback: &Fn(Event) -> Result<()>,
) -> Result<()> {
Err(ErrorKind::BackendUnavailable("reqwest").into())
}
}
76 changes: 0 additions & 76 deletions src/download/tests/download-curl-resume.rs
View file
Open in desktop

This file was deleted.

15 changes: 4 additions & 11 deletions src/rustup-utils/src/notifications.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ pub enum Notification<'a> {
DownloadFinished,
NoCanonicalPath(&'a Path),
ResumingPartialDownload,
UsingCurl,
UsingReqwest,
UsingHyperDeprecated,
}

Expand All @@ -36,9 +34,7 @@ impl<'a> Notification<'a> {
| DownloadContentLengthReceived(_)
| DownloadDataReceived(_)
| DownloadFinished
| ResumingPartialDownload
| UsingCurl
| UsingReqwest => NotificationLevel::Verbose,
| ResumingPartialDownload => NotificationLevel::Verbose,
UsingHyperDeprecated | NoCanonicalPath(_) => NotificationLevel::Warn,
}
}
Expand All @@ -62,12 +58,9 @@ impl<'a> Display for Notification<'a> {
DownloadFinished => write!(f, "download finished"),
NoCanonicalPath(path) => write!(f, "could not canonicalize path: '{}'", path.display()),
ResumingPartialDownload => write!(f, "resuming partial download"),
UsingCurl => write!(f, "downloading with curl"),
UsingReqwest => write!(f, "downloading with reqwest"),
UsingHyperDeprecated => f.write_str(
"RUSTUP_USE_HYPER environment variable is deprecated,\
use RUSTUP_USE_REQWEST instead",
),
UsingHyperDeprecated => {
f.write_str("RUSTUP_USE_HYPER environment variable is deprecated.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong feeling either way, but: should there be a warning about RUSTUP_USE_REQWEST being deprecated as well, as it is now the only option? If not, should the RUSTUP_USE_HYPER warning be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I think you're right that there should be some kind of warning. I'll add that in.

}
}
}
}
Loading