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

formula_auditor: create a versioned formula dependent conflict allowlist #13714

Merged
merged 1 commit into from
Aug 19, 2022

Conversation

carlocab
Copy link
Member

  • 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?

We have an audit that checks each formula's dependency tree for multiple
versions of the same software. We have an allowlist that allows us to
ignore this audit, but this allowlist requires each formula with a
conflict in its dependency tree to be listed there.

Here, I propose the reverse: if formula foo appears in the
versioned_formula_dependent_conflicts_allowlist, then all its
dependents will not fail the versioned dependencies conflict because of
a conflict with formula foo.

I'd like to do this in the case of python, where I think the versioned
dependencies conflict check hurts us more than helps us. Versioned
dependency conflicts are most problematic in the case of libraries with
the same install name but incompatible ABIs. This is almost never a
problem with Python: almost no formulae link with the Python framework
on macOS (in part due to one of our audits that disallows Python
framework linkage in Python modules). Moreover, the various Python
frameworks that we ship have the version in the install name.

The above might be a problem on Linux, since we allow unrestricted
linkage with libpython. However, we don't even check versioned
conflicts on Linux, so we aren't as concerned about this in the first
place.

This is also a lot more convenient than adding the dependents of some
Python formula one by one as they acquire conflicts due to changes in
other formulae.

I've also amended tap_auditor to allow the use of formula aliases in
an allowlist, to allow us to add python to this allowlist instead of
each individual versioned Python formula.

See also discussion at Homebrew/homebrew-core#108307.

I will open a PR in homebrew-core shortly to demonstrate what this
allowlist looks like.

We have an audit that checks each formula's dependency tree for multiple
versions of the same software. We have an allowlist that allows us to
ignore this audit, but this allowlist requires each formula with a
conflict in its dependency tree to be listed there.

Here, I propose the reverse: if formula `foo` appears in the
`versioned_formula_dependent_conflicts_allowlist`, then all its
dependents will not fail the versioned dependencies conflict because of
a conflict with formula `foo`.

I'd like to do this in the case of `python`, where I think the versioned
dependencies conflict check hurts us more than helps us. Versioned
dependency conflicts are most problematic in the case of libraries with
the same install name but incompatible ABIs. This is almost never a
problem with Python: almost no formulae link with the Python framework
on macOS (in part due to one of our audits that disallows Python
framework linkage in Python modules). Moreover, the various Python
frameworks that we ship have the version in the install name.

The above _might_ be a problem on Linux, since we allow unrestricted
linkage with `libpython`. However, we don't even check versioned
conflicts on Linux, so we aren't as concerned about this in the first
place.

This is also a lot more convenient than adding the dependents of some
Python formula one by one as they acquire conflicts due to changes in
other formulae.

I've also amended `tap_auditor` to allow the use of formula aliases in
an allowlist, to allow us to add `python` to this allowlist instead of
each individual versioned Python formula.

See also discussion at Homebrew/homebrew-core#108307.
@BrewTestBot
Copy link
Member

Review period will end on 2022-08-19 at 07:44:16 UTC.

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.

I'd like to do this in the case of python, where I think the versioned dependencies conflict check hurts us more than helps us.

I'm 👍🏻 on the fix, I just wonder if it's overly generic. Do you have plans to do this for non-python stuff in future? If not, perhaps we just hardcode python here for now?

@carlocab
Copy link
Member Author

I'm 👍🏻 on the fix, I just wonder if it's overly generic.

I did hardcode python here at first. And then I second-guessed that solution in anticipation of feedback that it would be weird to hardcode overly specific special cases in brew. 😅

Do you have plans to do this for non-python stuff in future?

I am contemplating doing the same for OpenSSL -- at least for the 1.1 -> 3 migration. But that's much more dangerous territory: lots of software link with some OpenSSL library, and, e.g. libcrypto.1.1.dylib and libcrypto.3.dylib export many of the same symbols, many of them likely to be incompatible.

@MikeMcQuaid
Copy link
Member

I am contemplating doing the same for OpenSSL -- at least for the 1.1 -> 3 migration. But that's much more dangerous territory: lots of software link with some OpenSSL library, and, e.g. libcrypto.1.1.dylib and libcrypto.3.dylib export many of the same symbols, many of them likely to be incompatible.

Yeh, I'd be strongly 👎🏻 on anything that actually has linkage against multiple versions in the same tree.

Python is (mostly) fine because most things just use it as an interpreter rather than linking to Python itself. It would be nice if somehow brew linkage could be extended to to this check more robustly, though, and we could remove from brew audit and brew install entirely in favour of actual linkage checks.

@carlocab
Copy link
Member Author

Yeh, I'd be strongly 👎🏻 on anything that actually has linkage against multiple versions in the same tree.

This is less of a concern these days with the two-level namespace on macOS (which we audit for!), because the linker also records information about which library a symbol was found in when linking a mach-o object. This mitigates concerns about situations foo links against libbar and libbaz but libbar and libbaz link against different versions of libssl.

It would be nice if somehow brew linkage could be extended to this check more robustly, though, and we could remove from brew audit and brew install entirely in favour of actual linkage checks.

Yes, agreed.


I thought about this check being overly generic, and perhaps it is. However, I think there is still value to the excepted formulae being listed in Homebrew/homebrew-core rather than in Homebrew/brew (where it could get old and crufty), so I think I'll stick with this implementation.

@MikeMcQuaid
Copy link
Member

However, I think there is still value to the excepted formulae being listed in Homebrew/homebrew-core rather than in Homebrew/brew (where it could get old and crufty), so I think I'll stick with this implementation.

I kinda feel the opposite, actually, which is I really don't want a homebrew-core maintainer to decide the fix to solve their problem is to just add more values to this file.

@carlocab
Copy link
Member Author

carlocab commented Aug 19, 2022

I kinda feel the opposite, actually, which is I really don't want a homebrew-core maintainer to decide the fix to solve their problem is to just add more values to this file.

Yes. I thought of this, which is why I was going to add @Homebrew/tsc to CODEOWNERS for this list.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 19, 2022
@BrewTestBot
Copy link
Member

Review period ended.

@carlocab carlocab merged commit d4e6925 into Homebrew:master Aug 19, 2022
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Aug 19, 2022
BrewTestBot pushed a commit to Homebrew/homebrew-core that referenced this pull request Aug 20, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 19, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants