Skip to content

Optionally use download_queue for brew install #20245

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

Merged
merged 1 commit into from
Jul 18, 2025
Merged
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
6 changes: 3 additions & 3 deletions Library/Homebrew/dependency.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@ def installed?(minimum_version: nil, minimum_revision: nil)
end
return false unless formula

# If the opt prefix doesn't exist: we likely have an incomplete installation.
return false unless formula.opt_prefix.exist?

return true if formula.latest_version_installed?

return false if minimum_version.blank?

# If the opt prefix doesn't exist: we likely have an incomplete installation.
return false unless formula.opt_prefix.exist?

installed_keg = formula.any_installed_keg
return false unless installed_keg

Expand Down
12 changes: 10 additions & 2 deletions Library/Homebrew/download_queue.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@

module Homebrew
class DownloadQueue
sig { params(retries: Integer, force: T::Boolean).void }
def initialize(retries: 0, force: false)
sig { params(retries: Integer, force: T::Boolean, pour: T::Boolean).void }
def initialize(retries: 0, force: false, pour: false)
@concurrency = T.let(EnvConfig.download_concurrency, Integer)
@quiet = T.let(@concurrency > 1, T::Boolean)
@tries = T.let(retries + 1, Integer)
@force = force
@pour = pour
@pool = T.let(Concurrent::FixedThreadPool.new(concurrency), Concurrent::FixedThreadPool)
end

Expand All @@ -24,6 +25,10 @@ def enqueue(downloadable)
) do |download, force, quiet|
download.clear_cache if force
download.fetch(quiet:)
if pour && download.bottle?
UnpackStrategy.detect(download.cached_download, prioritize_extension: true)
.extract_nestedly(to: HOMEBREW_CELLAR)
end
end
end

Expand Down Expand Up @@ -168,6 +173,9 @@ def cancel
sig { returns(T::Boolean) }
attr_reader :quiet

sig { returns(T::Boolean) }
attr_reader :pour

sig { returns(T::Hash[Downloadable, Concurrent::Promises::Future]) }
def downloads
@downloads ||= T.let({}, T.nilable(T::Hash[Downloadable, Concurrent::Promises::Future]))
Expand Down
15 changes: 0 additions & 15 deletions Library/Homebrew/download_strategy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,6 @@ class AbstractDownloadStrategy

abstract!

# Extension for bottle downloads.
module Pourable
extend T::Helpers

requires_ancestor { AbstractDownloadStrategy }

sig { params(block: T.nilable(T.proc.params(arg0: String).returns(T.anything))).returns(T.nilable(T.anything)) }
def stage(&block)
ohai "Pouring #{basename}"
super
end
end

# The download URL.
#
# @api public
Expand Down Expand Up @@ -74,7 +61,6 @@ def initialize(url, name, version, **meta)
@cache = T.let(meta.fetch(:cache, HOMEBREW_CACHE), Pathname)
@meta = T.let(meta, T::Hash[Symbol, T.untyped])
@quiet = T.let(false, T.nilable(T::Boolean))
extend Pourable if meta[:bottle]
end

# Download and cache the resource at {#cached_location}.
Expand Down Expand Up @@ -826,7 +812,6 @@ class LocalBottleDownloadStrategy < AbstractFileDownloadStrategy
sig { params(path: Pathname).void }
def initialize(path)
@cached_location = T.let(path, Pathname)
extend Pourable
end
# rubocop:enable Lint/MissingSuper

Expand Down
92 changes: 70 additions & 22 deletions Library/Homebrew/formula_installer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ class FormulaInstaller
sig { returns(T::Boolean) }
attr_accessor :link_keg

sig { returns(T.nilable(Homebrew::DownloadQueue)) }
attr_accessor :download_queue

