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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 24 additions & 22 deletions Library/Homebrew/brew.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@
Utils::Analytics.report_build_error(e)
e.dump(verbose: args&.verbose?)

if e.formula.head? || e.formula.deprecated? || e.formula.disabled?
if OS.unsupported_configuration?

Check warning on line 151 in Library/Homebrew/brew.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/brew.rb#L151 

Added line #L151 was not covered by tests
$stderr.puts "#{Tty.bold}Do not report this issue: you are running in an unsupported configuration.#{Tty.reset}"
elsif e.formula.head? || e.formula.deprecated? || e.formula.disabled?
reason = if e.formula.head?
"was built from an unstable upstream --HEAD"
elsif e.formula.deprecated?
Expand All @@ -166,35 +168,35 @@
Try to figure out the problem yourself and submit a fix as a pull request.
We will review it but may or may not accept it.
EOS

end

exit 1
rescue RuntimeError, SystemCallError => e
raise if e.message.empty?
rescue Exception => e # rubocop:disable Lint/RescueException
runtime_or_system_call_error = e.is_a?(RuntimeError) || e.is_a?(SystemCallError)
raise if runtime_or_system_call_error && e.message.empty?

onoe e
$stderr.puts Utils::Backtrace.clean(e) if args&.debug? || ARGV.include?("--debug")

exit 1
rescue MethodDeprecatedError => e
onoe e
if e.issues_url
$stderr.puts "If reporting this issue please do so at (not Homebrew/brew or Homebrew/homebrew-core):"
$stderr.puts " #{Formatter.url(e.issues_url)}"
method_deprecated_error = e.is_a?(MethodDeprecatedError)
runtime_or_system_call_or_method_deprecated_error = runtime_or_system_call_error || method_deprecated_error
if args&.debug? || ARGV.include?("--debug") || !runtime_or_system_call_or_method_deprecated_error
$stderr.puts Utils::Backtrace.clean(e)
end
$stderr.puts Utils::Backtrace.clean(e) if args&.debug? || ARGV.include?("--debug")
exit 1
rescue Exception => e # rubocop:disable Lint/RescueException
onoe e
if internal_cmd && !OS.unsupported_configuration?
if Homebrew::EnvConfig.no_auto_update?
$stderr.puts "#{Tty.bold}Do not report this issue until you've run `brew update` and tried again.#{Tty.reset}"
else
$stderr.puts "#{Tty.bold}Please report this issue:#{Tty.reset}"
$stderr.puts " #{Formatter.url(OS::ISSUES_URL)}"
end

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}"
MikeMcQuaid marked this conversation as resolved.
Show resolved Hide resolved
MikeMcQuaid marked this conversation as resolved.
Show resolved Hide resolved
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)}"

Check warning on line 194 in Library/Homebrew/brew.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/brew.rb#L194 

Added line #L194 was not covered by tests
elsif internal_cmd
$stderr.puts "#{Tty.bold}Please report this issue:#{Tty.reset}"
$stderr.puts " #{Formatter.url(OS::ISSUES_URL)}"

Check warning on line 197 in Library/Homebrew/brew.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/brew.rb#L197 

Added line #L197 was not covered by tests
Comment on lines +187 to +197
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

Comment on lines +195 to +197
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.

end
$stderr.puts Utils::Backtrace.clean(e)

exit 1
else
exit 1 if Homebrew.failed?
Expand Down
6 changes: 4 additions & 2 deletions Library/Homebrew/env_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,10 @@ module EnvConfig
},
HOMEBREW_NO_AUTO_UPDATE: {
description: "If set, do not automatically update before running some commands, e.g. " \
"`brew install`, `brew upgrade` and `brew tap`. Alternatively, " \
"run this less often by setting `HOMEBREW_AUTO_UPDATE_SECS` to a value higher than the default.",
"`brew install`, `brew upgrade` and `brew tap`. Preferably, " \
"run this less often by setting `HOMEBREW_AUTO_UPDATE_SECS` to a value higher than the " \
"default. Note that setting this and e.g. tapping new taps may result in a broken " \
"configuration. Please ensure you always run `brew update` before reporting any issues.",
boolean: true,
},
HOMEBREW_NO_BOOTSNAP: {
Expand Down
16 changes: 16 additions & 0 deletions Library/Homebrew/utils/backtrace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
.tap { |new_backtrace| print_backtrace_message if old_backtrace_length > new_backtrace.length }
end

sig { returns(String) }
def self.sorbet_runtime_path
@sorbet_runtime_path ||= "#{Gem.paths.home}/gems/sorbet-runtime"
end

sig { void }
def self.print_backtrace_message
return if @print_backtrace_message

Expand All @@ -32,5 +34,19 @@

@print_backtrace_message = true
end

sig { params(error: Exception).returns(T.nilable(String)) }
def self.tap_error_url(error)
backtrace = error.backtrace

Check warning on line 40 in Library/Homebrew/utils/backtrace.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/backtrace.rb#L40 

Added line #L40 was not covered by tests
return if backtrace.blank?

backtrace.each do |line|

Check warning on line 43 in Library/Homebrew/utils/backtrace.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/backtrace.rb#L43 

Added line #L43 was not covered by tests
if (tap = line.match(%r{/Library/Taps/([^/]+/[^/]+)/}))
return "https://github.com/#{tap[1]}/issues/new"
end
end

nil
end
end
end
7 changes: 5 additions & 2 deletions docs/Manpage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3740,8 +3740,11 @@ command execution e.g. `$(cat file)`.
`HOMEBREW_NO_AUTO_UPDATE`

: If set, do not automatically update before running some commands, e.g. `brew
install`, `brew upgrade` and `brew tap`. Alternatively, run this less often by
setting `HOMEBREW_AUTO_UPDATE_SECS` to a value higher than the default.
install`, `brew upgrade` and `brew tap`. Preferably, run this less often by
setting `HOMEBREW_AUTO_UPDATE_SECS` to a value higher than the default. Note
that setting this and e.g. tapping new taps may result in a broken
configuration. Please ensure you always run `brew update` before reporting any
issues.

`HOMEBREW_NO_BOOTSNAP`

Expand Down
4 changes: 2 additions & 2 deletions manpages/brew.1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
.\" generated by kramdown
.TH "BREW" "1" "March 2024" "Homebrew"
.TH "BREW" "1" "April 2024" "Homebrew"
.SH NAME
brew \- The Missing Package Manager for macOS (or Linux)
.SH "SYNOPSIS"
Expand Down Expand Up @@ -2445,7 +2445,7 @@ If set, do not send analytics\. Google Analytics were destroyed\. For more infor
.UE
.TP
\fBHOMEBREW_NO_AUTO_UPDATE\fP
If set, do not automatically update before running some commands, e\.g\. \fBbrew install\fP, \fBbrew upgrade\fP and \fBbrew tap\fP\&\. Alternatively, run this less often by setting \fBHOMEBREW_AUTO_UPDATE_SECS\fP to a value higher than the default\.
If set, do not automatically update before running some commands, e\.g\. \fBbrew install\fP, \fBbrew upgrade\fP and \fBbrew tap\fP\&\. Preferably, run this less often by setting \fBHOMEBREW_AUTO_UPDATE_SECS\fP to a value higher than the default\. Note that setting this and e\.g\. tapping new taps may result in a broken configuration\. Please ensure you always run \fBbrew update\fP before reporting any issues\.
.TP
\fBHOMEBREW_NO_BOOTSNAP\fP
If set, do not use Bootsnap to speed up repeated \fBbrew\fP calls\.
Expand Down