Put back ui json check by GuillaumeGomez · Pull Request #48684 · rust-lang/rust

GuillaumeGomez · 2018-03-02T21:21:47Z

r? @petrochenkov

petrochenkov · 2018-03-03T00:47:00Z

-Zui-testing was lost during revert.

It doesn't work at all. Whatever I try to do, I always end with empty stderr so I'll keep my code for the moment.

Ah nevermind, I didn't update correctly...

petrochenkov · 2018-03-03T00:51:02Z

Could you split this into revert commits for 9b597a1 and 1dc2015 and commit with actual new implementation?

Also UI test outputs need to be regenerated.

zackmdavis · 2018-03-03T02:47:33Z

-Zui-testing was lost during revert. [...] split this into revert commits for 9b597a1 and 1dc2015

@GuillaumeGomez I think I got the restoration of -Zui-testing right in #48621?—in case you would rather cherry-pick those commits rather than re-authoring similar commits from scratch yourself. (git revert 9b597a1 and git revert 1dc2015 both result in conflicts that need to be fixed manually.)

zackmdavis · 2018-03-03T03:09:57Z

While this does successfully implement #48041, it seems rather ad hoc to make a show_explain method be part of the contract of the Emitter trait. (In contrast to how it makes sense for all Emitters to have an emit method, because that's what being an Emitter is all about.) Can we implement this in a more generalizable way?
Two potential alternatives that come to mind:
Make the Emitter trait have a destination method that exposes something we can write arbitrary lines to, and use it to write the --explain usage message in abort_if_errors.
JsonEmitter's dst field is of type Box<Write + Send>, and EmitterWriter's Destination implements Write and the field of all three of its variants implement Send (visible in the source in the case of BufferedTerminal and Raw, and the term docs say thatStderrTerminal is Send), so I think the types should check out.
Implement the --explain message as a Diagnostic, adding a plainer formatting mode to Diagnostic if necessary.

@GuillaumeGomez can you comment on this?

zackmdavis · 2018-03-03T03:21:18Z

As I mentioned in passing at the end of a previous comment, Handleralready knows what what codes we've emitted; JsonEmitter shouldn't have to know this.

Oh indeed. I'll remove this add.

zackmdavis · 2018-03-03T03:27:03Z

(If we end up needing to regenerate the UI test expectations anyway in the course of this PR, then we might as well also update the language (and use backticks) at the same time; but if not, then we can continue leave those tasks to #48559.)

bors · 2018-03-03T04:56:36Z

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

GuillaumeGomez · 2018-03-03T12:32:43Z

@petrochenkov: Reverting seems like way too much troubles...

GuillaumeGomez · 2018-03-03T16:19:13Z

This new way of handling things will certainly please you more. :)

bors · 2018-03-04T07:11:25Z

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

bors · 2018-03-04T15:17:45Z

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

GuillaumeGomez · 2018-03-13T08:50:36Z

@petrochenkov: Tests (finally) passed.

petrochenkov · 2018-03-13T10:11:31Z

Last thing: removing this trailing empty line and the empty line after "Some errors occurred...", then we'll land this with higher priority.

GuillaumeGomez · 2018-03-13T16:54:00Z

@petrochenkov: Done as well, but let's wait for CI confirmation first.

GuillaumeGomez · 2018-03-13T23:53:57Z

@petrochenkov: New tests are being added all the time. T_T

petrochenkov · 2018-03-14T00:02:37Z

@bors r+ p=1

bors · 2018-03-14T00:02:40Z

📌 Commit 2e104a7 has been approved by petrochenkov

bors · 2018-03-14T01:40:48Z

⌛ Testing commit 2e104a7 with merge fab632f...

@petrochenkov

…chenkov Put back ui json check r? @petrochenkov

bors · 2018-03-14T04:20:25Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing fab632f to master...

rust-highfive assigned petrochenkov Mar 2, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2018

petrochenkov reviewed Mar 3, 2018
View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2018

zackmdavis mentioned this pull request Mar 3, 2018
revert #48337 (--explain usage note) #48621
Closed

zackmdavis reviewed Mar 3, 2018
View reviewed changes

petrochenkov 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 Mar 3, 2018

GuillaumeGomez force-pushed the put-back-ui-json-check branch 5 times, most recently from d61877f to 1975eb4Compare March 3, 2018 15:58

GuillaumeGomez force-pushed the put-back-ui-json-check branch from 1975eb4 to 4c5adcfCompare March 3, 2018 16:30

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2018

GuillaumeGomez force-pushed the put-back-ui-json-check branch 2 times, most recently from 7c371ad to 8a6f0a2Compare March 4, 2018 14:11

GuillaumeGomez force-pushed the put-back-ui-json-check branch 2 times, most recently from bcf99f3 to ef6dbb1Compare March 4, 2018 21:02

GuillaumeGomez force-pushed the put-back-ui-json-check branch 2 times, most recently from 46dc072 to 5a48d31Compare March 12, 2018 23:33

petrochenkov mentioned this pull request Mar 13, 2018
Update rustc --explain sentence #48559
Closed

petrochenkov reviewed Mar 13, 2018
View reviewed changes

GuillaumeGomez force-pushed the put-back-ui-json-check branch from 5a48d31 to c203cbbCompare March 13, 2018 16:52

GuillaumeGomez force-pushed the put-back-ui-json-check branch 2 times, most recently from 43bda9a to 6c673efCompare March 13, 2018 20:27

GuillaumeGomez added 2 commits March 14, 2018 00:52

test for putting back check on json
8c75e18

update tests
2e104a7

GuillaumeGomez force-pushed the put-back-ui-json-check branch from 6c673ef to 2e104a7Compare March 13, 2018 23:53

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2018

bors added a commit that referenced this pull request Mar 14, 2018
Auto merge of#48684- GuillaumeGomez:put-back-ui-json-check, r=petro…
fab632f
…chenkov Put back ui json check r? @petrochenkov

bors merged commit 2e104a7 into rust-lang:masterMar 14, 2018

This was referenced Mar 14, 2018
Better document the implementors of Clone and Copy #48171
Merged
Add crate name to "main function not found" error message. #48706
Merged
Stabilize inclusive range (..=) #47813
Merged
Implement RFC 2011 (nicer assert messages) #48973
Closed

This was referenced Mar 14, 2018
resolve '_ in dyn Trait just like ordinary elision #48960
Merged
Implement import renaming with _ (RFC 2166) #48922
Merged

GuillaumeGomez deleted the put-back-ui-json-check branch March 14, 2018 08:54

petrochenkov mentioned this pull request Mar 16, 2018
Most of UI tests are now compiled twice, once with --error-format json and once with --error-format human#48550
Closed

ehuss mentioned this pull request Apr 30, 2019
FailureNote JSON value is not useful #60425
Closed

Uh oh!

Uh oh!

Conversation

GuillaumeGomez commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Mar 3, 2018

Uh oh!

zackmdavis commented Mar 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Mar 3, 2018

Uh oh!

GuillaumeGomez commented Mar 3, 2018

Uh oh!

GuillaumeGomez commented Mar 3, 2018

Uh oh!

bors commented Mar 4, 2018

Uh oh!

bors commented Mar 4, 2018

Uh oh!

GuillaumeGomez commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Mar 13, 2018

Uh oh!

GuillaumeGomez commented Mar 13, 2018

Uh oh!

petrochenkov commented Mar 14, 2018

Uh oh!

bors commented Mar 14, 2018

Uh oh!

bors commented Mar 14, 2018

Uh oh!

bors commented Mar 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants