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.

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

Setup brew determine-test-runners for macOS 15 x86_64 #18356

Open
1 task done
Bo98 opened this issue Sep 19, 2024 · 11 comments
Open
1 task done

Setup brew determine-test-runners for macOS 15 x86_64 #18356

Bo98 opened this issue Sep 19, 2024 · 11 comments
Labels
features New features help wanted We want help addressing this

Comments

@Bo98
Copy link
Member

Bo98 commented Sep 19, 2024

Verification

Provide a detailed description of the proposed feature

We're ready to enable macOS 15 x86_64 for PRs but the setup is a little more complex.

We need brew determine-test-runners to add macOS 15 x86_64 to the matrix if any of the following are true:

  • The package is pkg-config or pkgconf
  • formula.class.pour_bottle_only_if == :clt_installed
  • Any recursive dependent of the tested formula satisfies dependent_formula.class.pour_bottle_only_if == :clt_installed
  • --all-supported is passed

The last condition may require making --eval-all mandatory.

What is the motivation for the feature?

In order to have proper testing of macOS 15 x86_64 and not require manual post-merge building.

It is also blocking Homebrew/homebrew-core#190981.

How will the feature be relevant to at least 90% of Homebrew users?

They will get upgraded to compatible bottles rather than temporarily get incompatible bottles that break their workflow.

What alternatives to the feature have been considered?

None

@Bo98 Bo98 added help wanted We want help addressing this features New features labels Sep 19, 2024
@carlocab
Copy link
Member

  • Any recursive dependent of the tested formula satisfies dependent_formula.class.pour_bottle_only_if == :clt_installed

This currently includes all Python dependents, no? Though I suppose we want to and should fix that.

@Bo98
Copy link
Member Author

Bo98 commented Sep 19, 2024

  • Any recursive dependent of the tested formula satisfies dependent_formula.class.pour_bottle_only_if == :clt_installed

This currently includes all Python dependents, no? Though I suppose we want to and should fix that.

All Python dependencies.

[email protected] ended up being a dependency of LLVM anyway so we have already built that depedency tree.

@carlocab
Copy link
Member

  • Any recursive dependent of the tested formula satisfies dependent_formula.class.pour_bottle_only_if == :clt_installed

This currently includes all Python dependents, no? Though I suppose we want to and should fix that.

All Python dependencies.

So, should the original post instead say

  • Any recursive dependency of the tested formula satisfies dependent_formula.class.pour_bottle_only_if == :clt_installed

? (Or am I confused?)

@Bo98
Copy link
Member Author

Bo98 commented Sep 19, 2024

If you are building e.g. mpdecimal, a Python dependency, you need to check dependents to find Python.

(btw I've also added a note about --all-supported to the original issue as I forgot about that case)

@Bo98
Copy link
Member Author

Bo98 commented Sep 19, 2024

Alternatively: you could find all formula matching pour_bottle_only_if, calculate their dependency trees, union into one and then check for intersections between that list and the tested formulae.

Might be computationally cheaper given you don't need to check every dependency tree though haven't tried it out.

@MikeMcQuaid
Copy link
Member

  • --all-supported is passed

What is this for/to do?

  • Any recursive dependent of the tested formula satisfies dependent_formula.class.pour_bottle_only_if == :clt_installed

Could this be non-recursive?


Feels like adding a new DSL for this might be nicer and a bit less involved?

@carlocab
Copy link
Member

If you are building e.g. mpdecimal, a Python dependency, you need to check dependents to find Python.

Ah, gotcha, thanks for the explanation.

@Bo98
Copy link
Member Author

Bo98 commented Sep 19, 2024

What is this for/to do?

Cache workflow + brew doctor workflow here.

Could this be non-recursive?

No, we build full dependency trees. For the affected formulae this isn't that big if we maybe flip it and check the dependency trees rather than constructing dependent trees.

Feels like adding a new DSL for this might be nicer and a bit less involved?

The DSL is effectively there for most cases except the pkg-config exception. We just need it to cover the full dep tree.

@carlocab
Copy link
Member

carlocab commented Sep 19, 2024

  • Any recursive dependent of the tested formula satisfies dependent_formula.class.pour_bottle_only_if == :clt_installed

This is pretty straightforward to do now with TestRunnerFormula#dependents.

Though the implementation of that is really inefficient -- it walks Formula.all for each runner and tested formula. This is part of why it's really slow, but the slowness didn't matter as much previously (since it could run concurrently with formula builds).

Ideally it would construct the dependency tree once for each runner and then query that for each tested formula instead.

@Bo98
Copy link
Member Author

Bo98 commented Sep 19, 2024

We can probably walk through it once to fetch the pour_bottle formulae (which we can likely assume isn't OS-specific) and then only construct the dependency trees for those formulae.

@MikeMcQuaid
Copy link
Member

What is this for/to do?

Cache workflow + brew doctor workflow here.

Ok, gotcha.

For the affected formulae this isn't that big if we maybe flip it and check the dependency trees rather than constructing dependent trees.

This seems nicer.

The DSL is effectively there for most cases except the pkg-config exception. We just need it to cover the full dep tree.

I think it's kind of there but very unintuitive to folks without a deep understanding of Homebrew and/or macOS SDKs.

I think a new DSL with audits or cops to enforce them based on the cases we already know would be nice rather than an implied DSL like we have today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
features New features help wanted We want help addressing this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants