brew bump-formula-pr and brew bump-cask-pr fail

xrisk commented 1 year ago

`brew doctor` output

Your system is ready to brew.

Verification

[X] My "brew doctor output" above says Your system is ready to brew. and am still able to reproduce my issue.
[X] I ran brew update twice and am still able to reproduce my issue.
[X] This issue's title and/or description do not reference a single formula e.g. brew install wget. If they do, open an issue at https://github.com/Homebrew/homebrew-core/issues/new/choose instead.

`brew config` output

HOMEBREW_VERSION: 4.0.15-126-g01e0c20
ORIGIN: https://github.com/Homebrew/brew
HEAD: 01e0c20d01c6aab28d844280d130f0e745e2ddd9
Last commit: 8 hours ago
Core tap JSON: 01 May 02:21 UTC
HOMEBREW_PREFIX: /opt/homebrew
HOMEBREW_CASK_OPTS: []
HOMEBREW_DISPLAY: /private/tmp/com.apple.launchd.LZcoXQR5bk/org.xquartz:0
HOMEBREW_EDITOR: nvim
HOMEBREW_GITHUB_API_TOKEN: set
HOMEBREW_MAKE_JOBS: 8
Homebrew Ruby: 2.6.10 => /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/bin/ruby
CPU: octa-core 64-bit arm_firestorm_icestorm
Clang: 14.0.3 build 1403
Git: 2.40.1 => /opt/homebrew/bin/git
Curl: 7.87.0 => /usr/bin/curl
macOS: 13.3.1-arm64
CLT: 14.3.0.0.1.1679647830
Xcode: N/A
Rosetta 2: false

What were you trying to do (and why)?

Update the WhatsApp cask.

What happened (include all command output)?

~
08:01:13 ❯ brew bump whatsapp
==> whatsapp
Current cask version   :  2.2317.10
Latest livecheck version: unable to get versions
Latest Repology version:  2.2318.10
Open pull requests:       none
Closed pull requests:     none

~
08:01:22 ❯ brew bump-cask-pr --version 2.2318.10 whatsapp
Error: This cask's tap is not a Git repository!

What did you expect to happen?

I expected brew to create a PR.

Step-by-step reproduction instructions (by running `brew` commands)

08:01:27 ❯ brew bump-cask-pr -d --version 2.2318.10 whatsapp
Error: This cask's tap is not a Git repository!
Error: Kernel.exit
/opt/homebrew/Library/Homebrew/extend/kernel.rb:83:in `exit'
/opt/homebrew/Library/Homebrew/extend/kernel.rb:83:in `odie'
/opt/homebrew/Library/Homebrew/dev-cmd/bump-cask-pr.rb:76:in `bump_cask_pr'
/opt/homebrew/Library/Homebrew/brew.rb:94:in `<main>'

or

2. 08:03:27 ❯ brew bump-formula-pr -d p7zip
/opt/homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /opt/homebrew/opt/p7zip/.brew/p7zip.rb
Error: This formula is not in a tap!
Error: Kernel.exit
/opt/homebrew/Library/Homebrew/extend/kernel.rb:83:in `exit'
/opt/homebrew/Library/Homebrew/extend/kernel.rb:83:in `odie'
/opt/homebrew/Library/Homebrew/dev-cmd/bump-formula-pr.rb:112:in `bump_formula_pr'
/opt/homebrew/Library/Homebrew/brew.rb:94:in `<main>'

apainintheneck commented 1 year ago

To bump formulae or casks you need to have a local tap which is essentially a git repo with all of the package files. These then can get changed and the resulting changes pushed to the remote repos using normal git operations. These dev commands do that work for you for trivial version upgrades.

The problem here is that it seems you're using the API to manage core casks and formulae. This is fine for all of the normal install, upgrade, reinstall and uninstall actions but means that you don't have local taps installed at all. Instead, you're getting the information in a bundle of JSON from the API.

Long story short, it's saying here that you can't create a pull request using changes from your local tap because you don't have one. If I remember correctly you'll need to tap homebrew/core and homebrew/cask to be able to do that along with setting HOMEBREW_NO_INSTALL_FROM_API so that it knows to load packages from the local taps instead of the API.

