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 Cask::Cask.all bug #16384

Merged
merged 2 commits into from Dec 22, 2023
Merged

Conversation

apainintheneck
Copy link
Contributor

@apainintheneck apainintheneck commented Dec 22, 2023

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

Fixes #16378

  1. 5a674c2

Fix bug in Cask::Cask.all where we'd only read cask files and not attempt to load casks from the API JSON.

I ran a quick benchmark to make sure this isn't going to slow things down more than usual and it seems in the neighborhood so nothing to worry about from a performance perspective.

$ hyperfine \
  --parameter-list branch master,fix-cask-all \
  --warmup 2 \
  --setup 'git switch {branch}' \
  'HOMEBREW_EVAL_ALL=1 brew ruby -rformula -e \'Cask::Cask.all\''
Benchmark 1: HOMEBREW_EVAL_ALL=1 brew ruby -rformula -e 'Cask::Cask.all' (branch = master)
  Time (mean ± σ):     10.360 s ±  0.039 s    [User: 7.026 s, System: 2.639 s]
  Range (min … max):   10.291 s … 10.435 s    10 runs
 
Benchmark 2: HOMEBREW_EVAL_ALL=1 brew ruby -rformula -e 'Cask::Cask.all' (branch = fix-cask-all)
  Time (mean ± σ):      9.124 s ±  0.120 s    [User: 6.689 s, System: 2.043 s]
  Range (min … max):    9.015 s …  9.346 s    10 runs
 
Summary
  HOMEBREW_EVAL_ALL=1 brew ruby -rformula -e 'Cask::Cask.all' (branch = fix-cask-all) ran
    1.14 ± 0.02 times faster than HOMEBREW_EVAL_ALL=1 brew ruby -rformula -e 'Cask::Cask.all' (branch = master)
  1. b797782

Resolve todo to remove ARGV references in Cask::Cask.all by adding the :eval_all parameter. I figured I'd just do this while I was in the area.

We were only loading casks from files with this command which,
of course, didn't work for casks that can only be loaded from
the API (when the core cask tap is not tapped). This changes
things to align more with what we do for formulae.
Now it takes the :eval_all parameter that means we can remove
ARGV handling inside Cask::Cask.all.
@apainintheneck apainintheneck added cask Homebrew Cask bug Reproducible Homebrew/brew bug labels Dec 22, 2023
@apainintheneck
Copy link
Contributor Author

@manuel-koch It would be great if you could test out the brew search command using this branch to double-check that this fix works for your use case.

@apainintheneck apainintheneck marked this pull request as ready for review December 22, 2023 07:49
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 great, thanks again @apainintheneck!

@MikeMcQuaid MikeMcQuaid merged commit 0b804d4 into Homebrew:master Dec 22, 2023
26 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Reproducible Homebrew/brew bug cask Homebrew Cask outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search is inconsistent, can't use brew search to find a cask that is available via brew info
2 participants