Skip to content
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.

Sign up for GitHub

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

github_runner_matrix: deploy new x86_64 runner when required #18363

Closed
wants to merge 1 commit into from
Closed

github_runner_matrix: deploy new x86_64 runner when required #18363

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 26 additions & 2 deletions Library/Homebrew/github_runner_matrix.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def initialize(testing_formulae, deleted_formulae, all_supported:, dependent_mat
@dependent_matrix = T.let(dependent_matrix, T::Boolean)
@compatible_testing_formulae = T.let({}, T::Hash[GitHubRunner, T::Array[TestRunnerFormula]])
@formulae_with_untested_dependents = T.let({}, T::Hash[GitHubRunner, T::Array[TestRunnerFormula]])
@deploy_new_x86_64_runner = T.let(all_supported, T::Boolean)

@runners = T.let([], T::Array[GitHubRunner])
generate_runners!
Expand Down Expand Up @@ -122,13 +123,30 @@ def create_runner(platform, arch, spec, macos_version = nil)

NEWEST_HOMEBREW_CORE_MACOS_RUNNER = :sequoia
OLDEST_HOMEBREW_CORE_MACOS_RUNNER = :ventura
NEWEST_HOMEBREW_CORE_INTEL_MACOS_RUNNER = :sonoma
NEWEST_DEFAULT_HOMEBREW_CORE_INTEL_MACOS_RUNNER = :sonoma

sig { params(macos_version: MacOSVersion).returns(T::Boolean) }
def runner_enabled?(macos_version)
macos_version <= NEWEST_HOMEBREW_CORE_MACOS_RUNNER && macos_version >= OLDEST_HOMEBREW_CORE_MACOS_RUNNER
end

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|
Copy link
Member

Choose a reason for hiding this comment

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

can this instead use e.g. the relevant tap(s) instead of reading all formulae unconditionally here?

Copy link
Member Author

@carlocab carlocab Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think handling formulae files is always nicer than doing Formula.all and evaluating every formula, whether needed or not.

Copy link
Member

Choose a reason for hiding this comment

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

You probably need to pass eval_all here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, will add it when I get around to updating the calls in homebrew/core

next false if formula.class.pour_bottle_only_if != :clt_installed

non_test_dependencies = Dependency.expand(formula, cache_key: "determine-test-runners") do |_, dependency|
Dependency.prune if dependency.test?
Bo98 marked this conversation as resolved.
Show resolved Hide resolved
end
non_test_dependencies.any? { |dep| @testing_formulae.map(&:name).include?(dep.name) }
end
end

NEWEST_GITHUB_ACTIONS_INTEL_MACOS_RUNNER = :ventura
OLDEST_GITHUB_ACTIONS_INTEL_MACOS_RUNNER = :monterey
NEWEST_GITHUB_ACTIONS_ARM_MACOS_RUNNER = :sonoma
Expand Down Expand Up @@ -181,7 +199,13 @@ def generate_runners!
)
@runners << create_runner(:macos, :arm64, spec, macos_version)

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 &&
Comment on lines -184 to +205
Copy link
Member

Choose a reason for hiding this comment

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

Was removing the !@all_supported intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's up here:

brew/Library/Homebrew/github_runner_matrix.rb

Line 63 in 30e25c5

@deploy_new_x86_64_runner = T.let(all_supported, T::Boolean)

Copy link
Member

@Bo98 Bo98 Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah thanks, that wasn't obvious at first.

!(@deploy_new_x86_64_runner ||= deploy_new_x86_64_runner?)
next
end

github_runner_available = macos_version <= NEWEST_GITHUB_ACTIONS_INTEL_MACOS_RUNNER &&
macos_version >= OLDEST_GITHUB_ACTIONS_INTEL_MACOS_RUNNER
Expand Down
Loading