Skip to content

upgrade: keep requested order in keg-only groups - #24092

Merged
MikeMcQuaid merged 1 commit into
Homebrew:mainfrom
zbeekman:upgrade-stable-keg-only-order
Sep 26, 2026
Merged

MikeMcQuaid merged 1 commit into
Homebrew:mainfrom
zbeekman:upgrade-stable-keg-only-order

Conversation

@zbeekman

@zbeekman zbeekman commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

brew upgrade --formula a b c … doesn't keep the order the formulae were named in. upgrade.rb#L106-L116 moves keg-only formulae first using sort! with a comparator that returns 0 for formulae of the same kind. Ruby's sort! isn't stable, so the requested order within each group gets scrambled. The dependency sort that follows (#L118-L124) keeps its input order, so the scramble reaches the install order.

This PR uses partition(&:keg_only?) instead. Keg-only formulae still go first, and the requested order within each group is kept. In my synthetic testing and benchmarking this was also ~20% faster than sort! although both took very little absolute time. I will try to run brew benchmark once some long running LLVM and Flang builds finish. (The joys of still owning an Intel mac...)

Why: on machines that build from source, users may want to order upgrades deliberately (e.g. quick builds before slow ones), and that order is currently lost.

Reproduce (prints shfmt eza fzf jq ripgrep tree wget yq shellcheck bat):

brew ruby -e 'f = %w[bat eza fzf jq ripgrep tree wget yq shellcheck shfmt].map { |n| Formula[n] }; f.sort! { |a, b| (!a.keg_only? && b.keg_only?) ? 1 : ((a.keg_only? && !b.keg_only?) ? -1 : 0) }; puts f.map(&:name).join(" ")'

See also: #23551


  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

AI disclosure: I used Claude Code (Claude Opus 5.5) to trace the ordering logic and to draft the reproduction, fix, test and this description. I reviewed all of it, confirmed the reproduction, and ran the new test (fails before the fix, passes after), brew tests --only=cmd/upgrade and brew lgtm locally.


New edits with some benchmarking numbers:

brew benchmark

Here are the results of brew benchmark although they don't seem to touch the brew upgrade code we changed too much so their results are somewhat meaning less:

brew benchmark sl figlet cowsay cmatrix fortune sipcalc (3 runs, Intel macOS):

main (86650d0):

operation workload       batch      wall  slowest phases
install   metadata cold      1    7.742s  extraction 2288ms, api_metadata_load 1394ms, pour 1238ms
install   archive cold       1    5.832s  extraction 2133ms, pour 1244ms, curl_body 285ms
install   archive warm       1    5.852s  extraction 2277ms, pour 1435ms, startup 247ms
install   fully warm         1    1.142s  startup 262ms, command_load 223ms, formula_resolution 119ms
fetch     metadata cold      1    2.911s  api_metadata_load 1226ms, curl_body 419ms, command_load 376ms
fetch     archive cold       1    1.466s  command_load 351ms, curl_body 308ms, startup 252ms
fetch     archive warm       1    1.165s  command_load 352ms, startup 253ms, cli_parse 59ms

This PR (20cd416):

operation workload       batch      wall  slowest phases
install   metadata cold      1    7.360s  extraction 2210ms, pour 1268ms, api_metadata_load 1117ms
install   archive cold       1    5.805s  extraction 2120ms, pour 1182ms, curl_body 296ms
install   archive warm       1    5.476s  extraction 2149ms, pour 1184ms, startup 251ms
install   fully warm         1    1.106s  startup 251ms, command_load 231ms, formula_resolution 117ms
fetch     metadata cold      1    2.827s  api_metadata_load 1173ms, command_load 366ms, curl_body 363ms
fetch     archive cold       1    1.464s  command_load 356ms, curl_body 294ms, startup 253ms
fetch     archive warm       1    1.160s  command_load 355ms, startup 254ms, cli_parse 59ms

Custom benchmarking

Micro-benchmark: keg-only ordering in Upgrade.formula_installers

Setup

  • 2.4 GHz 8-core Intel Core i9 (x86_64), macOS Tahoe, Ruby 4.0.7 via brew ruby.
  • Three implementations of the reorder at upgrade.rb:106, benchmarked side by side in one script:
    • original: sort! with the keg-only comparator block (current main)
    • partition + locals: keg_only, not_keg_only = list.partition(&:keg_only?) then list.replace(keg_only + not_keg_only)
    • partition inline (this PR): list.replace(list.partition(&:keg_only?).flatten(1))
  • Inputs:
    • Synthetic: n = 10, 100, 1,000 and 10,000 structs responding to keg_only?, about 20% keg-only (srand(42)).
    • Real: Formula objects for every name in my Cellar: 492 loaded (7 from untrusted taps skipped), 33 keg-only.
  • Benchmark.bmbm (rehearsal pass, then measured pass). Each repetition reorders a fresh list.dup; the copy costs the same for all three.
  • Correctness checked on every input: all three produce the same keg-only/non-keg-only grouping; both partition versions keep the original order within each group; sort! does not.

