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

filter pinned test dependencies #18597

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

muneebmahmed
Copy link
Contributor

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Addresses #18554. When upgrading formulae, check_install_sanity is called, which seems to walk the dependency tree for a formula to create a full list of dependencies and then - when determining the unsatisfied pinned dependencies - filters out the dependencies tagged as build. A problem with this is that build dependencies might have other non-build dependencies themselves, which won't be tagged as build but will still be present in the dependencies list, despite not being required for the formula being poured from bottle.

In the example from #18554, llvm had been pinned, and was preventing the upgrade of libomp, even though it was not required for the bottle. This was because libomp had a :build dependency on lit, which in turn had a :test dependency on llvm.

This PR just adds an additional check for test before adding to the pinned_unsatisfied_deps list, and in the related issue I suggested it might be better to prune the expansion of build/test dependencies somewhere here.
However, that might more trouble than it's worth for an issue that is not likely to affect many users, especially since there's a simple solution of just unpinning the formulae in question.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Seems reasonable; thanks!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks!

@MikeMcQuaid MikeMcQuaid merged commit bac7b68 into Homebrew:master Oct 21, 2024
27 checks passed
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.

4 participants