-
Notifications
You must be signed in to change notification settings - Fork 893
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
Update rustup-init to detect tls1.2 for curl 7.73+ (Fixes #2603) #2604
Update rustup-init to detect tls1.2 for curl 7.73+ (Fixes #2603) #2604
Conversation
This fixes rust-lang#2603. In curl 7.73.0, the help format was changed so as to only show a summary when calling `curl --help`, while the old menu can be accessed as `curl --help all`. This change updates the `check_help_for` function to detect if the command being checked contains the curl 7.73.0+ language indicating the use of `--help all`. If this language is present, `check_help_for` will insert the `all` category after the `--help` flag, allowing the query to work. If the language is not present, then it will insert an empty string following the `--help` flag, maintaining the same behavior with pre-7.73 versions of curl.
We actually use the script in our CI, so it's tested in that way, thank you for trying though. The change looks sane to me, though obviously we lack anything running that curl version to test it on. I assume you've also tested it against a system which exhibits the problem? if not, could you do so? |
I have, yes. I'm currently on a system with |
Since I'm guessing that trusting some rando (me) on the internet saying "works on my machine!" probably isn't a very comfy level of confidence for merging a PR, I went ahead and wrote up a dockerfile so you can try it on your machine (edit: pay no attention to the duplicate
(This takes a hot second to build because it's building the latest curl from source; it also downloads the rustup-init from this PR.) Then you can build/run/test it with:
|
Thank you for that effort. My only other question is whether this will affect wget based systems? Can you also check that? While I can run docker, I'm not entirely au-fait with making my own so it may be easier for you to do that. |
Good check! Here's another dockerfile that doesn't include curl at all, and instead installs wget:
Installing rustup the same way inside the docker image, but this time with a wget-only system (can also check that the
|
Fantastic, thank you so much. This looks good to merge. |
This fixes #2603. In curl 7.73.0, the help format was changed so as to only show a summary when calling
curl --help
, while the old menu can be accessed ascurl --help all
. This change updates thecheck_help_for
function to detect if the command being checked contains the curl 7.73.0+ language indicating the use of--help all
.If this language is present,
check_help_for
will insert theall
category after the--help
flag, allowing the query to work. If the language is not present, then it will insert an empty string following the--help
flag, maintaining the same behavior with pre-7.73 versions of curl.Side Note: This is my first contribution to the rustup repository and I've tried to follow the contribution guidelines to the best of my ability; however, it seems that much of that document is relevant mainly for actual code changes to rustup itself, not to the init script. I still ran the test suite just in case one of those tests covered the init script somehow. Please let me know if there's anything else I need to do for this contribution.