sig {
params(
formula: Formula,
Expand Down Expand Up @@ -136,9 +139,12 @@ def initialize(
@hold_locks = T.let(false, T::Boolean)
@show_summary_heading = T.let(false, T::Boolean)
@etc_var_preinstall = T.let([], T::Array[Pathname])
@download_queue = T.let(nil, T.nilable(Homebrew::DownloadQueue))

# Take the original formula instance, which might have been swapped from an API instance to a source instance
@formula = T.let(T.must(previously_fetched_formula), Formula) if previously_fetched_formula

@ran_prelude_fetch = T.let(false, T::Boolean)
end

sig { returns(T::Boolean) }
Expand Down Expand Up @@ -294,7 +300,7 @@ def install_bottle_for?(dep, build)
end

sig { void }
def prelude
def prelude_fetch
deprecate_disable_type = DeprecateDisable.type(formula)
if deprecate_disable_type.present?
message = "#{formula.full_name} has been #{DeprecateDisable.message(formula)}"
Expand All @@ -312,8 +318,24 @@ def prelude
end
end

# Needs to be done before expand_dependencies for compute_dependencies
fetch_bottle_tab if pour_bottle?

@ran_prelude_fetch = true
end

sig { void }
def prelude
prelude_fetch unless @ran_prelude_fetch

Tab.clear_cache

# Setup bottle_tab_runtime_dependencies for compute_dependencies
@bottle_tab_runtime_dependencies = formula.bottle_tab_attributes
.fetch("runtime_dependencies", []).then { |deps| deps || [] }
.each_with_object({}) { |dep, h| h[dep["full_name"]] = dep }
.freeze

verify_deps_exist unless ignore_deps?

forbidden_license_check
Expand Down Expand Up @@ -778,13 +800,15 @@ def install_dependencies(deps)
if deps.empty? && only_deps?
puts "All dependencies for #{formula.full_name} are satisfied."
elsif !deps.empty?
oh1 "Installing dependencies for #{formula.full_name}: " \
"#{deps.map(&:first).map { Formatter.identifier(_1) }.to_sentence}",
truncate: false
if deps.length > 1
oh1 "Installing dependencies for #{formula.full_name}: " \
"#{deps.map(&:first).map { Formatter.identifier(_1) }.to_sentence}",
truncate: false
end
deps.each { |dep, options| install_dependency(dep, options) }
end

@show_header = true unless deps.empty?
@show_header = true if deps.length > 1
end

sig { params(dep: Dependency).void }
Expand All @@ -808,6 +832,7 @@ def fetch_dependency(dep)
quiet: quiet?,
verbose: verbose?,
)
fi.download_queue = download_queue
fi.prelude
fi.fetch
end
Expand All @@ -830,7 +855,7 @@ def install_dependency(dep, inherited_options)
installed_keg = Keg.new(df.prefix)
tab ||= installed_keg.tab
tmp_keg = Pathname.new("#{installed_keg}.tmp")
installed_keg.rename(tmp_keg)
installed_keg.rename(tmp_keg) unless tmp_keg.directory?
end

if df.tap.present? && tab.present? && (tab_tap = tab.source["tap"].presence) &&
Expand Down Expand Up @@ -867,6 +892,7 @@ def install_dependency(dep, inherited_options)
verbose: verbose?,
)
oh1 "Installing #{formula.full_name} dependency: #{Formatter.identifier(dep.name)}"
fi.prelude
fi.install
fi.finish
# Handle all possible exceptions installing deps.
Expand Down Expand Up @@ -1337,9 +1363,13 @@ def fetch_dependencies

return if deps.empty?

oh1 "Fetching dependencies for #{formula.full_name}: " \
"#{deps.map(&:first).map { Formatter.identifier(_1) }.to_sentence}",
truncate: false
unless download_queue
dependencies_string = deps.map(&:first)
.map { Formatter.identifier(_1) }
.to_sentence
oh1 "Fetching dependencies for #{formula.full_name}: #{dependencies_string}",
truncate: false
end

deps.each { |(dep, _options)| fetch_dependency(dep) }
end
Expand All @@ -1360,15 +1390,18 @@ def previously_fetched_formula
def fetch_bottle_tab(quiet: false)
return if @fetch_bottle_tab

begin
formula.fetch_bottle_tab(quiet: quiet)
@bottle_tab_runtime_dependencies = formula.bottle_tab_attributes
.fetch("runtime_dependencies", []).then { |deps| deps || [] }
.each_with_object({}) { |dep, h| h[dep["full_name"]] = dep }
.freeze
rescue DownloadError, Resource::BottleManifest::Error
# do nothing
if (download_queue = self.download_queue) &&
(bottle = formula.bottle) &&
(manifest_resource = bottle.github_packages_manifest_resource)
download_queue.enqueue(manifest_resource)
else
begin
formula.fetch_bottle_tab(quiet: quiet)
rescue DownloadError, Resource::BottleManifest::Error
# do nothing
end
end
Comment on lines +1393 to 1403
Copy link
Member

@Bo98 Bo98 Jul 20, 2025

Choose a reason for hiding this comment

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

Need to incorporate the retry logic in

rescue DownloadError
raise unless fallback_on_error?
retry
rescue Resource::BottleManifest::Error
raise if @fetch_tab_retried
@fetch_tab_retried = true
resource.clear_cache
retry
since we're not calling fetch_bottle_tab anymore. Perhaps by some custom callbacks from the queue or something

The final # Do nothing rescue in the installer is also important for CI when we enable it there.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 I think this should do it? #20285

Copy link
Member

Choose a reason for hiding this comment

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

I don't see where that PR changes this

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 This is triggering a manual retry on bottle manifest download errors. That PR is automatically triggering a manual retry on any bottle manifest errors under the download queue.

Do you mean this logic needs reincorporated for the non-download queue case and that's missing/changed and still needs resolved beyond #20282?

Copy link
Member

@Bo98 Bo98 Jul 21, 2025

Choose a reason for hiding this comment

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

Oh right I see what you mean - I misunderstood there.

We do however also need a retry (with a clear_cache) if it gets a Resource::BottleManifest::Error triggered by verify_download_integrity of the bottle manifest:

def verify_download_integrity(_filename)
# We don't have a checksum, but we can at least try parsing it.
tab
end

Try modify a locally-cached manifest to e.g. remove all arm64 bottles and see if that works as a reproducer. (I've not retested this after the changes yet.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 gotcha, thanks, on it (and holding release on it)

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 Not blocking release but working on a fix for this as it only affects download queue case.

Copy link
Member Author

Choose a reason for hiding this comment

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


@fetch_bottle_tab = T.let(true, T.nilable(TrueClass))
end

Expand All @@ -1381,7 +1414,7 @@ def fetch
return if only_deps?
return if formula.local_bottle_path.present?

oh1 "Fetching #{Formatter.identifier(formula.full_name)}".strip
oh1 "Fetching #{Formatter.identifier(formula.full_name)}".strip unless download_queue

downloadable_object = downloadable
check_attestation = if pour_bottle?(output_warning: true)
Expand All @@ -1391,19 +1424,31 @@ def fetch
else
@formula = Homebrew::API::Formula.source_download(formula) if formula.loaded_from_api?

formula.fetch_patches
formula.resources.each(&:fetch)
if (download_queue = self.download_queue)
formula.enqueue_resources_and_patches(download_queue:)
else
formula.fetch_patches
formula.resources.each(&:fetch)
end

downloadable_object = downloadable

false
end
downloadable_object.fetch

if (download_queue = self.download_queue)
download_queue.enqueue(downloadable_object)
else
downloadable_object.fetch
end

# We skip `gh` to avoid a bootstrapping cycle, in the off-chance a user attempts
# to explicitly `brew install gh` without already having a version for bootstrapping.
# We also skip bottle installs from local bottle paths, as these are done in CI
# as part of the build lifecycle before attestations are produced.
if check_attestation &&
# TODO: support this for download queues at some point
download_queue.nil? &&
Homebrew::Attestation.enabled? &&
formula.tap&.core_tap? &&
formula.name != "gh"
Expand Down Expand Up @@ -1489,7 +1534,10 @@ def downloadable
sig { void }
def pour
HOMEBREW_CELLAR.cd do
downloadable.downloader.stage
# download queue has already done the actual staging but we'll lie about
# pouring now for nicer output
ohai "Pouring #{downloadable.downloader.basename}"
downloadable.downloader.stage unless download_queue
end

Tab.clear_cache
Expand Down
48 changes: 31 additions & 17 deletions Library/Homebrew/install.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
require "hardware"
require "development_tools"
require "upgrade"
require "download_queue"

module Homebrew
# Helper module for performing (pre-)install checks.
Expand Down Expand Up @@ -313,30 +314,43 @@ def install_formulae(
skip_post_install: false,
skip_link: false
)
unless dry_run
formulae_names_to_install = formula_installers.map { |fi| fi.formula.name }
return if formulae_names_to_install.empty?

if dry_run
ohai "Would install #{Utils.pluralize("formula", formulae_names_to_install.count,
plural: "e", include_count: true)}:"
puts formulae_names_to_install.join(" ")

