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

Set correct tap when loading installed casks #17823

Merged
merged 3 commits into from
Sep 11, 2024
Merged

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Jul 21, 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?

Fixes #17416

This PR uses the new cask tabs to set the correct tap when loading an installed cask.

I have added two new CaskLoader methods:

  1. load_installed_cask: this method is analogous to Formulary::from_keg. It accepts a token and returns a cask with that token from the tap specified in the install receipt of the cask. Essentially, if slack was installed from test/tap, running load_installed_cask("slack") will return the test/tap/slack cask and not the homebrew/cask/slack cask. Note: if a full name is passed, the tap specified in the name will be used, even if a different tap exists in the install receipt.
  2. load_from_installed_caskfile: this method takes an installed caskfile and loads a cask from it, making sure to set the correct tap. If passed a path that isn't an installed caskfile or doesn't exist, it will raise a CaskUnavailableError.

This means that Cask::Caskroom::casks will now return the correct casks when a third-party cask is installed and shares a name with a cask in homebrew/cask. This means that brew list --full-name will work correctly.

Additionally, handling of named arguments when requesting kegs or keg-like-casks (i.e. installed formulae/casks). Now, specifying a full name will raise an error if the installed formula with that short name does not have a matching tap. For example, if test/tap/hello is installed, brew uninstall homebrew/core/hello will not uninstall the formula. brew uninstall hello and brew uninstall test/tap/hello will still work as expected.

@Rylan12
Copy link
Member Author

Rylan12 commented Jul 21, 2024

I accidentally duplicated some of the work in #17821, so this might need to be modified around that PR depending on which route makes more sense

@apainintheneck apainintheneck added the cask Homebrew Cask label Jul 21, 2024
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.

The changes to named args seem like good sanity checks but it's not obvious to me that they accurately handle migrated/renamed packages or prevent name conflicts when trying to load installed packages. I'm still not sure where or how this should be handled. It doesn't need to be handled in this PR though; I'm just thinking out loud.

We currently have named_args [:installed_formula, :installed_cask] throughout the codebase but as far as I can tell that information is only really used for documentation and not acted upon anywhere.

In some places, we load formula from kegs or racks and that sort of works but we don't have a good way of doing the same thing for casks yet since that possibility is so new.

Would it be helpful to have a keg for casks that represents the directory in the cask room where the cask has been installed? This would give us easy access to the installed cask file, install receipt and config settings without having to load a cask at all. Naming might be tricky though (shelf, stand, etc.).

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

The changes to Homebrew::Cli::NamedArgs look good to me. I looked at the code a bit more and it seems that the #to_*_kegs methods are used in brew list, brew uninstall, brew link and brew unlink. The last three of those could potentially cause hard to debug problems if the wrong formula or cask is used.

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 @Rylan12! Feel free to resolve any comments and self-merge when you're ready.

Library/Homebrew/cask/cask_loader.rb Outdated Show resolved Hide resolved
@Rylan12
Copy link
Member Author

Rylan12 commented Aug 12, 2024

Sorry all, I know there are unanswered questions here and it's been a while. I haven't forgotten and I will return to this in the next two or three days

Copy link

github-actions bot commented Sep 3, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added stale No recent activity and removed stale No recent activity labels Sep 3, 2024
Rylan12 and others added 2 commits September 10, 2024 13:34
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
@Rylan12
Copy link
Member Author

Rylan12 commented Sep 10, 2024

Sorry for disappearing off the face of the earth for a few weeks...

I've made some changes to address the open comments a little bit

We currently have named_args [:installed_formula, :installed_cask] throughout the codebase but as far as I can tell that information is only really used for documentation and not acted upon anywhere.

Not important to this PR but: these also affect shell completions

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

@MikeMcQuaid MikeMcQuaid merged commit 057ee66 into master Sep 11, 2024
29 checks passed
@MikeMcQuaid MikeMcQuaid deleted the load-installed-casks branch September 11, 2024 07:57
@apainintheneck
Copy link
Contributor

Great work here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cask Homebrew Cask
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong tap when listing installed casks from custom taps using --json=v2 --installed
4 participants