Skip to content

emit error when doc generation fails#55933

Merged
bors merged 1 commit into
rust-lang:masterfrom
euclio:doc-panic
Dec 5, 2018
Merged

emit error when doc generation fails#55933
bors merged 1 commit into
rust-lang:masterfrom
euclio:doc-panic

Conversation

@euclio

@euclio euclio commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

Fixes #41813.

The diagnostic looks something like this:

error: couldn't generate documentation: No space left on device (os error 28)
  |
  = note: failed to create or modify "/path/to/crate/target/doc/src/lazycell"

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @steveklabnik

(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 Nov 13, 2018
@TimNN

TimNN commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

Ping form triage @steveklabnik / @rust-lang/rustdoc: This PR requires your review.

@steveklabnik

Copy link
Copy Markdown
Contributor

@QuietMisdreavus or @GuillaumeGomez should be r+'ing this, not me

@GuillaumeGomez

Copy link
Copy Markdown
Member

Looks nice! Please add a rustdoc-ui test and then I r+.

@euclio

euclio commented Nov 21, 2018

Copy link
Copy Markdown
Contributor Author

I'd like to, but I'm not sure how to simulate filesystem errors in a UI test.

@GuillaumeGomez

Copy link
Copy Markdown
Member

Ah that's a good point...

@rust-lang/infra Does anyone knows how to simulate a full disk?

@kennytm

kennytm commented Nov 22, 2018

Copy link
Copy Markdown
Member

what about write to a location which you have no permission? this will need to be changed to a run-make test though.

@GuillaumeGomez

Copy link
Copy Markdown
Member

Hard to write an all-OSes test...

@QuietMisdreavus

Copy link
Copy Markdown
Contributor

It may be enough to make it run on just Unix, though. The trick is just to make sure it outputs the right kind of error message when it encounters a filesystem error, even if the nature of that error doesn't matter much. Plus, plenty of other run-make-fulldeps tests skip running on Windows, so at least there's precedent there. At its core, we basically need to make sure that it doesn't say "internal compiler error" (or whatever the right capitalization should be) when it hits this error, right? If you want, i could try to sketch it out.

Otherwise, i quite like this PR. (I'm also not opposed to landing it as-is, since it's effectively just changing the way rustdoc prints certain kinds of errors.)

@euclio

euclio commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

@QuietMisdreavus pushed a run-make test that checks that it doesn't ICE.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this save the exit status into $(.SHELLSTATUS) for later? I'm not familiar with this syntax.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool, thanks for the link!

Comment thread src/librustdoc/lib.rs Outdated
@euclio euclio force-pushed the doc-panic branch 2 times, most recently from d4325f0 to e2fa3c1 Compare December 4, 2018 22:28

@QuietMisdreavus QuietMisdreavus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Excellent, thanks so much! r=me when travis is green.

@GuillaumeGomez

Copy link
Copy Markdown
Member

@bors: r=QuietMisdreavus

@bors

bors commented Dec 5, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit e2fa3c17ebf51aac41801a040ea0e7857e9da63f has been approved by QuietMisdreavus

@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 Dec 5, 2018
@kennytm

kennytm commented Dec 5, 2018

Copy link
Copy Markdown
Member

@bors r-

The test did not pass on Windows. See #56531 (comment)

---- [run-make] run-make-fulldeps\rustdoc-io-error stdout ----
error: make failed
status: exit code: 2
command: "make"
stdout:
------------------------------------------
make[1]: Entering directory '/c/projects/rust/src/test/run-make-fulldeps/rustdoc-io-error'
make[1]: Leaving directory '/c/projects/rust/src/test/run-make-fulldeps/rustdoc-io-error'
------------------------------------------
stderr:
------------------------------------------
make[1]: *** No targets.  Stop.
------------------------------------------
thread '[run-make] run-make-fulldeps\rustdoc-io-error' panicked at 'explicit panic', src\tools\compiletest\src\runtest.rs:3284:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
    [run-make] run-make-fulldeps\rustdoc-io-error
test result: FAILED. 192 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
thread 'main' panicked at 'Some tests failed', src\tools\compiletest\src\main.rs:503:22

@bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 5, 2018
@euclio

euclio commented Dec 5, 2018

Copy link
Copy Markdown
Contributor Author

The test was missing an else branch that runs an empty target. Fixed.

@QuietMisdreavus

Copy link
Copy Markdown
Contributor

r=me pending travis

@QuietMisdreavus

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Dec 5, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit c359f98 has been approved by QuietMisdreavus

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

bors commented Dec 5, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit c359f98 with merge 14997d5...

bors added a commit that referenced this pull request Dec 5, 2018
emit error when doc generation fails

Fixes #41813.

The diagnostic looks something like this:

```
error: couldn't generate documentation: No space left on device (os error 28)
  |
  = note: failed to create or modify "/path/to/crate/target/doc/src/lazycell"
```
@bors

bors commented Dec 5, 2018

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 14997d5 to master...

@bors bors merged commit c359f98 into rust-lang:master Dec 5, 2018
@michaelwoerister

Copy link
Copy Markdown
Member

I'm wondering why this caused a visible performance regression:
https://perf.rust-lang.org/compare.html?start=b866f7d258a2428e004039eb0f3fabd73cc9d6ae&end=14997d56a550f4aa99fe737593cd2758227afc56&stat=instructions:u

cc @rust-lang/wg-compiler-performance

@eddyb

eddyb commented Dec 7, 2018

Copy link
Copy Markdown
Contributor

@michaelwoerister That makes no sense, I'd suspect an issue with the perf setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

10 participants