formula_installers.each do |fi|
fi.prelude
fi.fetch
rescue CannotInstallFormulaError => e
ofail e.message
next
rescue UnsatisfiedRequirements, DownloadError, ChecksumMismatchError => e
ofail "#{fi.formula}: #{e}"
next
print_dry_run_dependencies(fi.formula, fi.compute_dependencies, &:name)
end
return
end

if dry_run
if (formulae_name_to_install = formula_installers.map { |fi| fi.formula.name })
ohai "Would install #{Utils.pluralize("formula", formulae_name_to_install.count,
plural: "e", include_count: true)}:"
puts formulae_name_to_install.join(" ")

formula_sentence = formulae_names_to_install.map { |name| Formatter.identifier(name) }.to_sentence
oh1 "Fetching downloads for: #{formula_sentence}", truncate: false
if EnvConfig.download_concurrency > 1
download_queue = Homebrew::DownloadQueue.new(pour: true)
formula_installers.each do |fi|
fi.download_queue = download_queue
end
end
begin
[:prelude_fetch, :prelude, :fetch].each do |step|
formula_installers.each do |fi|
print_dry_run_dependencies(fi.formula, fi.compute_dependencies, &:name)
fi.public_send(step)
rescue UnsatisfiedRequirements, DownloadError, ChecksumMismatchError => e
ofail "#{fi.formula}: #{e}"
next
end
download_queue&.fetch
rescue CannotInstallFormulaError => e
ofail e.message
next
end
return
ensure
download_queue&.shutdown
end