I wonder if this is documented anywhere already and if it'd be helpful to maybe add some better messaging here for this specific edge case.

MikeMcQuaid commented 1 year ago

I wonder if this is documented anywhere already and if it'd be helpful to maybe add some better messaging here for this specific edge case.

Agreed. This command (and others like it) should error if core/cask aren't tapped in cases like this.

apainintheneck commented 1 year ago

Actually, I think I misdiagnosed this. We already force HOMEBREW_NO_INSTALL_FROM_API with these commands. What I don't understand is how you're able to load a formula or cask from a local tap and then later on it says that it's not a git repo. I'm confused about where these are getting loaded from though.

It looks like the formula is not actually being loaded from a tap. It's getting loaded from the opt directory instead.

08:03:27 ❯ brew bump-formula-pr -d p7zip /opt/homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /opt/homebrew/opt/p7zip/.brew/p7zip.rb

It should look something like this.

/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/p7zip.rb

Could you maybe try running this command in the console?

echo '
cask = :whatsapp.c; nil
cask.sourcefile_path
cask.tap.path
cask.tap.git?

formula = :p7zip.f
formula.tap.path
formula.tap.git?
' | HOMEBREW_NO_INSTALL_FROM_API=1 brew irb

xrisk commented 1 year ago

@apainintheneck

It's getting loaded from the opt directory instead

isn’t this a consequence of the fact that my HOMEBREW_PREFIX is /opt/homebrew, which is the default on Apple Silicon I think?

I tried running your commands, the cask example gives


07:36:00 ❯ echo '
           cask = :whatsapp.c; nil
           cask.sourcefile_path
           cask.tap.path
           cask.tap.git?

           ' | HOMEBREW_NO_INSTALL_FROM_API=1 brew irb
==> Interactive Homebrew Shell
Example commands available with: `brew irb --examples`

WARNING: This version of ruby is included in macOS for compatibility with legacy software.
In future versions of macOS the ruby runtime will not be available by
default, and may require you to install an additional package.

Switch to inspect mode.

cask = :whatsapp.c; nil
nil
cask.sourcefile_path
nil
cask.tap.path
#<Pathname:/opt/homebrew/Library/Taps/homebrew/homebrew-cask>
cask.tap.git?
false

And the formula gives:

07:37:45 ❯ echo '
           formula = :p7zip.f
           formula.tap.path
           formula.tap.git?
           ' | HOMEBREW_NO_INSTALL_FROM_API=1 brew irb
==> Interactive Homebrew Shell
Example commands available with: `brew irb --examples`

WARNING: This version of ruby is included in macOS for compatibility with legacy software.
In future versions of macOS the ruby runtime will not be available by
default, and may require you to install an additional package.

Switch to inspect mode.

formula = :p7zip.f
==> Tapping homebrew/core
Cloning into '/opt/homebrew/Library/Taps/homebrew/homebrew-core'…

(it starts cloning the core repo, I didn’t let it finish)

apainintheneck commented 1 year ago

Sorry, that the command I gave you ended up trying to clone the core formula repo. I didn't expect that to happen.

It's unfortunately not that simple. There seem to be three situations that can happen with the bump-*-pr commands.

Before I start here are my taps:

~ $ brew tap
altinity/clickhouse
homebrew/cask-versions
mongodb/brew

The formula/cask is in a local tap.

~ $ brew bump-formula-pr --dry-run --debug clickhouse
/opt/homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /opt/homebrew/Library/Taps/altinity/homebrew-clickhouse/Formula/clickhouse@22.7.rb
...

It gets loaded from **/Library/Taps/**/{package-name}.rb as expected.

The core tap is not installed locally because we are using the API but we can load the formula/cask because we've already installed it.

