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.

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

Requires tools to test-pass if the corresponding submodule is updated. #47063

Merged
merged 1 commit into from
Dec 30, 2017
Merged

Requires tools to test-pass if the corresponding submodule is updated. #47063

merged 1 commit into from
Dec 30, 2017

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Dec 29, 2017 •
edited
Loading

Follow up of #46554. Breaking a tool would not stop bors from accepting a merge, but this also means when we intend to fix a tool but failed, bors will still accept the PR. This behavior is not helpful to the tool maintainers.

This PR attempts to fix this by rejecting the tool-update merge when a tool failed. It recognizes a PR as "tool-updating" when the corresponding submodule is changed, compared with the previous commit.

@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 29, 2017
@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@oli-obk
Copy link
Contributor

oli-obk commented Dec 29, 2017

Historically I seem to have used "reactivate/reenable". I had to check though, so this isn't really relevant.

Any reason we don't simply check for the names? So any PR with a toolname in its title needs to fix said tool?

@kennytm
Copy link
Member Author

kennytm commented Dec 29, 2017 •
edited
Loading

@oli-obk

Historically I seem to have used "reactivate/reenable".

I could add these two as well.

Any reason we don't simply check for the names?

To reduce false positives like #46960.

Or maybe we could check if the submodule is changed compared with the previous commit?

@est31
Copy link
Member

est31 commented Dec 29, 2017

I think the check can be improved by checking for changed submodules directly.

@est31
Copy link
Member

est31 commented Dec 29, 2017

On the auto branch, you can grep the output of git diff --name-status HEAD HEAD^ for submodule paths like src/tools/rls etc.

HEAD here will be the bors auto merge commit that is being tested, HEAD^ the auto merge commit of the PR before.

Not sure if this also runs on the PR CI that runs automatically for every PR, but here you need to write git log --author="bors <bors@rust-lang.org>" --format=%H | head -n1 instead of HEAD^ to get the last auto merge commit.

@kennytm
Copy link
Member Author

kennytm commented Dec 29, 2017 •
edited
Loading

Thanks @est31!

This won't run on PR CI since Travis conditional job doesn't support matching on PR title. I plan to do a bors-try for the actual verification anyway.

@kennytm kennytm changed the title [WIP] Requires tool to test-pass if PR description has “Update RLS/Rustfmt/Miri/…” [WIP] Requires tools to test-pass if the corresponding submodule is updated. Dec 29, 2017
@kennytm kennytm force-pushed the gate-tools-on-update branch 6 times, most recently from 126611e to f4d5743 Compare December 30, 2017 06:36
@kennytm
Copy link
Member Author

kennytm commented Dec 30, 2017

@bors try

First try. This try should fail because I've updated clippy and clippy is still test-fail because of compiletest_rs 0.3.3.

@bors
Copy link
Contributor

bors commented Dec 30, 2017

⌛ Trying commit f4d5743a3064f745947372dd8177af32c7bbf537 with merge 02df4ecf1be24d5ca3822de3c64adf3debc2d1fe...

@bors
Copy link
Contributor

bors commented Dec 30, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member Author

kennytm commented Dec 30, 2017 •
edited
Loading

Can't use git diff HEAD HEAD^ because the repository is shallow-cloned with depth=1. It seems git show produces the same output and does work when even shallow-cloned @est31.

Never mind, the depth can be configured to 2.

@bors try

@bors
Copy link
Contributor

bors commented Dec 30, 2017

⌛ Trying commit d52f00d2934ee8855953363408983dc2b52426db with merge da7ca6bb9d586cc946bb46a9bfd99426da36b957...

@bors
Copy link
Contributor

bors commented Dec 30, 2017

☀️ Test successful - status-travis
State: approved= try=True

@kennytm
Requires tools to test-pass if the corresponding submodule is updated.
8ed16fe 
If a PR intends to update a tool but its test has failed, abort the merge
regardless of current channel. This should help the tool maintainers if the
update turns out to be failing due to changes in latest master.
@bors
Copy link
Contributor

bors commented Dec 30, 2017

⌛ Trying commit 8db595a7add89c170d55040066b9de7cd0c7f764 with merge e4b725727229fc2729a6b8618cf8b3ce8c1b396e...

@bors
Copy link
Contributor

bors commented Dec 30, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member Author

kennytm commented Dec 30, 2017

@bors try

Second try. Now removed the clippy change, and the try should succeed.

@bors
Copy link
Contributor

bors commented Dec 30, 2017

⌛ Trying commit 05953b3 with merge 003b063...

bors added a commit that referenced this pull request Dec 30, 2017
@bors
Auto merge of #47063 - kennytm:gate-tools-on-update, r=<try>
003b063 
[WIP] Requires tools to test-pass if the corresponding submodule is updated.

Follow up of #46554. Breaking a tool would not stop bors from accepting a merge, but this also means when we intend to *fix* a tool but failed, bors will *still* accept the PR. This behavior is not helpful to the tool maintainers.

This PR attempts to fix this by rejecting the tool-update merge when a tool failed. It recognizes a PR as "tool-updating" when the corresponding submodule is changed, compared with the previous commit.

[WIP] - Final check to ensure the CI is interpreting the PR description correctly.
@bors
Copy link
Contributor

bors commented Dec 30, 2017

☀️ Test successful - status-travis
State: approved= try=True

@kennytm kennytm changed the title [WIP] Requires tools to test-pass if the corresponding submodule is updated. Requires tools to test-pass if the corresponding submodule is updated. Dec 30, 2017
@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 30, 2017
@kennytm
Copy link
Member Author

kennytm commented Dec 30, 2017

All experiments passed, ready for review now.

@alexcrichton
Copy link
Member

@bors: r+

Thanks so much for your help on this front @kennytm! This is a fantastic idea and great implementation.

@bors
Copy link
Contributor

bors commented Dec 30, 2017

📌 Commit 8ed16fe has been approved by alexcrichton

@kennytm kennytm 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 Dec 30, 2017
@bors
Copy link
Contributor

bors commented Dec 30, 2017

⌛ Testing commit 8ed16fe with merge 3f916bd...

bors added a commit that referenced this pull request Dec 30, 2017
@bors
Auto merge of #47063 - kennytm:gate-tools-on-update, r=alexcrichton
3f916bd 
Requires tools to test-pass if the corresponding submodule is updated.

Follow up of #46554. Breaking a tool would not stop bors from accepting a merge, but this also means when we intend to *fix* a tool but failed, bors will *still* accept the PR. This behavior is not helpful to the tool maintainers.

This PR attempts to fix this by rejecting the tool-update merge when a tool failed. It recognizes a PR as "tool-updating" when the corresponding submodule is changed, compared with the previous commit.
@bors
Copy link
Contributor

bors commented Dec 30, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 3f916bd to master...

@bors bors merged commit 8ed16fe into rust-lang:master Dec 30, 2017
@kennytm kennytm deleted the gate-tools-on-update branch December 31, 2017 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@est31 est31 est31 approved these changes

Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kennytm @rust-highfive @oli-obk @est31 @bors @alexcrichton