formula_installers.each do |fi|
Expand Down
3 changes: 3 additions & 0 deletions Library/Homebrew/retryable_download.rb
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ def verify_download_integrity(filename) = downloadable.verify_download_integrity
sig { override.returns(String) }
def download_name = downloadable.download_name

sig { returns(T::Boolean) }
def bottle? = downloadable.is_a?(Bottle)

private

sig { returns(Downloadable) }
Expand Down
7 changes: 5 additions & 2 deletions Library/Homebrew/test/cmd/deps_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,13 @@
setup_test_formula "recommended_test"
setup_test_formula "installed"

# Mock `Formula#any_version_installed?` by creating the tab in a plausible keg directory
keg_dir = HOMEBREW_CELLAR/"installed"/"1.0"
# Mock `Formula#any_version_installed?` by creating the tab in a plausible keg directory and opt link
keg_dir = HOMEBREW_CELLAR/"installed/1.0"
keg_dir.mkpath
touch keg_dir/AbstractTab::FILENAME
opt_link = HOMEBREW_PREFIX/"opt/installed"
opt_link.parent.mkpath
FileUtils.ln_sf keg_dir, opt_link

expect { brew "deps", "baz", "--include-test", "--missing", "--skip-recommended" }
.to be_a_success
Expand Down
Loading
Loading