-
Notifications
You must be signed in to change notification settings - Fork 60
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
Recognize curl as a downloading tool #335
Conversation
6f97ab3
to
2410350
Compare
2410350
to
e47b69e
Compare
cc @BruceForstall, PTAL. |
This LGTM. Can some @dotnet/jit-contrib Unix guru review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. But I have one question that I asked inline
status="${response[1]}" | ||
fi | ||
|
||
if (( status >= 200 || status < 400 )); then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What statuses other than 200
and 301
can indicate that the url is valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2xx and 3xx classes indicate the success-like cases.
However, I just noticed that I am missing --location
(or -L
) below at curl invocations to indicate "follow redirect" in case of 301, 308 etc. (wget follows redirects by default, but curl requires -L).
@echesakovMSFT Any more feedback here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Chances of having curl on a fresh Linux / macOS install are actually higher than having wget, but even if there is a tie, using available tool in PATH which does the job right is good. This is the only script related to runtime that force the user to install wget.
.. was about to run
brew install wget
then thought I should do this first. 😄