-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
make bootstrap use wget if available #128459
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kobzol (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
This comment has been minimized.
This comment has been minimized.
Some changes occurred in src/tools/cargo cc @ehuss |
whoops, didn't mean to touch that submodule |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Hi, thanks for the PR! Just out of curiosity, if you're having issues with network connection, wouldn't wget -c
be useful for you? With that flag wget could actually resume interrupted downloads.
doesn't just swapping in wget seemed to be enough in my experience. |
Yes, it only matters when you want to resume, I thought that is the situation that you want to improve. So if I understand it correctly, when curl retries a download, it starts from the beginning, but wget restart it where it ended? Overall, I'm fine with using wget, although I think that we should try to copy some of the options from the curl invocation. For example, there is no connection timeout in wget by default, but for curl we set it to 30s. We should try to get these options for wget and curl to be similar-ish. |
as i understand it, wget will automatically reconnect and resume by default, and you only need to use |
I see. Yeah, that makes sense, ok. Could you please try to configure the wget invocations to have more similar options as the curl invocations, particularly around (connection) timeouts, retry counts and similar? |
This comment has been minimized.
This comment has been minimized.
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.
Thank you! Could you now please squash the commits? There are a lot of tmp/WIP commits in there now.
@onur-ozkan Any concerns against using wget
as the default downloader if it is available? I'd personally merge this and if people encounter some issues with wget, we can e.g. hide it behind an opt-in env. var. flag.
Would passing |
Some of our CI runners are using very old versions of curl and
If we use recent features of |
I suppose that the easiest way to check is just to try to merge this. Could you please squash the commits? Thanks. |
this improves reliability when downloading over unreliable connections.
Okay, let's try. @bors r+ rollup=never |
make bootstrap use wget if available this improves reliability when downloading over unreliable connections. without this change, it was basically impossible for me to run the bootstrap tests.
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
Hmm, unfortunate. I wonder if we could just test if |
That flag is quite recent feature for
Makes sense. |
I didn't mean trying the flag for CI, but for local usage (on CI the flag would fail), assuming that recent distros already have new enough curl. |
Actually, it seems that the flag was introduced in early versions of curl. Even our CI runners use newer versions than that (as far as I remember, the major version was either 6 or 7 but definitely not lower or higher). We could try using it unconditionally (as there is no distro using that old version). |
@lolbinarycat If you're able to replicate your network issues, could you please try if the |
@Kobzol --continue-at works, but only in conjunction with |
Oh, shoot. That is sadly a quite new flag. |
We can check the curl version and enable it only if it's newer than 7.70.0 (as it was introduced with 7.71.0). Then, we can close #110178 as well. |
alternative to rust-lang#128459 fixes rust-lang#110178
alternative to rust-lang#128459 fixes rust-lang#110178
alternative to rust-lang#128459 fixes rust-lang#110178
alternative to rust-lang#128459 fixes rust-lang#110178
Closing this one as we are landing #129134. |
i believe this actually improves the documentation of what the existing curl flags do, and those comments never got added to the accepted PR should i add those comments in a seperate PR? or is a whole pr for a couple of comments too frivolous? |
Making a separate PR sounds good to me. |
bootstrap: improve error recovery flags to curl alternative to rust-lang#128459 fixes rust-lang#110178 r? `@Kobzol`
bootstrap: improve error recovery flags to curl alternative to rust-lang#128459 fixes rust-lang#110178 r? ``@Kobzol``
bootstrap: improve error recovery flags to curl alternative to rust-lang#128459 fixes rust-lang#110178 r? ```@Kobzol```
bootstrap: improve error recovery flags to curl alternative to rust-lang#128459 fixes rust-lang#110178 r? ````@Kobzol````
Rollup merge of rust-lang#129134 - lolbinarycat:continue-at, r=Kobzol bootstrap: improve error recovery flags to curl alternative to rust-lang#128459 fixes rust-lang#110178 r? ````@Kobzol````
this improves reliability when downloading over unreliable connections.
without this change, it was basically impossible for me to run the bootstrap tests.