github_runner_matrix: deploy new x86_64 runner when required #18363
Conversation
| NEW_INTEL_MACOS_MUST_BUILD_FORMULAE = %w[pkg-config pkgconf].freeze | ||
|
|
||
| sig { returns(T::Boolean) } | ||
| def deploy_new_x86_64_runner? | ||
| return true if @testing_formulae.any? { |f| NEW_INTEL_MACOS_MUST_BUILD_FORMULAE.include?(f.name) } | ||
| return true if @testing_formulae.any? { |f| f.formula.class.pour_bottle_only_if == :clt_installed } | ||
|
|
||
| Formula.all.any? do |formula| | ||
| non_test_dependencies = Dependency.expand(formula, cache_key: "determine-test-runners") do |_, dependency| | ||
| Dependency.prune if dependency.test? | ||
| end | ||
| next false if non_test_dependencies.none? { |dep| @testing_formulae.map(&:name).include?(dep.name) } | ||
|
|
||
| formula.class.pour_bottle_only_if == :clt_installed | ||
| end | ||
| end |
There was a problem hiding this comment.
As mentioned before: I'd love to see this be a temporary stop-gap while we add a DSL. I think this needs some pretty hefty amounts of comments until then explaining why these particular formulae need to be doing what they are doing here.
| return true if @testing_formulae.any? { |f| NEW_INTEL_MACOS_MUST_BUILD_FORMULAE.include?(f.name) } | ||
| return true if @testing_formulae.any? { |f| f.formula.class.pour_bottle_only_if == :clt_installed } | ||
|
|
||
| Formula.all.any? do |formula| |
There was a problem hiding this comment.
can this instead use e.g. the relevant tap(s) instead of reading all formulae unconditionally here?
There was a problem hiding this comment.
I can skip formulae not in the given tap, but Tap objects don't seem to have a nice way to recover the formulae they contain. They can give me the formulae files, but not sure handling those directly is nicer than this.
There was a problem hiding this comment.
I think handling formulae files is always nicer than doing Formula.all and evaluating every formula, whether needed or not.
carlocab
force-pushed
the
deploy-new-x86_64-runner
branch
from
22ccc44
to
30e25c5
Compare
| return true if @testing_formulae.any? { |f| NEW_INTEL_MACOS_MUST_BUILD_FORMULAE.include?(f.name) } | ||
| return true if @testing_formulae.any? { |f| f.formula.class.pour_bottle_only_if == :clt_installed } | ||
|
|
||
| Formula.all.any? do |formula| |
There was a problem hiding this comment.
You probably need to pass eval_all here
There was a problem hiding this comment.
Yep, will add it when I get around to updating the calls in homebrew/core
| next if !@all_supported && macos_version > NEWEST_HOMEBREW_CORE_INTEL_MACOS_RUNNER | ||
| # `#deploy_new_x86_64_runner?` is expensive, so: | ||
| # - avoid calling it if we don't have to | ||
| # - cache the result to a variable to avoid calling it multiple times | ||
| if macos_version > NEWEST_DEFAULT_HOMEBREW_CORE_INTEL_MACOS_RUNNER && |
There was a problem hiding this comment.
Was removing the !@all_supported intentional?
There was a problem hiding this comment.
It's up here:
There was a problem hiding this comment.
Ah thanks, that wasn't obvious at first.
Closes #18356. Co-authored-by: Bo Anderson <mail@boanderson.me>
carlocab
force-pushed
the
deploy-new-x86_64-runner
branch
from
30e25c5
to
c8d1644
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
I'll get to this late next week (or in the first half of the week following that). |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?Closes #18356.