Remove which-update; add which-entry to build executables DB from bottle manifests #21369
Conversation
|
Some commands I ran to test (in case this helps others): HOMEBREW_DEVELOPER=1 ./bin/brew which-entry --help
# Basic checks (print a single DB line):
HOMEBREW_DEVELOPER=1 ./bin/brew which-entry curl
HOMEBREW_DEVELOPER=1 ./bin/brew which-entry ripgrep
HOMEBREW_DEVELOPER=1 ./bin/brew which-entry jq
# Multiple formulas in one go:
HOMEBREW_DEVELOPER=1 ./bin/brew which-entry curl ripgrep jq
# Append/update a DB file (sorted) and inspect:
HOMEBREW_DEVELOPER=1 ./bin/brew which-entry --append-to=/tmp/executables.txt curl ripgrep jq
cat /tmp/executables.txt
# Force manifest-only (no tar fallback):
HOMEBREW_DEVELOPER=1 ./bin/brew which-entry --no-fallback curl |
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
@hmk Did you use AI/LLM to generate this? If so, which one and can you confirm you reviewed/understood every line of code before submitting this PR?
Yes — parts of this were assisted by AI. I used Claude Code for research and planning and Kilo for autocomplete. Models used: Claude Haiku 4.5, Claude Sonnet 4.5, and GPT-5.
Yes. I have reviewed every line of code and take responsibility for it. I understand the code as written. This is my first contribution to Homebrew, so I’m still building familiarity with the development workflow. Questions:
If I’m misunderstanding the intent or suggested implementation from that discussion, please let me know. |
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Reviewed. Code pretty hard to follow so far. Somewhat classic AI generation: comments for very obvious things but the choices made are not justified, the code isn't DRY, etc.
I'll probably do one more pass at this level of detail and if it's not good enough I'll pass and prompt an AI to do this myself.
|
|
||
| module Homebrew | ||
| module DevCmd | ||
| # `brew which-entry` command. |
There was a problem hiding this comment.
| # `brew which-entry` command. |
| cmd_args do | ||
| description <<~EOS | ||
| Generate an executables database entry for one or more formulae using | ||
| their bottle manifest metadata (sh.brew.path_exec_files). Optionally |
There was a problem hiding this comment.
| their bottle manifest metadata (sh.brew.path_exec_files). Optionally | |
| their bottle manifest metadata (in `sh.brew.path_exec_files`). Optionally |
| description <<~EOS | ||
| Generate an executables database entry for one or more formulae using | ||
| their bottle manifest metadata (sh.brew.path_exec_files). Optionally | ||
| append/update entries in an existing executables.txt file. |
There was a problem hiding this comment.
| append/update entries in an existing executables.txt file. | |
| append/update entries in an existing `executables.txt` file. |
| EOS | ||
|
|
||
| flag "--append-to=", | ||
| description: "Append or update entries in the given database file (e.g. executables.txt)." |
There was a problem hiding this comment.
| description: "Append or update entries in the given database file (e.g. executables.txt)." | |
| description: "Append or update entries in the given database file (e.g. `executables.txt`)." |
| sig { override.void } | ||
| def run | ||
| # Resolve all formula objects first | ||
| formulae = args.named.to_a.map { |name| Formulary.factory(name) } |
There was a problem hiding this comment.
| formulae = args.named.to_a.map { |name| Formulary.factory(name) } | |
| formulae = args.named.to_formulae_and_casks |
| end | ||
|
|
||
| existing_lines = if path.exist? | ||
| path.read.split("\n", -1).reject(&:empty?).map { |l| l.end_with?("\n") ? l : "#{l}\n" } |
There was a problem hiding this comment.
Split this to a .abc per line for readability. Also: this is somewhat incomprehensible.
| updated = [] | ||
| seen = {} | ||
| existing_lines.each do |line| | ||
| name = line[/\A([^()]+)\(/, 1] |
There was a problem hiding this comment.
Same on this regex. is this the same as above?
|
|
||
| DB_LINE_REGEX = /^(?<name>.*?)(?:\((?<version>.*)\)):(?<exes_line>.*)?$/ | ||
|
|
||
| # Represents a single formula entry in the executables database. |
There was a problem hiding this comment.
All these sort of AI generated comments should be removed.
| update_formula_binaries formula, binaries | ||
| end | ||
|
|
||
| class << self |
There was a problem hiding this comment.
just do def self.entry_from_manifest instead.
| # Fallback: scan the bottle tar directly | ||
| bottle.fetch |
Rylan12
left a comment
Rylan12
left a comment
There was a problem hiding this comment.
I will review more thoroughly once Mike's comments are addressed, but I don't think this is quite the right approach.
We should be removing all of this logic from this repo and instead writing it as a GitHub action in homebrew/core. Most of the logic in executables_db only exists to get user-friendly diff messages which we don't need once we're doing one-by-one updates, and we can easily pass the just-built bottles and metadata back and worth with github actions artifacts.
|
Thanks. I will close this for now in favor of opening a PR with a Github action in homebrew/core, then revisit this branch with a removal-only diff. |
3 participants
brew lgtm(style, typechecking and tests) with your changes locally?This PR implements the changes discussed in #20752 to remove the batch
which-updateworkflow and replace it with a small, maintainable path that derives executables database entries directly from bottle metadata.I chose to name it
which-entrybut am open to other suggestions.