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

New allow list: mismatched_binary_allowlist #11832

Merged
merged 1 commit into from Aug 11, 2021

Conversation

AkihiroSuda
Copy link
Contributor

@AkihiroSuda AkihiroSuda commented Aug 6, 2021

This is needed by Lima, which installs non-native binaries for supporting ARM-on-Intel / Intel-on-ARM emulation.

See Homebrew/homebrew-core#82723 (comment)

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

@carlocab
Copy link
Member

carlocab commented Aug 6, 2021

Instead of creating another allowlist, why don't we just remove from the mismatches hash the keys whose prefixes match the share directory?

@AkihiroSuda
Copy link
Contributor Author

AkihiroSuda commented Aug 6, 2021

@carlocab I'm not familiar with the code base, but the comment in the existing code says There is no binary_executable_or_library_files method for the generic OS, that's probably the reason

@carlocab
Copy link
Member

carlocab commented Aug 6, 2021

I'm not sure I follow. The reason for what?

@AkihiroSuda
Copy link
Contributor Author

The reason for "why don't we just remove from the mismatches hash the keys whose prefixes match the share directory".

@carlocab
Copy link
Member

carlocab commented Aug 6, 2021

I... still don't follow.

There's no reason why my suggestion is technically infeasible. At least, if it were, it would certainly not be due to the generic OS. I'm pretty sure you can implement it with something like

diff --git a/Library/Homebrew/formula_cellar_checks.rb b/Library/Homebrew/formula_cellar_checks.rb
index 1ba4a41a7..03dfbc451 100644
--- a/Library/Homebrew/formula_cellar_checks.rb
+++ b/Library/Homebrew/formula_cellar_checks.rb
@@ -325,6 +325,7 @@ module FormulaCellarChecks
       farch = file.arch
       mismatches[file] = farch unless farch == Hardware::CPU.arch
     end
+    mismatches.reject! { |f| f.first.to_s.start_with? formula.share }
     return if mismatches.empty?
 
     compatible_universal_binaries, mismatches = mismatches.partition do |file, arch|

@AkihiroSuda AkihiroSuda changed the title New allow list: check_binary_arches_allowlist check_binary_arches(): skip checking formula.share Aug 6, 2021
@AkihiroSuda
Copy link
Contributor Author

@carlocab Updated to use your code.

I thought we may want to exclude more directories, but we can revisit them later.

@carlocab
Copy link
Member

carlocab commented Aug 6, 2021

I did not test my suggestion, so please test it first with brew audit --strict to make sure it does what we expect it to.

@@ -325,6 +325,7 @@ def check_binary_arches(formula)
farch = file.arch
mismatches[file] = farch unless farch == Hardware::CPU.arch
end
mismatches.reject! { |f| f.first.to_s.start_with? formula.share }
Copy link
Member

Choose a reason for hiding this comment

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

Let's also add a comment here that we ignore share because some formulae (e.g. lima) intentionally install non-native binaries into share.

That said, share is typically used for architecture-independent data so I'm now wondering if this is indeed the right approach here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and tested on Ubuntu.

@AkihiroSuda AkihiroSuda marked this pull request as draft August 6, 2021 12:57
@AkihiroSuda AkihiroSuda marked this pull request as ready for review August 6, 2021 13:24
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'm fine with this but it seems a little weird to make a global exception based on a single formula. I'd probably favour and allowlist but don't feel strongly either way.

@carlocab
Copy link
Member

carlocab commented Aug 9, 2021

Yes, in retrospect, an allowlist is better. Apologies for my going back and forth on this!

Let's try something like this instead:

diff --git a/Library/Homebrew/formula_cellar_checks.rb b/Library/Homebrew/formula_cellar_checks.rb
index 1ba4a41a7..721baa069 100644
--- a/Library/Homebrew/formula_cellar_checks.rb
+++ b/Library/Homebrew/formula_cellar_checks.rb
@@ -338,9 +338,12 @@ module FormulaCellarChecks
     end
     return if mismatches.empty? && universal_binaries_expected
 
+    mismatches_expected = formula.tap.blank? || tap_audit_exception(:mismatched_binary_allowlist, formula.name)
+    return if compatible_universal_binaries.empty? && mismatches_expected
+
     s = ""
 
-    if mismatches.present?
+    if mismatches.present? && !mismatches_expected
       s += <<~EOS
         Binaries built for a non-native architecture were installed into #{formula}'s prefix.
         The offending files are:

I think we want to be able to be a bit more selective about what we're allowing formulae to do, rather than just remove this audit entirely.

As before, please test this first, as I haven't tested this patch.

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda
Copy link
Contributor Author

Updated. Tested on Ubuntu 21.04.

@AkihiroSuda AkihiroSuda changed the title check_binary_arches(): skip checking formula.share New allow list: mismatched_binary_allowlist Aug 11, 2021
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.

Thanks, @AkihiroSuda!

@carlocab carlocab merged commit 2d2aff3 into Homebrew:master Aug 11, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2021
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

3 participants