Skip to content

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?

This broke in fa68f3d (#20895). Since Formula#tap_path returns the theoretical/canonical location within a tap that a formula will live, it may not correspond to the actual location of the formula file (e.g., when a non-official tap is sharded). Using Formula#path instead correctly tracks the actual file location.

Fixes #20917.

This broke in fa68f3d (#20895). Since `Formula#tap_path` returns the
theoretical/canonical location within a tap that a formula will live, it
may not correspond to the actual location of the formula file (e.g.,
when a non-official tap is sharded). Using `Formula#path` instead
correctly tracks the actual file location.

Fixes #20917.
@ZhongRuoyu ZhongRuoyu requested review from Rylan12 and Copilot October 21, 2025 20:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a regression in formula version tracking introduced in commit fa68f3d. The issue occurred when Formula#tap_path (which returns the canonical/theoretical location) was used instead of Formula#path (which returns the actual file location), causing problems with version tracking for non-official sharded taps.

Key Changes:

  • Changed formula.tap_path to formula.path in FormulaVersions#initialize to track the actual formula file location instead of the theoretical one

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

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!

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Oct 22, 2025
Merged via the queue into main with commit 3a1ad1d Oct 22, 2025
37 checks passed
@MikeMcQuaid MikeMcQuaid deleted the formula-versions-tap-path branch October 22, 2025 08:02
@Rylan12
Copy link
Member

Rylan12 commented Oct 22, 2025

Sorry I wasn't able to see this yesterday. This is fine for now, but if HOMEBREW_USE_INTERNAL_API becomes the default, I expect that this will break brew bottle in homebrew/core CI (which is why it was done in the first place). It sounds like we need a more reliable way to figure out this path in the future

@MikeMcQuaid
Copy link
Member

@Rylan12 Maybe a stupid question: why would homebrew/core CI be using the (internal or current) API logic at all when it's got a full GitHub checkout available?

@Rylan12
Copy link
Member

Rylan12 commented Oct 22, 2025

@MikeMcQuaid sorry let me clarify: it's not actually about the API, it's because the Formulary::resolve changes from #20897 are gated by the HOMEBREW_USE_INTERNAL_API variable.

The issue is with calculating the bottle rebuild:

  • brew bottle is passed a formula name
  • We run Formulary::resolve on that name to get the installed formula
  • Now, formula.path returns /opt/homebrew/opt/foo/.brew/foo.rb
  • When calculating the bottle rebuild, FormulaVersions uses /opt/homebrew/opt/foo/.brew/foo.rb as the path, when really it needs to use the tap-path since that's where the git history lives
  • There's no git history for /opt/homebrew/opt/foo/.brew/foo.rb, so the rebuild is assumed to be 0 always

Example reproduction instructions, which work regarless of HOMEBREW_NO_INSTALL_FROM_API:

$ brew install --build-bottle hello
==> Fetching downloads for: hello
✔︎ Formula hello (2.12.2)
==> ./configure --disable-silent-rules
==> make install
==> Not running 'post_install' as we're building a bottle
You can run it manually using:
  brew postinstall hello
🍺  /opt/homebrew/Cellar/hello/2.12.2: 56 files, 355.0KB, built in 29 seconds
==> Running `brew cleanup hello`...

$ brew bottle --json hello --only-json-tab
==> Determining hello bottle rebuild... /opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/h/hello.rb
==> Bottling hello--2.12.2.arm64_tahoe.bottle.1.tar.gz...
./hello--2.12.2.arm64_tahoe.bottle.1.tar.gz
  bottle do
    rebuild 1
    sha256 arm64_tahoe: "45b9d5dbc87e6cf7509c4e5e099ff7889d65c1e1710c2ef57d1e3d9b5896deaf"
  end

$ export HOMEBREW_USE_INTERNAL_API=1

$ rm ./hello--2.12.2.arm64_tahoe.bottle.1.tar.gz ./hello--2.12.2.arm64_tahoe.bottle.json

$ brew bottle --json hello --only-json-tab
==> Determining hello bottle rebuild... /opt/homebrew/opt/hello/.brew/hello.rb
==> Bottling hello--2.12.2.arm64_tahoe.bottle.tar.gz...
./hello--2.12.2.arm64_tahoe.bottle.tar.gz
  bottle do
    sha256 arm64_tahoe: "45b9d5dbc87e6cf7509c4e5e099ff7889d65c1e1710c2ef57d1e3d9b5896deaf"
  end

Notice that the first time, bottle rebuild was set to 1 (which is correct based on hello's existing bottle in homebrew/core), and the second time, it was set to 0

@MikeMcQuaid
Copy link
Member

@Rylan12 Thanks, makes sense. It seems like the formula.path that's being used for Git should "know" that inside of opt or Cellar will never be a correct Git repository and it needs to look inside the tap. .tap_path seems like the right thing in that case. Not sure what I'm missing as to why that's a problem?

@Rylan12
Copy link
Member

Rylan12 commented Oct 22, 2025

@MikeMcQuaid I believe the .tap_path logic from before this PR works fine except for non-core taps that have sharded their formulae.

This is because tap_path calls Tap#new_formula_path, which assumes only CoreTap is sharded and all other taps just have a single Formula/ directory.


From my perspective, it seems that the problem here is really that Tap#new_formula_path assumes only homebrew/core is sharded, and I'm guessing there are other issues with sharded third-party taps like brew create not working, or other things like that.

I don't have time right now to investigate more, unfortunately, but I can try to do so tonight or tomorrow.

@MikeMcQuaid
Copy link
Member

@Rylan12 We shouldn't be relying on new_formula_path to ever find existing formulae, only guess a good location for new ones when using `brew create.

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 audit --online --git --skip-style is Reporting That revision Should Only Increment By 1 When It Does Only Increment By 1

5 participants