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

Display unbottled dependencies when skipping #717

Merged
merged 4 commits into from
Nov 18, 2021
Merged

Display unbottled dependencies when skipping #717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e41d68e
test_formulae: check deps of `:all` bottles in `#bottled?`
carlocab Nov 18, 2021
4210598
formulae: display unbottled dependencies when skipping
carlocab Nov 18, 2021
9e43057
formulae_dependents: remove unnecessary `:all` bottle handling
carlocab Nov 18, 2021
140e7d0
test_formulae: add explanatory comment
carlocab Nov 18, 2021
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
12 changes: 10 additions & 2 deletions lib/test_formulae.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,16 @@ def initialize(tap:, git:, dry_run:, fail_fast:, verbose:)

protected

def bottled?(formula, tag = nil, no_older_versions: false)
formula.bottle_specification.tag?(Utils::Bottles.tag(tag), no_older_versions: no_older_versions)
def bottled?(formula, no_older_versions: false)
# If a formula has an `:all` bottle, then all its dependencies have
# to be bottled too for us to use it. We only need to recurse
# up the dep tree when we encounter an `:all` bottle because
# a formula is not bottled unless its dependencies are.
if formula.bottle_specification.tag?(Utils::Bottles.tag(:all))
formula.deps.all? { |dep| bottled?(dep.to_formula, no_older_versions: no_older_versions) }
Comment on lines +21 to +22
Copy link
Member

Choose a reason for hiding this comment

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

Should this be recursive deps? Feels like a comment explaining why this is done would be useful.

Copy link
Member Author

@carlocab carlocab Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No need. This procedure guarantees that the recursive dependencies are bottled too because a formula won't be bottled unless its dependencies are. (The handful that were accidentally bottled during mass bottling have been bumped since.)

We only need to recurse further up the tree whenever we find an :all bottle, but that's what this method does already.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha.

Copy link
Contributor

Choose a reason for hiding this comment

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

This procedure guarantees that the recursive dependencies are bottled too because a formula won't be bottled unless its dependencies are. (The handful that were accidentally bottled during mass bottling have been bumped since.)

This may be the case in core, but it is not the case in osrf/simulation right now. There are some formulae with big_sur bottles that were built before Nov 3 against catalina or earlier bottles of at least one dependency. Now, when rebuilding these formulae, they lose their big_sur bottle since their dependency chain isn't completely bottled on big_sur (for example osrf/homebrew-simulation@140ece0).

I'll try to test and find a reproducible test case from the osrf/simulation tap. Thanks for you help with all this!

Copy link
Member Author

@carlocab carlocab Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Changing deps to recursive_dependencies here only makes your problem worse, not better, though. This discussion is also about formulae with :all bottles, which I guess you don't have yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough; I don't fully understand all this code. thanks for your patience

we don't yet have :all bottles; we need to resolve osrf/homebrew-simulation#1682 before we will get :all bottles automatically from test-bot

else
formula.bottle_specification.tag?(Utils::Bottles.tag, no_older_versions: no_older_versions)
end
end

def skipped(formula_name, reason)
Expand Down
14 changes: 9 additions & 5 deletions lib/tests/formulae.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,13 +247,17 @@ def formula!(formula_name, args:)
return
end

all_deps_have_compatible_bottles = formula.deps.all? do |dep|
bottled?(dep.to_formula)
end
deps_without_compatible_bottles = formula.deps
.map(&:to_formula)
.reject { |dep| bottled?(dep) }
bottled_on_current_version = bottled?(formula, no_older_versions: true)

if !all_deps_have_compatible_bottles && (!bottled_on_current_version || bottled?(formula, :all))
skipped formula_name, "#{formula_name} has dependencies without a compatible bottle!"
if deps_without_compatible_bottles.present? && !bottled_on_current_version
message <<~EOS
#{formula_name} has dependencies without compatible bottles:
#{deps_without_compatible_bottles * "\n "}
EOS
skipped formula_name, message
return
end

Expand Down
23 changes: 4 additions & 19 deletions lib/tests/formulae_dependents.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,15 @@ def dependent_formulae!(formula_name, args:)
dependents_for_formula(formula, formula_name, args: args)

source_dependents.each do |dependent|
# TODO: Work out how to use this code to identify
# dependents that can be bottled. See
# https://github.com/Homebrew/homebrew-test-bot/pull/678
next unless bottled?(dependent, no_older_versions: true)
next if bottled?(dependent, :all) && dependent.deps.any? do |d|
next if dependent.deps.any? do |d|
f = d.to_formula
built_formulae = @testing_formulae - skipped_or_failed_formulae

!bottled?(f, no_older_versions: true) && built_formulae.exclude?(f.full_name)
!bottled?(f) && built_formulae.exclude?(f.full_name)
end

install_dependent(dependent, testable_dependents, build_from_source: true, args: args)
install_dependent(dependent, testable_dependents, args: args)
install_dependent(dependent, testable_dependents, args: args) if bottled?(dependent)
end

bottled_dependents.each do |dependent|
Expand Down Expand Up @@ -103,18 +99,7 @@ def dependents_for_formula(formula, formula_name, args:)
source_dependents = source_dependents.transpose.first.to_a

testable_dependents = source_dependents.select(&:test_defined?)
bottled_dependents = dependents.select do |dep|
# If a dependent has an all bottle, we need to check that the dependent's dependencies
# have useable bottles. Otherwise, we can check the dependent directly.
next bottled?(dep, no_older_versions: true) unless bottled?(dep, :all)

dep.deps.all? do |d|
f = d.to_formula
built_formulae = @testing_formulae - skipped_or_failed_formulae

bottled?(f, no_older_versions: true) || built_formulae.include?(f.full_name)
end
end
bottled_dependents = dependents.select { |dep| bottled?(dep) }
testable_dependents += bottled_dependents.select(&:test_defined?)

info_header "Source dependents:"
Expand Down