Don't tap user-untapped official taps #10366

Rylan12 · 2021-01-19T23:32:51Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?
Have you successfully run brew man locally and committed any changes?

This PR modifies the logic for when to automatically tap official taps.

As a refresher, there are two times where an official tap would be automatically tapped:

When running the bundle, test-bot, or services command and the respective repo isn't tapped
When args.named.to_casks (or other similar methods that convert named args to casks) is unable to match a name to a formula or cask. This is seen mostly when running brew install <invalid_formula_or_cask> or brew uninstall <invalid_formula_or_cask>.

Now, if a user has untapped an official tap, we will remember this in the Homebrew settings. If a user does one of the above actions that would normally tap the repo, we will first check to see whether they have already untapped the repo. If so, we assume that they genuinely do not want that repo, so we don't re-tap.

A few comments:

There is one more spot where Tap.install_default_cask_tap_if_necessary is used in Library/Homebrew/cask/cmd.rb. Should this be modified to always tap the cask repo, even if the user has explicitly untapped it? I'm not sure if this file will even have a use after Homebrew 2.8.0 (where brew cask commands are removed).
If there are other places where a similar thing is done that should be modified, let me know.
I will probably refactor some of this logic out to an e.g. Tap.untapped_official_taps to avoid repeating the untapped = Settings.read(:untapped) || [] line in many places. I didn't for now simply because I wanted to get this PR up for feedback.

CC: @carlocab, @fxcoudert, and @jonchang

BrewTestBot · 2021-01-19T23:33:06Z

Review period will end on 2021-01-20 at 23:32:51 UTC.

MikeMcQuaid

Nice idea here! I think it might be nicer to reduce the situations in which install_default_cask_tap_if_necessary is run to only when a e.g. --cask option or similar is specifically run. Otherwise, if we're looking up "formula or a cask" and it's not tapped: we should simply skip all cask logic. Thoughts?

Rylan12 · 2021-01-20T15:10:34Z

Nice idea here! I think it might be nicer to reduce the situations in which install_default_cask_tap_if_necessary is run to only when a e.g. --cask option or similar is specifically run. Otherwise, if we're looking up "formula or a cask" and it's not tapped: we should simply skip all cask logic. Thoughts?

I think it depends on whether we can assume that the only reason the cask tap wouldn't be installed is that a user actively uninstalled it (if that makes sense).

Does the Homebrew installer always tap the cask tap (on macOS)? If so, for how long has it done this?

If the answer is no, I don't think we should do that. We want brew install <cask> to work out of the box, so keeping the auto-tap feature is important.

If the answer is yes, I'd be more on-board. I think that means that we can assume that the cask tap being uninstalled indicates an active user choice to untap. We may still want to show a message reminding users (who don't have homebrew/cask in the homebrew.untapped setting) to brew tap homebrew/cask if they want casks.

MikeMcQuaid · 2021-01-20T17:06:34Z

Does the Homebrew installer always tap the cask tap (on macOS)?

It does not.

We want brew install <cask> to work out of the box, so keeping the auto-tap feature is important.

Agreed but I don't think it is in any case other than brew install.

Rylan12 · 2021-01-20T17:08:25Z

Agreed but I don't think it is in any case other than brew install.

Ah, I see. Yeah, we can move the retry if Tap. install_default_cask_tap_if_necessary line to the actual install command itself.

Rylan12 · 2021-01-20T17:23:26Z

Okay, I updated so it will only attempt to tap on brew install. I also decided to override the homebrew.untapped setting if --cask is passed, assuming that if a user passes --cask they are okay with homebrew/cask being tapped.

Rylan12 · 2021-01-20T18:41:32Z

I've seen this failure many times recently. Not sure what's going on. It seems that simply rerunning the tests fixes it.

  1) Homebrew::Cleanup::cleanup_logs cleans up logs if older than 30 days
     Failure/Error: expect(path).not_to exist
       expected #<Pathname:/tmp/homebrew-tests-20210120-8897-1khwbac/logs/delete_me> not to exist
     # ./test/cleanup_spec.rb:215:in `block (3 levels) in <top (required)>'
     # ./test/cleanup_spec.rb:43:in `block (2 levels) in <top (required)>'
     # ./test/spec_helper.rb:207:in `block (3 levels) in <top (required)>'
     # ./test/spec_helper.rb:206:in `block (2 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.6.0/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:124:in `block in run'
     # ./vendor/bundle/ruby/2.6.0/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:110:in `loop'
     # ./vendor/bundle/ruby/2.6.0/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:110:in `run'
     # ./vendor/bundle/ruby/2.6.0/gems/rspec-retry-0.6.2/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./vendor/bundle/ruby/2.6.0/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:37:in `block (2 levels) in setup'
     # ./vendor/bundle/ruby/2.6.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'

BrewTestBot · 2021-01-21T00:47:47Z

Review period ended.

MikeMcQuaid · 2021-01-21T13:30:31Z

I've seen this failure many times recently. Not sure what's going on. It seems that simply rerunning the tests fixes it.

