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

utils: add check_binary_linkage function #19116

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

alebcay
Copy link
Member

@alebcay alebcay commented Jan 18, 2025

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

This PR adds Utils::check_binary_linkage, which is a helper-function that is already used in some (primarily Rust-based) formulae.

The function is more or less copied verbatim, except types are loosened to accept String or Pathname. An optional prefix parameter can also be set to change the filtering behavior - this flag really only exists because we have to tweak the filtering behavior a bit in the test. Maybe we can just not do any filtering and that'll be fine - but for now I've kept it as close to the original as possible.

Open to suggestions all around, but I'm a bit extra miffed about:

  • Where would be the best place to put this file, and the file name. There didn't appear to be another file in utils that seemed like a good place, so I created a new one.
  • The tests still feel a bit clunky to me and I think there's probably room for improvement. Note that we set prefix in invocations to Utils::check_binary_linkage because in the test environment, HOMEBREW_CELLAR is a sibling to HOMEBREW_PREFIX, not a child, so filtering on HOMEBREW_PREFIX will exclude all the libraries in HOMEBREW_CELLAR, unlike in a real Homebrew installation where the cellar is in HOMEBREW_PREFIX/"Cellar".

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.

Looks good to me if suggestion is applied (or similar naming to end in ?). If you're happy to apply suggestion as-is: feel free to merge without a rereview.

Library/Homebrew/utils/linkage.rb Outdated Show resolved Hide resolved
@alebcay alebcay force-pushed the utils-check-binary-linkage branch from 1e4048f to 796dc51 Compare January 20, 2025 02:38
@alebcay
Copy link
Member Author

alebcay commented Jan 20, 2025

  • Renamed the function to binary_linked_to_library?
  • Would appreciate some ideas on the test failure. The two added test cases seem to pass locally for me on macOS and in ubuntu22.04 brew container. Looks like the compilation of the test program is failing, but I'm not able to reproduce the failure (or would be helpful to get some verbose/debug output for why the command is failing)

@alebcay alebcay force-pushed the utils-check-binary-linkage branch 2 times, most recently from 49ff39c to 53da757 Compare January 22, 2025 19:51
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

The only thing: is it worth adding two integration tests for this? I know in the past we've tried to limit integration tests because they're slow, but that may have changed now (it's been a while since I've reviewed a test-related Homebrew/brew PR)

I wonder if it's possible to test the function outside of a formula context to avoid needing to spin up the brew environment. Maybe just using a temporary directory and then running the shell commands there?

@Rylan12
Copy link
Member

Rylan12 commented Jan 23, 2025

Note that we set prefix in invocations to Utils::check_binary_linkage because in the test environment, HOMEBREW_CELLAR is a sibling to HOMEBREW_PREFIX, not a child, so filtering on HOMEBREW_PREFIX will exclude all the libraries in HOMEBREW_CELLAR, unlike in a real Homebrew installation where the cellar is in HOMEBREW_PREFIX/"Cellar".

I opened #19134 to move the test cellar to HOMEBREW_PREFIX/"Cellar" which might remove the need for this

@MikeMcQuaid
Copy link
Member

The only thing: is it worth adding two integration tests for this? I know in the past we've tried to limit integration tests because they're slow, but that may have changed now (it's been a while since I've reviewed a test-related Homebrew/brew PR)

No, I still agree with that. I think a 1-2 unit tests should be sufficient here.

@alebcay alebcay force-pushed the utils-check-binary-linkage branch from 53da757 to 2f0f02a Compare January 23, 2025 13:11
@alebcay
Copy link
Member Author

alebcay commented Jan 23, 2025

I opened #19134 to move the test cellar to HOMEBREW_PREFIX/"Cellar" which might remove the need for this

Thank you, updated this PR to remove the prefix parameter altogether, since it should work reliably with prefix = HOMEBREW_PREFIX.

I wonder if it's possible to test the function outside of a formula context to avoid needing to spin up the brew environment. Maybe just using a temporary directory and then running the shell commands there?

I did initially think about this, the challenge was invoking the compiler outside of a formula context to produce the libraries/binary to test with. I will revisit this and see if it could be improved.

Updated the test to run outside of a formula context. Still facing challenges with setting the rpath/dependency paths in the binary, which is something that the formula context usually takes care of.

@alebcay alebcay force-pushed the utils-check-binary-linkage branch from 2f0f02a to fd6b5cf Compare January 23, 2025 14:25
@alebcay alebcay force-pushed the utils-check-binary-linkage branch from fd6b5cf to 2373b6d Compare January 23, 2025 16:26
@MikeMcQuaid
Copy link
Member

Thanks @alebcay!

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Jan 24, 2025
Merged via the queue into Homebrew:master with commit f4371b8 Jan 24, 2025
28 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.

3 participants