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.

Sign up for GitHub

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

Jump to bottom

op25 (new formula) #55327

Closed
wants to merge 4 commits into from
Closed

op25 (new formula) #55327

wants to merge 4 commits into from

Conversation

mikemorris
Copy link
Contributor

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

mikemorris
mikemorris commented May 27, 2020
View reviewed changes
Formula/op25.rb
depends_on "itpp"

def install
# TODO: apply patch if gnuradio >= 3.8?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently expects to build against [email protected] as explained in https://git.osmocom.org/op25/commit/?id=c26b51e0c1b5fd9eee0c5b13dc137050cd0c988b, refs #49733

mikemorris
mikemorris commented May 27, 2020
View reviewed changes
Formula/op25.rb
# TODO: apply patch if gnuradio >= 3.8?

args = std_cmake_args + %w[
-Wno-dev
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suppresses warnings intended for project development, but they're only visible when installing with --verbose, should this just be removed?

mikemorris
mikemorris commented May 27, 2020
View reviewed changes
Formula/op25.rb
libexec.install Dir["#{buildpath}/op25/gr-op25_repeater/apps/**"]

# FIXME: is this an appropriate way to install?
bin.install_symlink libexec/"rx.py" => "op25"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure if any of the Python application scripts in the apps directory should actually be renamed or installed to bin or if only installing into libexec would be more appropriate.

mikemorris
mikemorris commented May 27, 2020
View reviewed changes
Formula/op25.rb
depends_on "cppunit" => :build
depends_on "swig" => :build
depends_on "gnuradio"
depends_on "gr-osmosdr"
Copy link
Contributor Author

@mikemorris mikemorris May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Successfully builds against [email protected], will need to be updated if/when #50921 is unblocked and merged.

mikemorris
mikemorris commented May 27, 2020
View reviewed changes
Formula/op25.rb
depends_on "cmake" => :build
depends_on "cppunit" => :build
depends_on "swig" => :build
depends_on "gnuradio"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this defaults to building with macOS system Python 2.7 but I think should also build successfully with Python 3 (refs #54848). I tested building against the [email protected] formula from #49733 and it appeared to work.

mikemorris
mikemorris commented May 27, 2020
View reviewed changes
Formula/op25.rb
Formula["gr-osmosdr"].lib,
lib,
].each do |dep|
Pathname.glob(dep/"python*/site-packages").each do |path|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would using Language::Python.major_minor_version similar to https://github.com/Homebrew/homebrew-core/blob/master/Formula/urh.rb#L32 be a better option here?

@alebcay alebcay added the new formula PR adds a new formula to Homebrew/homebrew-core label May 27, 2020
@mikemorris
Copy link
Contributor Author

mikemorris commented May 27, 2020 •
edited
Loading

brew test-bot seems to be stuck on a linker issue with not finding symbols from libgnuradio-pmt.dylib that I'm not hitting locally...

Undefined symbols for architecture x86_64:
  "pmt::dict_has_key(boost::intrusive_ptr<pmt::pmt_base> const&, boost::intrusive_ptr<pmt::pmt_base> const&)", referenced from:
      gr::basic_block::has_msg_port(boost::intrusive_ptr<pmt::pmt_base>) in fsk4_demod_ff_impl.cc.o
      gr::basic_block::has_msg_port(boost::intrusive_ptr<pmt::pmt_base>) in fsk4_slicer_fb_impl.cc.o
      gr::basic_block::has_msg_port(boost::intrusive_ptr<pmt::pmt_base>) in decoder_bf_impl.cc.o
      gr::basic_block::has_msg_port(boost::intrusive_ptr<pmt::pmt_base>) in decoder_ff_impl.cc.o
      gr::basic_block::has_msg_port(boost::intrusive_ptr<pmt::pmt_base>) in pcap_source_b_impl.cc.o

@SMillerDev
Copy link
Member

@iMichka can you help here? It's python + gnuradio

@iMichka
Copy link
Member

iMichka commented May 27, 2020

Our gnuradio uses system Python 2.

We can not accept new formulae that depend on a deprecated library.

#49733 is ready but has one issue; and I am still waiting from feedback from upstream to get it fixed, as I was not able to fix it myself. As long as #49733 is not merged, we can not accept the op25 formula.

@mikemorris
Copy link
Contributor Author

@iMichka Happy to wait for #49733 and update or reopen this targeting Python 3 and [email protected]

@SMillerDev
Copy link
Member

I'd say reopening is the way to go here, given the uncertainty in the timing of #49733

@SMillerDev SMillerDev closed this Jun 3, 2020
@mikemorris
Copy link
Contributor Author

mikemorris commented Jun 7, 2020 •
edited
Loading

Missing/broken dependencies (for future reference):

  • zmq and waitress Python libs needed for -l http:localhost:PORT web UI
  • copy missing gr-op25_repeater/www directory as peer to libexec for web UI
  • gnuplot> set output "../www/images relative path assumption fails unless running rx.py directly from /usr/local/Cellar/op25/25/libexec
  • GNUPLOT = /usr/bin/gnuplot in gr_gnuplot.py possible to override with Python env wrapper to GNUPLOT = /usr/local/bin/gnuplot? Need to add gnuplot Hombrew dep too
  • gnuplot set terminal x11 broken on macOS, Homebrew-installed gnuplot appears to not support x11 terminal even with brew cask install xquartz - switching this to set terminal qt works but appears to be hardcoded and not easily configurable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mikemorris @SMillerDev @iMichka @alebcay