Results (time per call, measured pass)

Input n keg-only original sort! partition + locals partition inline (PR) PR vs original
synthetic 10 3 4.19 µs 3.69 µs 4.12 µs 1.6% faster
synthetic 100 27 32.4 µs 21.3 µs 24.3 µs 25% faster
synthetic 1,000 215 275 µs 182 µs 211 µs 23% faster
synthetic 10,000 2,004 2.70 ms 1.77 ms 1.96 ms 27% faster
real formulae 492 33 0.771 ms 0.363 ms 0.360 ms 53% faster (2.1x)

The PR's version is as fast as or faster than sort! at every size, and about twice as fast on real Formula objects, where each keg_only? call costs more and partition makes n calls where sort! makes up to four per comparison over O(n log n) comparisons.

brew benchmark (install and fetch workloads) doesn't run this code: its install runs set HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK, so formula_installers returns early at upgrade.rb:104. Its results for main and this PR are within run-to-run noise.

@zbeekman

Copy link
Copy Markdown
Contributor Author

small issue on non-macos builds/tests. I have a fix that I will push in a second after I thoroughly test it locally.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The ordering fix is correct and covered by a focused regression test.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
What changed in this PR

Preserves requested formula upgrade order while prioritising keg-only formulae.

Changes:

  • Replaces unstable sorting with stable partitioning.
  • Adds regression coverage for ordering.
File Description
Library/​Homebrew/​upgrade.rb Stably groups keg-only formulae first.
Library/​Homebrew/​test/​upgrade_spec.rb Verifies relative ordering is retained.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread Library/Homebrew/upgrade.rb Outdated
@zbeekman

Copy link
Copy Markdown
Contributor Author

The failing tests appear to be orthogonal to my changes:

  • GitHub::get_artifact_urls: the event_payload artifact has expired (also failing on other PRs).
  • Utils::Output#odeprecated (2 examples): dev-cmd/audit_spec.rb leaves Homebrew.auditing = true, which silences odeprecated when it runs first in the same process. Reproduce with brew tests --no-parallel --seed 57449 --only=dev-cmd/audit,utils/output. I could probably work up a fix for that one. LMK if you think it worthwhile.

@zbeekman zbeekman changed the title upgrade: keep requested order within keg-only groups upgrade: keep requested order in keg-only groups Sep 25, 2026
@zbeekman
zbeekman force-pushed the upgrade-stable-keg-only-order branch from 1d981b5 to 20cd416 Compare September 25, 2026 19:25
@zbeekman

Copy link
Copy Markdown
Contributor Author

@MikeMcQuaid Should I take a stab at fixing the odeprecated issue? OK to add a commit to this PR? or does it need its own branch/PR?

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Izaak Beekman <zbeekman@gmail.com>
Date: Fri, 25 Sep 2026 16:00:00 -0400
Subject: [PATCH] tests: reset `Homebrew.auditing` between examples

- `brew audit` specs set `Homebrew.auditing` and never reset it, so
  later `odeprecated` examples in the same process print nothing and
  fail depending on the random seed.
---
 Library/Homebrew/test/spec_helper.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb
index 100be27f84..6525a97b95 100644
--- a/Library/Homebrew/test/spec_helper.rb
+++ b/Library/Homebrew/test/spec_helper.rb
@@ -309,6 +309,7 @@ RSpec.configure do |config|

   config.around do |example|
     Homebrew.raise_deprecation_exceptions = true
+    Homebrew.auditing = false

     Tap.installed.each(&:clear_cache)
     Cacheable::Registry.clear_all_caches
--
2.51.0

@zbeekman
zbeekman requested a balanced review from Copilot September 25, 2026 20:01
@zbeekman
zbeekman marked this pull request as ready for review September 25, 2026 20:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The focused implementation preserves existing precedence and includes appropriate regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

- `Array#sort!` is not stable, so the order formulae were named in
  was scrambled within the keg-only and non-keg-only groups.
- `partition` keeps keg-only formulae first and the requested order.
@MikeMcQuaid
MikeMcQuaid force-pushed the upgrade-stable-keg-only-order branch from 20cd416 to 7dcb151 Compare September 26, 2026 00:27
@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Sep 26, 2026
Merged via the queue into Homebrew:main with commit ca6e46b Sep 26, 2026
51 checks passed
@zbeekman
zbeekman deleted the upgrade-stable-keg-only-order branch September 26, 2026 14:40
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.

3 participants