~ $ brew ls pandoc
/opt/homebrew/Cellar/pandoc/3.1.2/bin/pandoc
/opt/homebrew/Cellar/pandoc/3.1.2/etc/bash_completion.d/pandoc
/opt/homebrew/Cellar/pandoc/3.1.2/share/man/man1/pandoc.1
~ $ brew bump-formula-pr --dry-run --debug pandoc
/opt/homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /opt/homebrew/opt/pandoc/.brew/pandoc.rb
Error: This formula is not in a tap!
...

It loads the cached version of the formula/cask file as best as I can tell. This is cached during installation. It's not in a tap though so the error is appropriate but not that helpful.

The core tap is not installed locally because we are using the API and we haven't installed the formula/cask.

~ [1]$ brew ls luau
Error: No such keg: /opt/homebrew/Cellar/luau
~ [1]$ brew bump-formula-pr --dry-run --debug luau
Error: No available formula with the name "luau".
...

We aren't able to load the formula/cask at all so it dies in the parser.

It's pretty clear that the final two should have more informative errors. Neither mention the need to download a local version of a core tap.

Does it even make sense to load the cached file in the case where we're forcing HOMEBREW_NO_INSTALL_FROM_API and the formula/cask was originally loaded from the API? I assume this logic is here to allow you to interact with packages from taps that no longer exist but it doesn't really fit here.

I wonder if we could somehow handle this inside Homebrew::CLI::NamedArgs and then provide some generic messaging for API users that don't have local versions of the core taps.

MikeMcQuaid commented 1 year ago

It's pretty clear that the final two should have more informative errors. Neither mention the need to download a local version of a core tap.

Agreed 👍🏻

Does it even make sense to load the cached file in the case where we're forcing HOMEBREW_NO_INSTALL_FROM_API and the formula/cask was originally loaded from the API? I assume this logic is here to allow you to interact with packages from taps that no longer exist but it doesn't really fit here.

I don't think so.

I wonder if we could somehow handle this inside Homebrew::CLI::NamedArgs and then provide some generic messaging for API users that don't have local versions of the core taps.

Sounds ideal and Formulary and raise/odie if not.

Bo98 commented 1 year ago

It's pretty clear that the final two should have more informative errors. Neither mention the need to download a local version of a core tap.

Agreed 👍🏻

I wonder if we could somehow handle this inside Homebrew::CLI::NamedArgs and then provide some generic messaging for API users that don't have local versions of the core taps.

Sounds ideal and Formulary and raise/odie if not.

I've worked on this a bit and I've got something locally that can say that a full clone is required. This linked a bit with #15049, where basically I'm moving the HOMEBREW_NO_INSTALL_FROM_API command list from a global shell setting to being scoped around formula loads (mainly CLI parser - e.g. named_args :formula, without_api: true), which works much better in mixed tap scenarios.

I'm not entirely sure on the messaging yet though. We can tell users to brew tap Homebrew/core but there is the "stale tap" problem.

Currently, if you have a local tap, brew update will not update it and it's up to you to git pull manually. So we have the issue where running brew bump-formula-pr, which abstracts git commands away, is paired with a system that expects you to be maintaining it yourself using git pull.

Not sure what the best general solution is. The bump commands are in the auto-update list so special casing is possible there, though that assumes no other command has been run recently in the auto-update interval (1 hour). We could have brew update update taps (if installed) for devcmdrun users.

The question does expand a bit beyond the bump commands. For example, how should brew edit formula behave if run on a 6 month old tap. It probably should at least be giving a warning.

MikeMcQuaid commented 1 year ago

Currently, if you have a local tap, brew update will not update it and it's up to you to git pull manually. So we have the issue where running brew bump-formula-pr, which abstracts git commands away, is paired with a system that expects you to be maintaining it yourself using git pull.

Some sort of brew update --actuallyupdatealltapsplz (not that name hopefully obviously) would be good to be able to both tell people to run in similar messages and for triggering as part of the brew bump-* auto-update.

It probably should at least be giving a warning.

For something like brew edit: I like the idea of a warning. I'd expect anyone who would use it to be able to cd and run git pull.

MikeMcQuaid commented 1 year ago

Fixed by https://github.com/Homebrew/brew/pull/15563

Homebrew / brew