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

Added required dependencies for WebRTC and SRT elements #25680

Merged
merged 2 commits into from
Apr 10, 2018

Conversation

jon-courtney
Copy link
Contributor

Added dependency on srt for srt elements
Added tests to assert existence of webrtcbin and srtclientsrc elements

  • 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?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Added dependency on srt for srt elements
Added tests to assert existence of webrtcbin and srtclientsrc elements
@@ -23,6 +23,8 @@ class GstPluginsBad < Formula
depends_on "gettext"
depends_on "gst-plugins-base"
depends_on "openssl"
depends_on "libnice"
Copy link
Contributor

Choose a reason for hiding this comment

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

As WebRTC support is relatively new in gst-plugins-bad, mentioned in this comment, and may break often - shouldn't this be made an :optional dependency? Is it a similar situation with srt?

Choose a reason for hiding this comment

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

@rakkesh As per Gstreamer documentation, "GStreamer Bad Plug-ins is a set of plug-ins that aren't up to par compared to the rest. They might be close to being good quality, but they're missing something - be it a good code review, some documentation, a set of tests, a real live maintainer, or some actual wide use". In short, anything in this package is already understood as being less stable than what is in the "good plug-ins" package and more stable than what is in the "ugly plug-ins" package. Therefore I don't think it is necessary to make WebRTC and SRT optional here, they have already been deemed good enough for the "bad plug-ins" package by the Gstreamer developers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. My comment was not much of a review but just an enquiry.
I had missed out on the more pressing comment by @tschoonj, as he usually reviews freedesktop related formulae.

Choose a reason for hiding this comment

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

@rakkesh Yes, I understand that. @jon-courtney already responded to that comment on the same thread. I'm just pointing out here that the "may break often" argument is redundant with the fact that these functionalities are provided in a package that already implies unstable functionalities. If we make WebRTC and SRT optional, then all other plugins in this package should also be made optional, and vice versa.

Copy link
Contributor Author

@jon-courtney jon-courtney Mar 28, 2018

Choose a reason for hiding this comment

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

The only other comment that I would add to what @sjmgarnier has said is that, since gst-plugins-bad is a collection of a large number of independent plugins, the individual user is probably less concerned about the stability of any individual component. Odds are that s/he is relying on a small fraction of the elements provided by the package anyway. What is more troubling (to me at least) is when the GStreamer project advertises the presence of a plugin but it can't be found via Homebrew. So I personally favor the inclusion of all the plugins, all other things being equal.

I suppose an argument in favor of NOT including any individual plugin by default might be if its inclusion pulls in a particularly large dependency. If that is a particular concern with the webrtcbin and srt plugins (although I don't know that it should be), then I suppose they could be marked as :recommended instead.

@fxcoudert
Copy link
Member

We're making them optional for now, and we can reconsider in the future if they are used a lot.

@fxcoudert fxcoudert merged commit 65ea53b into Homebrew:master Apr 10, 2018
@lock lock bot added the outdated PR was locked due to age label May 10, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
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.

4 participants