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

gst-plugins-bad - Missing dependencies for WebRTCBin #41713

Closed
wants to merge 1 commit into from

Conversation

vinicius-tona
Copy link
Contributor

@vinicius-tona vinicius-tona commented Jul 8, 2019

LibNice and SRTP are required to build WebRTCBin

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

@vinicius-tona vinicius-tona changed the title Missing dependencies for WebRTCBin gst-plugins-bad - Missing dependencies for WebRTCBin Jul 8, 2019
@fxcoudert
Copy link
Member

libnice is a heavy dependency, because it brings gnutls. Not sure how many users this will benefit.

@vinicius-tona
Copy link
Contributor Author

vinicius-tona commented Jul 10, 2019

@fxcoudert How about adding it as optional?
But we need to make sure it will be kept on further changes because if we check the history we can see this dependency always being added and removed. Probably because brew audit --strict says that Formulae should not have optional or recommended dependencies.

Also, I don't see an issue on adding this dependency considering that webrtcbin is part of gst-plugins-bad. I find it inconsistent and always require debugging (even to realize that you need to add a -with-libnice). But that's only my opinion.

@SMillerDev
Copy link
Member

Options have been removed from homebrew core last year because they simply break way too often.

@vinicius-tona
Copy link
Contributor Author

Ok sorry, I didn't know that.

Do we add libnice since webrtcbin is part of gst-plugins-bad? Do we keep it inconsistent? Is there another approach I'm not aware of?

Regards

@nirbheek
Copy link

Speaking as a gstreamer developer, webrtc support is important and is used by a large number of people. I would highly recommend enabling it by default. All Linux distros ship it, for instance.

@SMillerDev
Copy link
Member

If it benefits a lot of users let's just include it as a default

@vinicius-tona
Copy link
Contributor Author

@SMillerDev
My PR is including it as default, is there anything else I need to do before merging it?

@jonchang
Copy link
Contributor

Please bump the revision so users will get the new build on brew upgrade.

@fxcoudert
Copy link
Member

Please bump the revision so users will get the new build on brew upgrade

And squash everything into a single commit, with message gst-plugins-bad: add WebRTCBin

@vinicius-tona
Copy link
Contributor Author

@fxcoudert Done, thanks for letting me know

@fxcoudert
Copy link
Member

Thanks @vinicius-tona you rock!

@fxcoudert fxcoudert closed this in f5f6441 Jul 14, 2019
@lock lock bot added the outdated PR was locked due to age label Jan 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
almost there PR is nearly ready to merge outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants