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.

Sign up for GitHub

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

Jump to bottom

brew.rb: tell more people to not report issues. #17003

Merged
merged 1 commit into from
Apr 3, 2024
Merged

brew.rb: tell more people to not report issues. #17003

merged 1 commit into from
Apr 3, 2024

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Apr 2, 2024 •
edited
Loading

  • We should tell people to not report issues if they are running in an unsupported configuration.
  • We should tell people to run brew update before reporting issues if they have HOMEBREW_NO_AUTO_UPDATE set.
  • We should tell people to not report issues in more types of exceptions.
  • Warn people in HOMEBREW_NO_AUTO_UPDATE documentation.
  • DRY up brew.rb exception handling.

@MikeMcQuaid MikeMcQuaid requested a review from a team April 2, 2024 12:12
@MikeMcQuaid MikeMcQuaid mentioned this pull request Apr 2, 2024
Merged
colindean
colindean reviewed Apr 2, 2024
View reviewed changes
Copy link
Member

@colindean colindean left a comment

Choose a reason for hiding this comment

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

Adding

You have disabled automatic updates.

to the error message clarifies why the error occurred.

It would be kind to include how to reenable them, too, since that's the preferred path.

Library/Homebrew/brew.rb Outdated Show resolved Hide resolved
Library/Homebrew/brew.rb Outdated Show resolved Hide resolved
Library/Homebrew/brew.rb Outdated Show resolved Hide resolved
Library/Homebrew/brew.rb Show resolved Hide resolved
@MikeMcQuaid MikeMcQuaid requested a review from Bo98 April 2, 2024 15:36
@MikeMcQuaid
Copy link
Member Author

I've revamped this a bit to DRY up the repeated brew.rb logic.

Bo98
Bo98 approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Hard to tell from the diff but I think it's ok

EricFromCanada
EricFromCanada reviewed Apr 3, 2024
View reviewed changes
docs/Manpage.md Outdated Show resolved Hide resolved
colindean
colindean approved these changes Apr 3, 2024
View reviewed changes
Library/Homebrew/brew.rb Show resolved Hide resolved
@MikeMcQuaid @colindean
brew.rb: tell more people to not report issues.
21d99c5 
- We should tell people to not report issues if they are running in an
  unsupported configuration.
- We should tell people to run `brew update` before reporting issues if
  they have `HOMEBREW_NO_AUTO_UPDATE` set.
- We should tell people to not report issues in more types of
  exceptions.
- Warn people in `HOMEBREW_NO_AUTO_UPDATE` documentation.
- DRY up `brew.rb` exception handling.

Co-authored-by: Colin Dean <[email protected]>
@MikeMcQuaid
Copy link
Member Author

Thanks all!

carlocab
carlocab reviewed Apr 3, 2024
View reviewed changes
Library/Homebrew/brew.rb
Comment on lines +187 to +197
if OS.unsupported_configuration?
$stderr.puts "#{Tty.bold}Do not report this issue: you are running in an unsupported configuration.#{Tty.reset}"
elsif Homebrew::EnvConfig.no_auto_update?
$stderr.puts "#{Tty.bold}You have disabled automatic updates.#{Tty.reset}"
$stderr.puts "#{Tty.bold}Do not report this issue until you've run `brew update` and tried again.#{Tty.reset}"
elsif (issues_url = (method_deprecated_error && e.issues_url) || Utils::Backtrace.tap_error_url(e))
$stderr.puts "If reporting this issue please do so at (not Homebrew/brew or Homebrew/homebrew-core):"
$stderr.puts " #{Formatter.url(issues_url)}"
elsif internal_cmd
$stderr.puts "#{Tty.bold}Please report this issue:#{Tty.reset}"
$stderr.puts " #{Formatter.url(OS::ISSUES_URL)}"
Copy link
Member

Choose a reason for hiding this comment

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

This is probably a pre-existing issue, but: not sure these categories should be treated as mutually exclusive (which is kinda what happens as a consequence of the if-elsif-else structure).

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlocab which ones do you think should not be exclusive? The first two are broadly "don't report this", the latter two are "here's where to report it" (in different places).

I could see it being worth actually checking when the last brew update was done and, if e.g. today, then we don't print the auto-update message?

Copy link
Member

Choose a reason for hiding this comment

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

I could see it being worth actually checking when the last brew update was done and, if e.g. today, then we don't print the auto-update message?

Yea, I was thinking something along those lines. Because a user who still has HOMEBREW_NO_AUTO_UPDATE set but has run brew update in, say, the last 5 minutes would never see the last two messages even when they are relevant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened PR for this in #17018

@MikeMcQuaid MikeMcQuaid merged commit ce5887a into master Apr 3, 2024
26 checks passed
@MikeMcQuaid MikeMcQuaid deleted the more_do_not_report_issue branch April 3, 2024 07:26
cho-m
cho-m reviewed Apr 3, 2024
View reviewed changes
Library/Homebrew/brew.rb
Comment on lines +195 to +197
elsif internal_cmd
$stderr.puts "#{Tty.bold}Please report this issue:#{Tty.reset}"
$stderr.puts " #{Formatter.url(OS::ISSUES_URL)}"
Copy link
Member

@cho-m cho-m Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like refactor is causing this message to be output more often? Like on a non-existent formula, e.g.

❯ brew uninstall test
Error: No available formula with the name "test". Did you mean testssl?
Please report this issue:
  https://docs.brew.sh/Troubleshooting

❯ git checkout 4.2.16
Note: switching to '4.2.16'.
...

❯ brew uninstall test
Error: No available formula with the name "test". Did you mean testssl?

In my opinion, it seems a little odd to tell users to report this given I consider this a user issue (mistyping formula name or using wrong name).

Copy link
Member Author

Choose a reason for hiding this comment

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

@cho-m I agree this is a regression and we should not tell users to report this. However, it's late here and I cannot obviously figure out what specific change causes this. If you can: I'd love a PR or even just a comment ❤️

Copy link
Member

@cho-m cho-m Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Regression is probably from no longer just exiting on RuntimeError. We do classify a bunch of exceptions as RuntimeError (in this case, FormulaUnavailableError < FormulaOrCaskUnavailableError < RuntimeError).

If there are RuntimeErrors that should output message, then need to rescue or is_a? detect specific Errors to restore previous behavior and exit before reporting message.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cho-m Thanks, will take a look.

This was referenced Apr 3, 2024
Merged
Merged
@github-actions github-actions bot added the outdated PR was locked due to age label May 5, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@EricFromCanada EricFromCanada EricFromCanada left review comments

@cho-m cho-m cho-m left review comments

@carlocab carlocab carlocab left review comments

@colindean colindean colindean approved these changes

@Bo98 Bo98 Bo98 approved these changes

@ZhongRuoyu ZhongRuoyu ZhongRuoyu approved these changes

@p-linnane p-linnane p-linnane approved these changes

@SMillerDev SMillerDev Awaiting requested review from SMillerDev

@razvanazamfirei razvanazamfirei Awaiting requested review from razvanazamfirei

Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@MikeMcQuaid @colindean @Bo98 @SMillerDev @EricFromCanada @razvanazamfirei @cho-m @ZhongRuoyu @carlocab @p-linnane