Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 0 additions & 94 deletions .github/workflows/command-not-found-db-update.yml

This file was deleted.

69 changes: 69 additions & 0 deletions Library/Homebrew/dev-cmd/which-entry.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# typed: strict
# frozen_string_literal: true

require "abstract_command"
require "formula"
require "formulary"

module Homebrew
module DevCmd
class WhichEntry < AbstractCommand
cmd_args do
description <<~EOS
Generate an `executables.txt` entry for <formula> using bottle manifest metadata.
EOS
flag "--output-db=",
description: "Append or update the entry in the given `executables.txt` database file."
named_args :formula, min: 1
end

sig { override.void }
def run
db_path = args.output_db
raise UsageError, "`--output-db` is required." unless db_path

args.named.to_formulae.each { |formula| process(formula, db_path: Pathname(db_path)) }
end

private

sig { params(formula: Formula, db_path: Pathname).void }
def process(formula, db_path:)
line = db_line(formula)
write_db(db_path, formula.full_name, line)
end

sig { params(db_path: Pathname, name: String, line: T.nilable(String)).void }
def write_db(db_path, name, line)
lines = db_path.readlines(chomp: true).compact_blank if db_path.exist?
# TODO: remove once pkg_versions are no longer in executables.txt
lines = Array(lines).reject { |l| l.start_with?(/#{Regexp.escape(name)}(?:\(.+\))?:/) }
lines << line if line
db_path.write("#{lines.sort.join("\n")}\n")
end

sig { params(formula: Formula).returns(T.nilable(String)) }
def db_line(formula)
return if formula.disabled? || formula.deprecated?

exes = executables_from_manifest(formula)
"#{formula.full_name}:#{exes.join(" ")}"
end

sig { params(formula: Formula).returns(T::Array[String]) }
def executables_from_manifest(formula)
manifest_resource = formula.bottle&.github_packages_manifest_resource
return [] unless manifest_resource

manifest_resource.fetch unless manifest_resource.downloaded?
manifest_path = manifest_resource.cached_download
return [] unless manifest_path.exist?
Comment on lines +59 to +60
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think .fetch is probably good enough here. It's called by default with verify_download_integrity: true, so I think we'll get an error before this point if we weren't able to download the manifest. I'd say we can just get rid of these lines since I'm not convinced they add additional safety. It seems appropriate to me for this to fail if the formula is bottled but unable to download the bottle manifest. That does seem error-worthy

Suggested change
manifest_path = manifest_resource.cached_download
return [] unless manifest_path.exist?


manifest_resource.path_exec_files
rescue JSON::ParserError => e
opoo "Failed to parse bottle manifest for #{formula.name}: #{e.message}"
[]
Comment on lines +63 to +65
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's get rid of this. BottleResource#manifest_annotations raises a nice error if the JSON doesn't parse, and I think we want a hard error here. Also it's a bit weird to rescue a JSON parse error when we don't actually call JSON.parse directly in this method.

Suggested change
rescue JSON::ParserError => e
opoo "Failed to parse bottle manifest for #{formula.name}: #{e.message}"
[]

end
Comment thread
ooye-sanket marked this conversation as resolved.
end
end
end
8 changes: 8 additions & 0 deletions Library/Homebrew/resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,14 @@ def installed_size
manifest_annotations["sh.brew.bottle.installed_size"]&.to_i
end

sig { returns(T::Array[String]) }
def path_exec_files
exec_files = manifest_annotations["sh.brew.path_exec_files"]
return [] if exec_files.blank?

exec_files.split.map { |f| File.basename(f) }.sort
end
Comment on lines +415 to +420
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This does not work correctly for annotations that have multiple entries. The entries are comma-separated, so split doesn't work.

Additionally, the File.basename doesn't belong in this method. This method should return the data directly, and leave it up to the caller how to process it.

Suggested change
def path_exec_files
exec_files = manifest_annotations["sh.brew.path_exec_files"]
return [] if exec_files.blank?
exec_files.split.map { |f| File.basename(f) }.sort
end
def path_exec_files
manifest_annotations.fetch("sh.brew.path_exec_files", "").split(",")
end


sig { override.returns(String) }
def download_queue_type = "Bottle Manifest"

Expand Down
16 changes: 16 additions & 0 deletions Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/which_entry.rbi

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions Library/Homebrew/test/dev-cmd/which-entry_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# typed: false
# frozen_string_literal: true

require "cmd/shared_examples/args_parse"
require "dev-cmd/which-entry"

RSpec.describe Homebrew::DevCmd::WhichEntry do
it_behaves_like "parseable arguments"

it "raises a UsageError if --output-db is not passed", :integration_test do
expect { brew "which-entry", "wget" }
.to output(/`--output-db` is required/).to_stderr
.and be_a_failure
end
end
Loading