Yeh, it's either order-dependent or failing based on another test that's running. Could ideally try to fix that or bump the retries?

MikeMcQuaid · 2021-01-21T13:31:08Z

Library/Homebrew/cmd/install.rb

+      formulae, casks = args.named.to_formulae_and_casks
+                            .partition { |formula_or_cask| formula_or_cask.is_a?(Formula) }
+    rescue FormulaOrCaskUnavailableError, Cask::CaskUnavailableError => e
+      retry if Tap.install_default_cask_tap_if_necessary(force: args.cask?)


Perfect 🎉

MikeMcQuaid · 2021-01-21T13:32:22Z

Library/Homebrew/brew.rb

    possible_tap = Tap.fetch(possible_tap.first) if possible_tap

-    odie "Unknown command: #{cmd}" if !possible_tap || possible_tap.installed?
+    if !possible_tap || possible_tap.installed? || Tap.untapped_official_taps.include?(possible_tap.name)


So this logic (and everything related) means that if you untap it: it'll never be automatically retapped?

Yes, that's the idea. Although, if you retap using brew tap homebrew/cask, it removes homebrew/cask from the homebrew.untapped setting meaning that it would be retapped automatically in the future if needed (assuming the reason it became untapped was not done using tap.uninstall. tbh I'm not sure how that would happen).

Actually, now that I'm thinking about it, are there any instances where a tap could be uninstalled and we wouldn't want to write to the setting? Maybe we should only write if a user untaps via the brew untap command...

Yes, that's the idea. Although, if you retap using brew tap homebrew/cask, it removes homebrew/cask from the homebrew.untapped setting meaning that it would be retapped automatically in the future if needed (assuming the reason it became untapped was not done using tap.uninstall. tbh I'm not sure how that would happen).

Seems good!

Maybe we should only write if a user untaps via the brew untap command...

Yeh, maybe?

This line, in particular, is saying "raise an error saying unknown command if there is no tap found with the given command, the tap with the given command is already installed, or the tap with the given command has been manually uninstalled by the user. Otherwise, continue and autotap"

Don't tap user-untapped official taps

3ca68c0

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 19, 2021

Rylan12 mentioned this pull request Jan 19, 2021

brew taps homebrew/cask on user typos #10303

Closed

MikeMcQuaid approved these changes Jan 20, 2021

View reviewed changes

Tap: add untapped_official_taps method

ebba369

Only tap homebrew/cask on CaskUnavailableError in brew install

6481665

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 21, 2021

BrewTestBot approved these changes Jan 21, 2021

View reviewed changes

MikeMcQuaid approved these changes Jan 21, 2021

View reviewed changes

tap: write untapped setting only on manual untap

ecfad29

Rylan12 merged commit acede73 into Homebrew:master Jan 21, 2021

Rylan12 deleted the auto-tap-fix branch January 21, 2021 20:47

scpeters mentioned this pull request Jan 27, 2021

Explicitly tap homebrew-test-bot gazebo-tooling/release-tools#383

Merged

BrewTestBot added the outdated PR was locked due to age label Feb 21, 2021

Homebrew locked as resolved and limited conversation to collaborators Feb 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Don't tap user-untapped official taps #10366

Don't tap user-untapped official taps #10366

Uh oh!

Rylan12 commented Jan 19, 2021

BrewTestBot commented Jan 19, 2021

MikeMcQuaid left a comment

Rylan12 commented Jan 20, 2021

MikeMcQuaid commented Jan 20, 2021

Rylan12 commented Jan 20, 2021 •

edited

Loading

Rylan12 commented Jan 20, 2021

Rylan12 commented Jan 20, 2021

BrewTestBot commented Jan 21, 2021

MikeMcQuaid commented Jan 21, 2021

MikeMcQuaid Jan 21, 2021

MikeMcQuaid Jan 21, 2021

Rylan12 Jan 21, 2021

MikeMcQuaid Jan 21, 2021

Rylan12 Jan 21, 2021

Uh oh!

Don't tap user-untapped official taps #10366

Don't tap user-untapped official taps #10366

Uh oh!

Conversation

Rylan12 commented Jan 19, 2021

BrewTestBot commented Jan 19, 2021

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Rylan12 commented Jan 20, 2021

MikeMcQuaid commented Jan 20, 2021

Rylan12 commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rylan12 commented Jan 20, 2021

Rylan12 commented Jan 20, 2021

BrewTestBot commented Jan 21, 2021

MikeMcQuaid commented Jan 21, 2021

MikeMcQuaid Jan 21, 2021

Choose a reason for hiding this comment

MikeMcQuaid Jan 21, 2021

Choose a reason for hiding this comment

Rylan12 Jan 21, 2021

Choose a reason for hiding this comment

MikeMcQuaid Jan 21, 2021

Choose a reason for hiding this comment

Rylan12 Jan 21, 2021

Choose a reason for hiding this comment

Rylan12 commented Jan 20, 2021 •

edited

Loading