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

boostrap.py use curl by default #96602

Merged
merged 1 commit into from
May 15, 2022
Merged

boostrap.py use curl by default #96602

merged 1 commit into from
May 15, 2022

Conversation

TApplencourt
Copy link
Contributor

@TApplencourt TApplencourt commented May 1, 2022

Fixes #61611

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2022
@jyn514
Copy link
Member

jyn514 commented May 1, 2022

Please also modify the downloads in src/bootstrap/native.rs to use the same fallback.

@rust-log-analyzer

This comment has been minimized.

@TApplencourt
Copy link
Contributor Author

TApplencourt commented May 1, 2022

Thanks a lot for the quick feedback!

  • Fixed python tidy
  • Trying to fix native.rs. Sorry for the stupid question, but I was not able to tests my change of native.rs. For python I just did python bootstrap.py. May I ask how to do the same for native.rs? Sorry!

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented May 1, 2022

@TApplencourt you can test the rust changes by setting [llvm] download-ci-llvm = true in config.toml, then making sure it successfully downloads LLVM (just x.py check should do it, or x.py build if you want to be 100% sure rustc_llvm can link to it).

Sorry for the stupid question, but I was not able to tests my change of native.rs

Not a stupid question! Codebases tend to get more complicated over time 😅 it's perfectly fine to ask for help figuring out how they work :)

src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved
src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved
src/bootstrap/native.rs Outdated Show resolved Hide resolved
src/bootstrap/native.rs Outdated Show resolved Hide resolved
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Please also keep commits squashed. Thanks!

src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved
@TApplencourt
Copy link
Contributor Author

I think I addressed all the comments. Thanks a lot for the feedback!

I did some checks on my Linux machine; if curl is not present, we will check if we are on windows and if so try the PowerShell way.

I'm not sure about the naming of check_run (the try_run who never aborts). I can change it to any other more descriptive name if needed.

Thanks again for your time and your guidance!

@TApplencourt
Copy link
Contributor Author

@jyn514, @Mark-Simulacrum If you have time, I think this is ready to review!
Sorry, if I'm too pushy 🙏🏽

@Mark-Simulacrum Mark-Simulacrum changed the title boostrap.py use curl by default Fix #61611 boostrap.py use curl by default May 14, 2022
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 15, 2022

📌 Commit ad7dbe1 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2022
@bors
Copy link
Contributor

bors commented May 15, 2022

⌛ Testing commit ad7dbe1 with merge 0be8768...

@bors
Copy link
Contributor

bors commented May 15, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 0be8768 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 15, 2022
@bors bors merged commit 0be8768 into rust-lang:master May 15, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 15, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0be8768): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@TApplencourt TApplencourt deleted the patch-1 branch May 15, 2022 23:04
@TApplencourt
Copy link
Contributor Author

Thanks a lot y'all for the guidance on this one!

@jyn514
Copy link
Member

jyn514 commented May 16, 2022

Thank you for working on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x.py fails (timeout) to download components
8 participants