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

Fix grpc_cli missing a library for execution #68383

Closed
wants to merge 8 commits into from

Conversation

jan-xyz
Copy link
Contributor

@jan-xyz jan-xyz commented Jan 6, 2021

This glob pattern doesn't match the actual file on macOS anymore.
On MacOS the file is called libgrpc++_test_config.1.dylib. Trying to run grpc_cli results in an error:

$ grpc_cli ls localhost:8080
dyld: Library not loaded: @rpath/libgrpc++_test_config.1.dylib
  Referenced from: /usr/local/bin/grpc_cli
  Reason: image not found
[1]    40338 abort      grpc_cli ls localhost:8080

I am not sure if the new pattern also matches in linux. Is there a way I can provide a test which makes sure we do not break it again? It broke during #67474

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

This glob pattern doesn't match the actual file on macOS anymore.
@BrewTestBot BrewTestBot added the no ARM bottle Formula has no ARM bottle label Jan 6, 2021
@SMillerDev
Copy link
Member

Is there a way I can provide a test which makes sure we do not break it again?

You can invoke the cli in the test block.

@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jan 6, 2021

@SMillerDev, thanks for the hint. It's my first patch to homebrew-core and I have 0 experience, so I really appreciate this "obvious" hint!

I added the test:
Without the patch on macOS:

$ brew test grpc
==> Testing grpc
==> /usr/bin/clang test.cpp -I/usr/local/Cellar/grpc/1.33.2_2/include -L/usr/local/Cellar/grpc/1.33.2_2/lib -lgrpc -o test
==> ./test
==> grpc_cli help ls
Last 15 lines from /Users/jan/Library/Logs/Homebrew/grpc/test.03.grpc_cli:
2021-01-06 13:42:33 +0100

grpc_cli
help
ls

dyld: Library not loaded: @rpath/libgrpc++_test_config.1.dylib
  Referenced from: /usr/local/bin/grpc_cli
  Reason: image not found
Error: grpc: failed
An exception occurred within a child process:
  BuildError: Failed executing: grpc_cli help ls
/usr/local/Homebrew/Library/Homebrew/formula.rb:2096:in `block in system'
/usr/local/Homebrew/Library/Homebrew/formula.rb:2032:in `open'
/usr/local/Homebrew/Library/Homebrew/formula.rb:2032:in `system'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/grpc.rb:83:in `block in <class:Grpc>'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1900:in `block (3 levels) in run_test'
/usr/local/Homebrew/Library/Homebrew/utils.rb:530:in `with_env'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1899:in `block (2 levels) in run_test'
/usr/local/Homebrew/Library/Homebrew/formula.rb:904:in `with_logging'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1898:in `block in run_test'
/usr/local/Homebrew/Library/Homebrew/mktemp.rb:63:in `block in run'
/usr/local/Homebrew/Library/Homebrew/mktemp.rb:63:in `chdir'
/usr/local/Homebrew/Library/Homebrew/mktemp.rb:63:in `run'
/usr/local/Homebrew/Library/Homebrew/formula.rb:2145:in `mktemp'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1892:in `run_test'
/usr/local/Homebrew/Library/Homebrew/test.rb:43:in `block in <main>'
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.6.3_2/lib/ruby/2.6.0/timeout.rb:93:in `block in timeout'
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.6.3_2/lib/ruby/2.6.0/timeout.rb:33:in `block in catch'
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.6.3_2/lib/ruby/2.6.0/timeout.rb:33:in `catch'
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.6.3_2/lib/ruby/2.6.0/timeout.rb:33:in `catch'
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.6.3_2/lib/ruby/2.6.0/timeout.rb:108:in `timeout'
/usr/local/Homebrew/Library/Homebrew/test.rb:42:in `<main>'

with the patch on macOS:

$ brew test grpc
==> Testing grpc
==> /usr/bin/clang test.cpp -I/usr/local/Cellar/grpc/1.33.2_2/include -L/usr/local/Cellar/grpc/1.33.2_2/lib -lgrpc -o test
==> ./test
==> grpc_cli help ls

Formula/grpc.rb Outdated Show resolved Hide resolved
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

This should work then. And it'll make sure it fails as we expect it to fail.

Formula/grpc.rb Outdated Show resolved Hide resolved
Co-authored-by: Sean Molenaar <[email protected]>
@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jan 6, 2021

Hmm the new test fails. I am trying to figure out locally why it doesn't match.

@iMichka
Copy link
Member

iMichka commented Jan 6, 2021

The .* at the end needs to remain, as the filename is something like .so.1 on linux if I remember well.

Formula/grpc.rb Outdated Show resolved Hide resolved
@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jan 6, 2021

The .* at the end needs to remain, as the filename is something like .so.1 on linux if I remember well.

I will re-add that :) Thanks for checking in. There is no CI test for Linux, right?

EDIT: @iMichka what do you think about libgrpc++_test_config.{1.dylib,so.1}? Or would you prefer libgrpc++_test_config*.{dylib,so}*

@iMichka
Copy link
Member

iMichka commented Jan 6, 2021

There is no CI test for Linux, right?

It's tested once it is merged into linuxbrew-core. The test is a little bit ... delayed :) But I will come back to this PR if it fails.

I would not hardcode the .1, that value might be different. Ideally there should just have been a single "make install" step so that we do not have to go through so many hoops to ship this.

Changing from libgrpc++_test_config*.{dylib,so}.* to libgrpc++_test_config*.{dylib,so}*, as the only change you make is to remove the mandatory dot at the end, which is not needed on Mac, but keep the * to tell it to include everything at the end on linux (I hope I got my ruby-regex-fu right).

@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jan 6, 2021

(I hope I got my ruby-regex-fu right)

It's actually a shell glob pattern :D

Changing from libgrpc++_test_config*.{dylib,so}.* to libgrpc++_test_config*.{dylib,so}*

Uhh yeah, that's a nice minimal change. I adjusted the PR accordingly, thanks!

carlocab
carlocab previously approved these changes Jan 6, 2021
iMichka
iMichka previously requested changes Jan 6, 2021
Copy link
Member

@iMichka iMichka left a comment

Choose a reason for hiding this comment

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

We need to bump the revision to 3 to ship the updated content to everyone.

@carlocab carlocab dismissed their stale review January 6, 2021 18:03

missed need for revision

@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jan 6, 2021

I bumped the revision. Sorry for that 😬

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@jan-xyz
Copy link
Contributor Author

jan-xyz commented Jan 7, 2021

Thank you everyone! This was super pleasant to work on and I really appreciate all the quick replies and feedback. It was a small fix but super motivating and rewarding. Have a great day ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no ARM bottle Formula has no ARM bottle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants