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

Stabilize -Zcompile-progress. #5995

Merged
merged 3 commits into from Sep 12, 2018

Conversation

Projects
None yet
4 participants
@kennytm
Copy link
Member

kennytm commented Sep 8, 2018

Closes #2536.

@dwijnand
Copy link
Member

dwijnand left a comment

LGTM

@kennytm kennytm force-pushed the kennytm:stabilize-compile-progress branch from a80b83a to b657f58 Sep 8, 2018

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Sep 9, 2018

The error on stable and beta is legit. The rustdoc --color flag is stabilized by rust-lang/rust#53003, merged on Aug 5th, meaning it will only be present on 1.30+. I'll fix the error for nightly.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 9, 2018

Oh jeez sorry we have way too many tests asserting exactly what command line is being run... In any case this looks great to me, thanks @kennytm!

@bors: r+ delegate+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 9, 2018

✌️ @kennytm can now approve this pull request

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 9, 2018

📌 Commit 595dbe6 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 9, 2018

⌛️ Testing commit 595dbe6 with merge c4d81d8...

bors added a commit that referenced this pull request Sep 9, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 9, 2018

💔 Test failed - status-travis

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 10, 2018

It looks like the tests are failing because rustdoc on stable/beta doesn't have the --color option stable and this is passing --color never. To be clear there's no deep-rooted need to get Cargo working across beta/stable/nightly, it's only been tested so far to ease development for Cargo folks. In that sense the thing we need to do here is figure out hopefully a non-invasive way of getting the tests to pass on stable/beta, which may involve disabling tons of tests on stable/beta.

An alternative strategy is to do feature detection like we do with rustc and learn if rustdoc supports --color, but I'd be wary of doing so because it can increase compile times by running more binaries.

@kennytm do you have a preferred way to solve this? Or maybe @dwijnand as the now-expert on our test suite, do you have thoughts on this problem?

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Sep 10, 2018

One option, in the interim--as we roll this change out of cargo, while the underlying compiler change is still rolling out to stable, is to use [..] more.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Sep 10, 2018

I plan to execute rustdoc --color never -V and check its response. It will error on the older versions.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 10, 2018

@kennytm hm so one problem with that though is that it can slow down Cargo's execution time. Even running rustdoc -V can take a few hundred milliseconds which can have a noticeable impact on incremental builds unfortunately :(

Are you thinking of just doing this in the test suite though? Or in Cargo itself?

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Sep 10, 2018

In cargo itself, call it once and then memorize the result (lazy_static it).

I think this is needed beyond the test suite, eg targeting toolchains installed via bisection which normally won't download the corresponding cargo.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 10, 2018

Ah that's a good point. We'd definitely cache globally once in a process but I'm also worried about caching across Cargo invocations (where lazy_static wouldn't help much). That being said though I'm remembering now that we actually cache rustc information on the filesystem in a JSON file, and we can probably do that for rustdoc as well (save this to JSON) which would alleviate any concern about rustdoc feature detection taking too long.

kennytm added some commits Sep 8, 2018

Do not add --color to rustdoc if it doesn't support it.
We detect this by executing `rustdoc --color never -V` and see if the
result is successful. To avoid repeatedly creating a new process, we
cache the result into `.rustc_info.json`.

@kennytm kennytm force-pushed the kennytm:stabilize-compile-progress branch from 1a911a9 to 9597bce Sep 12, 2018

@kennytm kennytm closed this Sep 12, 2018

@kennytm kennytm reopened this Sep 12, 2018

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Sep 12, 2018

Looks like this is green. Nice one!

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Sep 12, 2018

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 12, 2018

📌 Commit 9597bce has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 12, 2018

⌛️ Testing commit 9597bce with merge 21cddb7...

bors added a commit that referenced this pull request Sep 12, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 12, 2018

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

@bors bors merged commit 9597bce into rust-lang:master Sep 12, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@kennytm kennytm deleted the kennytm:stabilize-compile-progress branch Sep 12, 2018

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