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

Make Tap::each respect the API and clear all tap caches before each test. #16710

Merged
merged 2 commits into from Feb 22, 2024

Conversation

reitermarkus
Copy link
Member

@reitermarkus reitermarkus commented Feb 20, 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?

Ensure that Tap::each includes CoreTap and CoreCaskTap when using the API. This ensures all caches can be cleared and Tap::reverse_tap_migrations_renames works correctly when these taps are not installed locally.

More general solution than #16680, which added this before one specific test.

@reitermarkus reitermarkus changed the title Make Tap::each respect the API and clear all tap caches before each test. Make Tap::each respect the API and clear all tap caches before each test. Feb 20, 2024
Library/Homebrew/tap.rb Outdated Show resolved Hide resolved
Library/Homebrew/tap.rb Show resolved Hide resolved
@@ -260,6 +261,7 @@
@__stdin.close

Formulary.clear_cache
Tap.each(&:clear_cache)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like without modifying Tap.each you could do this instead or something similar. I have not tested this at all though.

Suggested change
Tap.each(&:clear_cache)
Tap.each(&:clear_cache)
CoreTap.clear_cache
CoreCaskTap.clear_cache

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it will work, but it doesn't make sense for CoreTap and CoreCaskTap not to already be included in Tap.each. In fact, they may actually be included, depending on whether the tap directory exists in tests, which is even weirder.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should actually be done before the ENV is restored above since this depends on HOMEBREW_NO_INSTALL_FROM_API.

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

I think it might be worth just making the simple change here (#16710 (comment)) and then discuss how we want Tap.each to behave in a separate PR. It looks like this will change user facing behavior and it's a lot to review in one PR since it's used in a bunch of different places.

Library/Homebrew/tap.rb Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

Needs rebased now but looks good so far!

@MikeMcQuaid
Copy link
Member

brew typecheck needs fixed but otherwise looks good to me, nice work!

@reitermarkus reitermarkus merged commit deb0488 into Homebrew:master Feb 22, 2024
31 checks passed
@reitermarkus reitermarkus deleted the tap-each-clear-cache branch February 22, 2024 17:18
@apainintheneck
Copy link
Contributor

This helped solve one of the test problems in #16638. Thanks for catching this, @reitermarkus!

@apainintheneck apainintheneck mentioned this pull request Feb 25, 2024
7 tasks
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 27, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants