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

cmd/info: only display keg info if tap matches #19295

Merged
merged 2 commits into from
Feb 16, 2025
Merged

Conversation

ZhongRuoyu
Copy link
Member

  • 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 #19294.

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
EricFromCanada Eric Knibbe
Fixes #19294.
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Seems fine though do we want to handle the case where a tab tap is nil any differently?

@Bo98 Bo98 added this pull request to the merge queue Feb 16, 2025
Merged via the queue into master with commit 7436173 Feb 16, 2025
30 checks passed
@Bo98 Bo98 deleted the brew-info-check-keg branch February 16, 2025 22:43
@jasonkarns
Copy link
Contributor

Thank you, this resolves the core bug.

I am curious on the overall expected use of homebrew with taps, however. The recent trajectory of homebrew has been to promote more third party taps and less in core. (Removing build options and whatnot such that homebrew core is the 80% happy path, and pushing customization into taps.)

However, the overall use of homebrew does not make taps a "friendly" experience. To wit: brew list lists formulae short names. So a naive process of:

  1. brew list
  2. find formula
  3. brew info <formula-found-in-list-output>

Will not give desired results. brew list lists a formula name that is installed by a tap. Then brew info gives the core formula, not the tap. So on one had brew list says "formula X" is installed. Then brew info X says "Not Installed".

While this PR fixes the core bug (brew-info providing false information), it doesn't improve the overall cohesive use of brew with the ever-growing-presence of 3rd party taps.

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.

I think this should probably be reverted. We don't have many(/any?) other commands where we require fully scoping formulae from taps like this. Instead it feels like we should use the actual formula from the tap if the opt-linked keg is from that tap.

Thoughts?

@jasonkarns
Copy link
Contributor

jasonkarns commented Feb 17, 2025

@MikeMcQuaid I think agree. And that would also improve the cohesiveness issue I commented above. If I have (tap) formula X installed, and I say brew info X, it's most likely the case that I want to see the info for the formula that's actually installed. (I'm looking for installation date; or a reprint of the caveats, or I'm trying to get the URL that I can share with a colleague.)

The downside, then, is that info reports on different formula based on the state of my system. Which seems like it shouldn't be the case. (Info seems like it should be effectively stateless, aside from including installation date details.) I'd also be concerned that switching to the tap formula would make it impossible (?) to see info on the shadowed/core formula? It's rarely my desire to see the core formula info when I have a tap that's shadowing it, but perhaps others have differing needs?

@jrobotham-square
Copy link

FYI We're seeing this change break some of our internal tooling that uses brew info under the hood to decide if a tool installed from one of our taps has already been installed. (it's not using the fully qualified tap name when it runs brew info)

We should be able to work around it, but I suspect this is a breaking change that may affect many others.

@jasonkarns
Copy link
Contributor

@jrobotham-square I'm curious how your scripts have been detecting whether the tool that's installed is from core or your tap? (That was the bug this originally attempted to address.)

@Bo98
Copy link
Member

Bo98 commented Feb 18, 2025

For scripts you should be using --json as we don't guarantee stable output in the CLI output, though seems to follow the old behaviour and is thus now incosistent with this new behaviour.

A possible solution here is to:

  • Keep this new behaviour for explicit homebrew/core/<name>
  • Make brew info <formula> check for installed formulae and use those taps' formulae if it's installed (still loading the latest version from the tap itself for version info rather than the file in the install directory)
  • Update --json to match the above

I think that covers all cases?

@jrobotham-square
Copy link

I'm curious how your scripts have been detecting whether the tool that's installed is from core or your tap? (That was the bug this originally attempted to address.)

Short answer - I don't think our internal tooling was actually doing this properly and we've probably just been lucky it hasn't bitten us earlier. (it's some tooling our team has inherited)

For scripts you should be using --json as we don't guarantee stable output in the CLI output, though seems to follow the old behaviour and is thus now incosistent with this new behaviour.

Yeah - makes sense - being coupled to the brew info console output isn't great.

FWIW Our script currently has a mix of fully qualified references and short names. I think we've been relying on this bug for it to work up until now.
I think the best path forward for us is to always use the full reference.

@MikeMcQuaid
Copy link
Member

  • Make brew info <formula> check for installed formulae and use those taps' formulae if it's installed (still loading the latest version from the tap itself for version info rather than the file in the install directory)
  • Update --json to match the above

This seems ideal for now. Reverting this until then in #19324 until then.

The recent trajectory of homebrew has been to promote more third party taps and less in core. (Removing build options and whatnot such that homebrew core is the 80% happy path, and pushing customization into taps.)

I disagree with this statement.

While this PR fixes the core bug (brew-info providing false information), it doesn't improve the overall cohesive use of brew with the ever-growing-presence of 3rd party taps.

The only real "fix" here is something we've recommended for a long time but should consider adding more warnings and/or documentation for: using the same names as homebrew/core formulae in taps is simply a bad idea with a lot of rough edges, some of which we can improve, some we cannot.

@jasonkarns
Copy link
Contributor

jasonkarns commented Mar 1, 2025

This seems ideal for now. Reverting this until then in #19324 until then.

👍

Is there an issue already in place to capture the future state? (I searched but didn't find one) I can open a new one, or reopen #19294 (which is again an issue with the above PR reverted)

The recent trajectory of homebrew has been to promote more third party taps and less in core. (Removing build options and whatnot such that homebrew core is the 80% happy path, and pushing customization into taps.)

I disagree with this statement.

I should have couched this in more equivocating terms. I didn't mean to speak for homebrew team. This was the inference I took from a lot of gh issues being closed with, effectively, "make or use a tap". My only reason to mention this: it is my understanding that taps are still an expected and supported mechanism for distributing tools that don't receive broad community use. And that third party taps aren't being deprecated or discouraged as a feature. (If they are, then homebrew would have little incentive to ensure core commands and features continue to play well with taps.)

using the same names as homebrew/core formulae in taps is simply a bad idea with a lot of rough edges, some of which we can improve, some we cannot.

Understood. Though it's worth mentioning that in this case, the git-sync formula in the tap was created over a year before the conflicting one was added to core. (git-sync added to https://github.com/jacobwgillespie/homebrew-tap in June 2021; kubernetes' git-sync added to core in Aug 2022). It doesn't make sense for third party taps to squat on formula names; so I'm not suggesting that the tap should or could have reserved "git-sync" for itself. However, I think it is worth recognizing that if name conflicts between core and taps cause such issues, we shouldn't assume that the core formula came first. In such cases as this, the tap formula existed first and was working fine. (Indeed, it was following the recommended practice of choosing a unique name that did not conflict with core.) And then through no fault of and no change by the tap, suddenly it was in conflict with one from core and causing issues.

That implies that the addition of new formula in core can effectively break existing workflows (brew commands and scripts using them) involving taps. That seems problematic.

@MikeMcQuaid
Copy link
Member

However, I think it is worth recognizing that if name conflicts between core and taps cause such issues, we shouldn't assume that the core formula came first.

We shouldn't but tap providers should consider if it's worthwhile continuing to maintain something that homebrew/core does with the same name, particularly given the previously stated issues.

And then through no fault of and no change by the tap, suddenly it was in conflict with one from core and causing issues.

This is the nature of a relatively fast-moving rolling release package manager like Homebrew. See also: we deprecate things and these also require changes from taps to maintain identical end-user behaviour

That implies that the addition of new formula in core can effectively break existing workflows (brew commands and scripts using them) involving taps. That seems problematic.

Break? No, I don't think so. Cause suboptimal workflows/output? Yes. Given how taps and kegs are implemented: I don't really see any solution to this requiring a huge migration for all taps when seems much worse than the status-quo.

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

Successfully merging this pull request may close these issues.

Brew info reports an incorrect formula URL from a tap
5 participants