Emit note when --future-incompat-report had nothing to report by Aaron1011 · Pull Request #9263 · rust-lang/cargo (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Aaron1011

@rust-highfive

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@Aaron1011

@alexcrichton

I would personally think that this wasn't necessary (e.g. rustc doesn't print anything saying "there were 0 warnings"). Could you elaborate a bit on why this should be added? (having a blank PR description and a commit message which is simply the PR title isn't too helpful...)

@Aaron1011

@alexcrichton

Ok thanks for the link, if it's ok with y'all I figure we can discuss it here rather than continuing on the issue over there?

In my opinion there should be no need to print something out like this (e.g. rustc doesn't print out "0 warnings were found"). Could y'all elaborate, though, as to why you feel like a message is necessary? I could see that while testing you might expect it to have a bug and not work, but for a final form of the feature which is expected to work I would think that no output is translateable to "nothing that I need to do"

@jyn514

I could see that while testing you might expect it to have a bug and not work, but for a final form of the feature which is expected to work I would think that no output is translateable to "nothing that I need to do"

My understanding is that the final product won't have this option at all, and will print out future-incompat warnings unconditionally. I agree at that point printing "0 errors" isn't helpful. But I think explicitly opting into the warnings should confirm that yes it works.

@ehuss

I believe the (stable) experience is intended to be:

  1. Use runs cargo build. This prints a notice that there are future-incompat warnings.
  2. User has two options here:
    2a. Run cargo describe-future-incompatibilities --id=xyz
    2b. Run cargo build --future-incompat-report

The concern is someone runs cargo build --future-incompat-report, and it doesn't display anything, that could be confusing.

Personally, I still think the --future-incompat-report flag doesn't seem like the right UX. I'm not entirely clear what the use case is for that where the report command doesn't suffice.

If the intent is that people always pass --future-incompat-report, then yea, printing 0 doesn't make sense. But I don't know why someone would always pass that flag.

@Aaron1011

I'm not entirely clear what the use case is for that where the report command doesn't suffice.

The use case is running this on CI, which may build dependencies that a user machine typically does not (e.g. platform-specific dependencies like winapi). Instead of making scripts try to extract the report id from a human-readable message, we provide a flag that can be added to every command invocation.

@alexcrichton

I was not under the impression that Cargo would print "you have future incompat warnings" by default. I thought that users would have to opt-in to the notifications. Seems like it would be best to clarify that point about the expected usage before continuing this discussion since that likely heavily influences this!

@ehuss

Sorry about the confusion. RFC 2834 describes the behavior where Cargo will always check for future-incompat warnings and print a small notice informing the user. The user can then run the describe command to get a full report. The intent is to warn the developer that one of their dependencies may stop compiling soon, and they need to do something about that. If it isn't enabled all the time, it likely won't have the intended impact of getting projects updated.

A concern is the inability to silence the warning, since it may take a while to resolve it. The RFC included an "Annoyance Modulation", but I'm not sure if that specifically is how it should work.

@alexcrichton

Ok thanks for the link, I had forgotten this part of the original RFC. If that's the case then this seems fine since it'll likely be rarely run anyway (seems like the majority of what happens is you run cargo build and get a short message to run cargo describe-future-incompat at the end).

@ehuss

I'm going to go ahead and merge this. I'm also uncertain about the overall UX here, but I think this will be less confusing.

@bors r+

@bors

📌 Commit f9512ff has been approved by ehuss

@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

Mar 22, 2021

@ehuss ehuss mentioned this pull request

Mar 22, 2021

14 tasks

@bors

@bors

@ehuss ehuss mentioned this pull request

Mar 23, 2021

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request

Mar 23, 2021

@Dylan-DPC

Update cargo

8 commits in 90691f2bfe9a50291a98983b1ed2feab51d5ca55..58a961314437258065e23cb6316dfc121d96fb71 2021-03-16 21:36:55 +0000 to 2021-03-22 22:59:56 +0000

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request

Mar 23, 2021

@Dylan-DPC

Update cargo

8 commits in 90691f2bfe9a50291a98983b1ed2feab51d5ca55..58a961314437258065e23cb6316dfc121d96fb71 2021-03-16 21:36:55 +0000 to 2021-03-22 22:59:56 +0000

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 26, 2021

@bors

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.