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.

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

Deprecate cURL #1660

Merged
merged 3 commits into from
Feb 27, 2019
Merged

Deprecate cURL #1660

merged 3 commits into from
Feb 27, 2019

Conversation

das-sein
Copy link
Contributor

Part of the simplification project: #1611

Deprecates cURL in favor of reqwest.

This is the preliminary step for the complete disuse of cURL by rustup; on-going discussion: #1657

cc's from previous PR:
@dwijnand
@brson
@briansmith
@jnicholls
@seanmonstar

@das-sein das-sein changed the title Deprecate curl Deprecate cURL Feb 13, 2019
@dwijnand
Copy link
Member

I don't think RUSTUP_USE_CURL should be introduced and on usage it emits a warning message. I think the old UsingCurl notification should remain.

@das-sein
Copy link
Contributor Author

@dwijnand Does this mean we should have the user compile themselves with the default feature in the Cargo.toml set to cURL if they want to use it?

@dwijnand
Copy link
Member

No, no compilation necessary, as the rustup binary crate depends on both backends/features:

https://github.com/rust-lang/rustup.rs/blob/194026d2cac3d06c8ef19dc6e23dabda5d3900c4/Cargo.toml#L23

@das-sein
Copy link
Contributor Author

How would a user enable one backend over the other without some kind of environment variable? I think I'm missing something here.

@dwijnand
Copy link
Member

Ah, I see the confusion. I meant we should introduce RUSTUP_USE_CURL but, if a user sets it, it shouldn't emit a deprecation warning message. Just "downloading with curl".

@das-sein das-sein force-pushed the deprecate-curl branch 2 times, most recently from f043963 to b5bb7f1 Compare February 14, 2019 15:18
@das-sein
Copy link
Contributor Author

@dwijnand Removed the deprecation warning.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

This is good, though I'd prefer the odd bogon of a diff hunk was fixed.

src/rustup-utils/src/notifications.rs Show resolved Hide resolved
@das-sein
Copy link
Contributor Author

das-sein commented Feb 18, 2019

@kinnison Weird delta removed from commits.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

LGTM other than the Hyper thing

src/rustup-utils/src/notifications.rs Outdated Show resolved Hide resolved
@nrc nrc mentioned this pull request Feb 26, 2019
@dwijnand dwijnand dismissed kinnison’s stale review February 27, 2019 07:33

Odd bogon of a diff hunk fixed

@dwijnand dwijnand merged commit 6b89dff into rust-lang:master Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants