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.

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

Fix untap cmd bugs #16875

Merged
merged 7 commits into from Mar 14, 2024
Merged

Fix untap cmd bugs #16875

merged 7 commits into from Mar 14, 2024

Conversation

apainintheneck
Copy link
Contributor

@apainintheneck apainintheneck commented Mar 11, 2024

  • 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?

This bug was found when reviewing #16867.

The warning for installed formulae or casks in brew untap wasn't working before for a few reasons.

  1. It never got past the installed name check because the
    installed name sets had short names and the tap names were
    long names including the tap namespace too. Now we just trim the
    long name before comparing it to the installed name set.

Before:

["name"].include?("tap/full/name") # always false

After:

["name"].include("tap/full/name".split("/").last) # sometimes true
  1. The names we were trying to load formulae and casks with
    were incorrect.

Before:

tap = Tap.fetch("homebrew/cask-versions")
token = "homebrew/cask-versions/token"

cask = Cask::CaskLoader.load("#{tap}/#{token}")

After:

token = "homebrew/cask-versions/token"
cask = CaskCaskLoader.load(token)

I tried to keep the commits organized so they're probably best to review in order. The change itself is pretty simple (see the first commit) but adding tests is quite a feat. Let me know if there's some way to simplify the tests without turning them into a bunch of integration tests.

It is not that tough to test that things are working correctly now.

If you have any casks installed, the following line should fail for you.

$ HOMEBREW_NO_INSTALL_FROM_API=1 HOMEBREW_DEVELOPER= brew untap homebrew/cask
Error: Refusing to untap homebrew/cask because it contains the following installed formulae or casks:
discord
iterm2
visual-studio-code

Beyond that I installed the candy formula from a third-party tap and made sure that it gave me the appropriate warning when trying to untap the tap with that formula.

$ HOMEBREW_DEVELOPER= brew untap owenthereal/candy
Error: Refusing to untap owenthereal/candy because it contains the following installed formulae or casks:
candy
$ brew uninstall candy
...
$ HOMEBREW_DEVELOPER= brew untap owenthereal/candy
Untapping owenthereal/candy...
Untapped 1 formula (107 files, 60.9KB).

This wasn't working before for a few reasons.

1. It never got past the installed name check because the
installed name sets had short names and the tap names were
long names including the tap namespace too. Now we just trim the
long name before comparing it to the installed name set.

Before:

```
["name"].include?("tap/full/name") # always false
```

After:

```
["name"].include("tap/full/name".split("/").last) # sometimes true
```

2. The names we were trying to load formulae and casks with
were incorrect.

Before:

```
tap = Tap.fetch("homebrew/cask-versions")
token = "homebrew/cask-versions/token"

cask = Cask::CaskLoader.load("#{tap}/#{token}")
```

After:

```
token = "homebrew/cask-versions/token"
cask = CaskCaskLoader.load(token)
```
These are regression tests to make sure that this logic is reproducible.
If this logic is not working, it might mean that someone removes a tap
accidentally that still includes a formula or cask that they currently
have installed.

The tests are extravagant and over-engineered but I'm not sure that
there's an easier way to do this without massive integration tests.
@apainintheneck apainintheneck added the bug Reproducible Homebrew/brew bug label Mar 11, 2024
Copy link
Member

@ZhongRuoyu ZhongRuoyu left a comment

Choose a reason for hiding this comment

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

Makes sense to me, thanks @apainintheneck!

Library/Homebrew/untap.rb Outdated Show resolved Hide resolved
Library/Homebrew/untap.rb Outdated Show resolved Hide resolved
Library/Homebrew/untap.rb Show resolved Hide resolved
Library/Homebrew/untap.rb Outdated Show resolved Hide resolved
Library/Homebrew/untap.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/untap_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/untap_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/untap_spec.rb Show resolved Hide resolved
Library/Homebrew/test/untap_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good to me, please feel free to self-merge when you consider this ready!

@apainintheneck
Copy link
Contributor Author

Install_test_formula is quite slow though. It increases the total test runtime locally around 15 seconds.

Top 4 slowest examples (15.33 seconds, 100.0% of total time):
  Homebrew::Untap.installed_formulae_for with core tap returns the expected formulae
    8.06 seconds ./test/untap_spec.rb:55
  Homebrew::Untap.installed_formulae_for with non-core tap returns the expected formulae
    6.93 seconds ./test/untap_spec.rb:55
  Homebrew::Untap.installed_casks_for with core cask tap returns the expected casks
    0.19983 seconds ./test/untap_spec.rb:115
  Homebrew::Untap.installed_casks_for with non-core cask tap returns the expected casks
    0.13921 seconds ./test/untap_spec.rb:115

I'm kind of surprised it's really that slow. setup_test_formula and install_with_caskfile work well enough and don't really change the total test time too much so those changes are worth keeping.

Before:

Top 4 slowest examples (0.14913 seconds, 97.7% of total time):
  Homebrew::Untap.installed_casks_for with non-core cask tap returns the expected casks
    0.05421 seconds ./test/untap_spec.rb:113
  Homebrew::Untap.installed_formulae_for with core tap returns the expected formulae
    0.03849 seconds ./test/untap_spec.rb:54
  Homebrew::Untap.installed_casks_for with core cask tap returns the expected casks
    0.02884 seconds ./test/untap_spec.rb:113
  Homebrew::Untap.installed_formulae_for with non-core tap returns the expected formulae
    0.02759 seconds ./test/untap_spec.rb:54

After:

Top 4 slowest examples (0.40141 seconds, 99.4% of total time):
  Homebrew::Untap.installed_casks_for with non-core cask tap returns the expected casks
    0.1855 seconds ./test/untap_spec.rb:109
  Homebrew::Untap.installed_casks_for with core cask tap returns the expected casks
    0.14829 seconds ./test/untap_spec.rb:109
  Homebrew::Untap.installed_formulae_for with core tap returns the expected formulae
    0.03499 seconds ./test/untap_spec.rb:49
  Homebrew::Untap.installed_formulae_for with non-core tap returns the expected formulae
    0.03264 seconds ./test/untap_spec.rb:49

FYI, I'm using the following command to profile the tests.

$ brew tests --only=untap --profile=10

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks @apainintheneck. Fine to ship as-is for fixing user bugs but feels like this makes sense to live in another file 🔜 that I'd love in a follow-up PR.

require "extend/cachable"

module Homebrew
# Helpers for the `brew untap` command.
Copy link
Member

Choose a reason for hiding this comment

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

If these are only used by the untap command: can they live in cmd/untap instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that its not the best location but it does make the logic and testing simpler. It'll be easy to move back into the command once we're using the new command interface described in #16815.

@apainintheneck apainintheneck merged commit 7473e63 into master Mar 14, 2024
26 checks passed
@apainintheneck apainintheneck deleted the fix-untap-cmd-bugs branch March 14, 2024 03:17
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 14, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Reproducible Homebrew/brew bug outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants