Abandon cURL

[das-sein]

Part of the simplification project #1611

Abandons the use of cURL entirely in favor of reqwest.

[das-sein]

Failure is due to Cargo.lock not being updated. Should I add that to the commit? @nrc @kinnison Wasn't sure if that's an okay thing to do on my side.

[dwijnand]

I'm not sure why --locked is used, but I think you should update it so CI validation passes, and then we can see if it's right?

[das-sein]

@kinnison @dwijnand Added Cargo.lock

[dwijnand]

The patch looks good to me. The question is whether dropping curl in favour of reqwest is the right decision?

[briansmith]

The patch looks good to me. The question is whether dumping curl in favour of reqwest is the right decision?

(I wish we weren't using words like "abandon" and "dumping" when talking about cURL since that's not very nice.)

I think replacing Curl with a Rust library is the right thing to do for both rhetorical (political) reasons and technical (security) reasons. So, I think the question isn't "whether" we should do something like this but what, if anything, do we need to do to make sure reqwest is a reasonable choice? The good thing about reqwest is that it is based on libraries that are widely used in production.

[das-sein]

@briansmith Sorry! I meant it in a totally non-emotional, literal way.

[jnicholls]

This looks good to me!

[seanmonstar]

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?

[das-sein]

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

[brson]

It looks to me like prior to this patch curl was the default, so pretty much everybody has been using the curl backend. Switching to reqwest while also dropping the curl backend is risky. I recall the reqwest backend not working in some user scenarios (it's possible reqwest was even the default once and had to be switched back but I don't recall).

I'd suggest not doing this now, and instead turning on reqwest by default, releasing a new build, mentioning loudly in the release notes that the backend has changed, please report bugs, here's how to work around any problem (by requesting the curl backend). Wait a reasonable amount of time, then do this patch.

[brson]

I'm not sure why --locked is used, but I think you should update it so CI validation passes, and then we can see if it's right?

It's so that Cargo.lock is kept updated, so that the release uses the same versions as the developers. Seems like a best practice to me; I don't see a downside.

[dwijnand]

Yeah, makes sense to me. Just not used to it - in the cargo repo, even though it's also a binary tool, we're submoduled in, so we have no Cargo.lock.

[briansmith]

I agree it makes sense to have a way to switch back to using curl, e.g. via an environment variable, for a release.

[das-sein]

@brson Excellent points. I'll begin working on a PR that deprecates cURL without entirely removing it, and keep this PR open and up to date until it's safe to proceed.

[bors]

☔ The latest upstream changes (presumably #1630) made this pull request unmergeable. Please resolve the merge conflicts.

[nrc]

Closing in favour of #1660

[briansmith]

@nrc #1660 is just a prerequisite to this, not a replacement for this.

[dwijnand]

@briansmith That's right. The idea is let's go ahead and land #1660 instead, first, and remove cURL entirely after some time.

[lnicola]

Is it time to drop the cURL backend?

[jnicholls]

It's been 7 months. That is probably long enough.

[lnicola]

Filed #2034 for this.