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

Revert "Auto merge of #57842 - gnzlbg:extract_libtest, r=gnzlbg" #59766

Merged
merged 1 commit into from
Apr 7, 2019

Conversation

xales
Copy link
Contributor

@xales xales commented Apr 7, 2019

This reverts commit 3eb4890, reversing
changes made to 7a4df3b.

This is, as best I can tell, a clean revert of #57842. It retains some interim changes, like moving black_box, and otherwise brings libtest back in as it was before removal.

…lbg"

This reverts commit 3eb4890, reversing
changes made to 7a4df3b.
@rust-highfive
Copy link
Collaborator

r? @bluss

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 7, 2019
@xales
Copy link
Contributor Author

xales commented Apr 7, 2019

r? @Manishearth

cc @gnzlbg

@rust-highfive rust-highfive assigned Manishearth and unassigned bluss Apr 7, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 7, 2019

This LGTM. The change being reverted had many unforeseen consequences (EDIT: sysroot crates linkage, feature gates, the test crate being super special, ...), and while there are some paths to fix those, it is unclear whether those paths are the right thing to do. After hacking on-and-off on it for a couple of weeks, this is something that we definitely want to get right, and we won't be able to do so with confidence before the next release, so reverting the change is the best we can do.

We probably want to try to do this again in the future, and come up with a solution that fixes everything that this one broke. But when doing so we should definitely involve the clippy maintainers in the process, so that we can make sure that landing this won't break clippy in an unrecoverable way like it happened here. And if it lands and clippy breaks, we should probably prioritize a revert right away to avoid clippy from being broken for weeks on nightly.

@Manishearth
Copy link
Member

@bors r+ p=10

high priority since we need to get this stuff through before the beta

@bors
Copy link
Contributor

bors commented Apr 7, 2019

📌 Commit 28ea249 has been approved by Manishearth

@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 Apr 7, 2019
@Manishearth
Copy link
Member

I'll file an issue for moving libtest out that has a plan for doing this.

@bors
Copy link
Contributor

bors commented Apr 7, 2019

⌛ Testing commit 28ea249 with merge 11510b2...

bors added a commit that referenced this pull request Apr 7, 2019
Revert "Auto merge of #57842 - gnzlbg:extract_libtest, r=gnzlbg"

This reverts commit 3eb4890, reversing
changes made to 7a4df3b.

This is, as best I can tell, a clean revert of #57842. It retains some interim changes, like moving `black_box`, and otherwise brings libtest back in as it was before removal.
@bors
Copy link
Contributor

bors commented Apr 7, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Manishearth
Pushing 11510b2 to master...

@RalfJung
Copy link
Member

RalfJung commented Apr 7, 2019

For the future, it would have been great to have this comment be part of the PR description so that it gets embedded into the git history.

@xales xales deleted the revertlibtest branch April 7, 2019 20:13
bors added a commit to rust-lang/rust-clippy that referenced this pull request Apr 7, 2019
Revert compiletest hacks, use latest compiletest

The libtest changes have been reverted, see rust-lang/rust#59766,  Manishearth/compiletest-rs#174
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.

7 participants