Skip to content

build: do not reinstall master toolchain if it is up-to-date #4713

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 24, 2019
Merged

build: do not reinstall master toolchain if it is up-to-date #4713

merged 1 commit into from
Oct 24, 2019

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Oct 22, 2019

changelog: none

@tesuji tesuji force-pushed the no-reinstall-toolchain branch from 0515464 to f9d2a1f Compare October 22, 2019 08:49
@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 22, 2019
@matthiaskrgr
Copy link
Member

When I run git ls-remote https://github.com/rust-lang/rust master it can sometimes take several 10s of seconds to execute. Is anyone else getting that?

git ls-remote https://github.com/rust-lang/rust master  0,17s user 0,10s system 0% cpu 53,640 total

@phansch
Copy link
Contributor

phansch commented Oct 24, 2019 •
edited
Loading

When I run git ls-remote https://github.com/rust-lang/rust master it can sometimes take several 10s of seconds to execute. Is anyone else getting that?

git ls-remote https://github.com/rust-lang/rust master  0,17s user 0,10s system 0% cpu 53,640 total

Yes, I can also reproduce this. About 5% of the time it will take 10+ seconds to get the result. On my slow 20Mbps connection this PR is still a significant speed up currently.

Also considering that it isn't always that slow, I'd say it's fine to merge this?

@tesuji tesuji force-pushed the no-reinstall-toolchain branch from f9d2a1f to e780756 Compare October 24, 2019 09:37
@tesuji
Copy link
Contributor Author

tesuji commented Oct 24, 2019

Note that old commit used git : https://github.com/rust-lang/rust-clippy/blob/3fb497c8c860cb286ed59fa49bc1796086196697/setup-toolchain.sh.
Also, rustup-toolchain-install-master uses git internally when not providing rust commit: https://github.com/kennytm/rustup-toolchain-install-master/blob/bcfef70d00feb43fbaa865cbef005ca176883c11/src/main.rs#L297-L305

@bors
Copy link
Contributor

bors commented Oct 24, 2019

☔ The latest upstream changes (presumably #4650) made this pull request unmergeable. Please resolve the merge conflicts.

@tesuji tesuji force-pushed the no-reinstall-toolchain branch 3 times, most recently from 608e2d0 to 6193b36 Compare October 24, 2019 14:59
@tesuji
Copy link
Contributor Author

tesuji commented Oct 24, 2019

Ci passed

@phansch
Copy link
Contributor

phansch commented Oct 24, 2019

@bors r+ thanks!

@bors
Copy link
Contributor

bors commented Oct 24, 2019

📌 Commit 6193b36 has been approved by phansch

@bors
Copy link
Contributor

bors commented Oct 24, 2019

🌲 The tree is currently closed for pull requests below priority 10, this pull request will be tested once the tree is reopened

@phansch
Copy link
Contributor

phansch commented Oct 24, 2019

@bors treeclose-

@phansch
Copy link
Contributor

phansch commented Oct 24, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Oct 24, 2019

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Oct 24, 2019

📌 Commit 6193b36 has been approved by phansch

@bors
Copy link
Contributor

bors commented Oct 24, 2019

🌲 The tree is currently closed for pull requests below priority 10, this pull request will be tested once the tree is reopened

@phansch
Copy link
Contributor

phansch commented Oct 24, 2019

@bors treeclosed-

bors added a commit that referenced this pull request Oct 24, 2019
@bors
Auto merge of #4713 - lzutao:no-reinstall-toolchain, r=phansch
37ea436 
build: do not reinstall master toolchain if it is up-to-date

changelog: none
@bors
Copy link
Contributor

bors commented Oct 24, 2019

⌛ Testing commit 6193b36 with merge 37ea436...

@bors
Copy link
Contributor

bors commented Oct 24, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing 37ea436 to master...

@bors bors merged commit 6193b36 into rust-lang:master Oct 24, 2019
@tesuji tesuji deleted the no-reinstall-toolchain branch October 24, 2019 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

+1 more reviewer

@phansch phansch phansch approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tesuji @matthiaskrgr @phansch @bors @flip1995