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

fix tap migration renames with API #17555

Closed
wants to merge 3 commits into from
Closed

fix tap migration renames with API #17555

wants to merge 3 commits into from
+167 −21

Conversation

apainintheneck
Copy link
Contributor

@apainintheneck apainintheneck commented Jun 24, 2024 •
edited
Loading

  • 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 #17234

We don't load casks and formulae that used to exist in an external tap but have been moved and renamed to an API tap when using the API. Loading works correctly when the core formula or cask tap is tapped locally though.

This PR adds some logic to map tap migration renames to the API and check that when loading formulae and casks.

Todo:

  • Add tests
  • Manual testing

Rylan12
Rylan12 approved these changes Jun 24, 2024
View reviewed changes
Copy link
Member

@Rylan12 Rylan12 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! 👍

@apainintheneck apainintheneck added the install from api Relates to API installs label Jun 24, 2024
MikeMcQuaid
MikeMcQuaid approved these changes Jun 24, 2024
View reviewed changes
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.

Makes sense so far, thanks @apainintheneck!

Library/Homebrew/cask/cask_loader.rb Outdated Show resolved Hide resolved
Library/Homebrew/formulary.rb Outdated Show resolved Hide resolved
@apainintheneck
Copy link
Contributor Author

I've been thinking about this more and I just wanted to get some opinions on the order of execution for the formula and cask loaders. This PR changes things slightly for cask and formula short names. The question is how to handle ambiguous cask/formula short names.

Before

  • If the short name is a core cask/formula, it is evaluated to the core name even if it is ambiguous.
  • If the short name is still ambiguous, it returns an error.

After

  • If the short name is a core cask/formula, it is evaluated to the core name even if it is ambiguous.
  • If the short name has been migrated to a core cask/formula and we are using the API, it is evaluated to the core name even if it is ambiguous.
  • If the short name is still ambiguous, it returns an error.

Does this sound reasonable? Would it be better to try to match the old behavior a bit more? That would probably involve modifying the name loader to call the API loader.

@MikeMcQuaid
Copy link
Member

Does this sound reasonable?

Sounds reasonable, yes, thanks @apainintheneck ❤️

If it helps: in the medium-term I think we should avoid having homebrew/core and homebrew/cask names ever overlapping at all. Given they are both "official" and "auto-tapped": it's unnecessarily confusing for our names to overlap. Many users don't know (or care) about the differences between formulae and casks.

That said: formulae are more widely used and should be the "default".

@apainintheneck
fix tap migration renames with API
e16bde1 
We don't load casks and formulae that used to exist in an external
tap but have been moved and renamed to an API tap when using the
API. Loading works correctly when the core formula or cask tap
is tapped locally though.

This PR adds some logic to map tap migration renames to the API
and check that when loading formulae and casks.
@apainintheneck
Add tests
18d2373 
Check for the following:
- Tap migration rename to core tap can be loaded by short name
- Tap migration rename to core tap can be loaded by long name
- Tap migration renam that clashes with existing core tap short name
  is ignored in favor of loading the cask/formula from the core tap
@apainintheneck
show migrated from when loading packages from API
0c81a31 
This changes slightly how we load packaged migrated to core taps
when they are loaded using the API. Now it should show the warning
that the package has been migrated and renamed.
@apainintheneck apainintheneck force-pushed the fix-tap-migration-renames-with-api branch from fcaeae9 to 0c81a31 Compare June 29, 2024 22:38
@apainintheneck
Copy link
Contributor Author

The loading logic should be correct now. I tweaked things so that the warning message that the package has been renamed shows up correctly now. I still need to do some manual testing to make sure that the brew migrate command works correctly here. It might also be possible to add some tests for the migrator classes too.

Rylan12
Rylan12 reviewed Jun 29, 2024
View reviewed changes
Library/Homebrew/formulary.rb
!Homebrew::API::Formula.all_renames.key?(name)
return

ref = if (name = parse_name(ref))
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on naming this something like:

Suggested change
ref = if (name = parse_name(ref))
ref = if (name = parse_homebrew_core_name(ref))

Or something like that? That way it's clear that the "parsing" that's happening is only within the homebrew/core tap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe parse_core_name here and parse_core_token in Cask::CaskLoader?

@apainintheneck apainintheneck mentioned this pull request Jun 30, 2024
Merged
7 tasks
@apainintheneck
Copy link
Contributor Author

Closing in favor of #17599

@wenglaurie30
Copy link

fix-tap-migration-renames-with-api

1 similar comment
@wenglaurie30
Copy link

fix-tap-migration-renames-with-api

@wenglaurie30
Copy link

e16bde1

1 similar comment
@wenglaurie30
Copy link

e16bde1

@ZhongRuoyu ZhongRuoyu deleted the fix-tap-migration-renames-with-api branch July 1, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rylan12 Rylan12 Rylan12 approved these changes

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

Assignees
No one assigned
Labels
install from api Relates to API installs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

brew migrate doesn't recognise migrations to API taps
4 participants
@apainintheneck @MikeMcQuaid @wenglaurie30 @Rylan12