Add Opus support to Mplayer build. #15186

Closed
wants to merge 1 commit into from

4 participants

@ohookins

Opus codec will be automatically detected, but this change adds an explicit dependency on the library.
It is also only supported in the HEAD of the mplayer source.
The patch to the configure script has also changed format, so move this out into a gist for retrieval.

@ohookins

@adamv understood, and I've fixed those issues.

@gvarisco

Adam, Mike - changes work for me. Both your suggestions have been implemented with the last commit. Can we merge it? Thanks, guys!

@adamv

Patch doesn't apply cleanly; can we get this squashed and rebased?

@adamv

Looks OK to me; any other maintainer want to review this?

@mistydemeo mistydemeo commented on the diff Dec 2, 2012
Library/Formula/mplayer.rb
depends_on :x11 if build.include? 'with-x'
+ depends_on 'opus' if build.include? 'with-opus'

Let's add an extra if build.head? guard here. No sense dragging in an opus dependency that won't be used if the user mistakenly uses the option on a stable build.

@ohookins
ohookins added a note Dec 2, 2012

Is this change acceptable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adamv

This needs to be squashed to a single commit and rebased on master.

@ohookins ohookins Add Opus support to Mplayer build.
Opus codec will be automatically detected, but this change adds an explicit dependency on the library.
It is also only supported in the HEAD of the mplayer source.
The patch to the configure script has also changed format, so move this out into a gist for retrieval.
31ead69
@ohookins

@adamv done.

@adamv

Thanks; I'll try to test when I get a chance.

@adamv adamv was assigned Jan 15, 2013
@adamv

@jacknagel @mistydemeo @mikemcquaid I'm not sure how I feel about adding an option that requires a HEAD build.

MacPorts has a devel version, which supports opus: https://trac.macports.org/browser/trunk/dports/multimedia/mplayer-devel/Portfile

I suppose the core issue is that mplayer updates much more often than it tags stable releases.

Thoughts? I can just pull this to clear out the issue, of course.

@MikeMcQuaid
Homebrew member

Yeh, don't add an option that requires HEAD I think.

@adamv

@ohookins one option here would be to make a new formula MplayerHead or MplayerDevel, and submit it to https://github.com/Homebrew/homebrew-headonly

@adamv

Ok, I'm going to close this.

@ohookins , there's nothing wrong this this patch, it's just that we don't want to add an option for head-only mode, as that can lead to complexity and bug reports down the road.

I would suggest, as above, making a new, separate formula that specifically only installs the head version of this, add the opus (and any other) options, and open a new pull request at https://github.com/Homebrew/homebrew-headonly

Sorry for the delay in dealing with this.

@adamv adamv closed this Feb 9, 2013
@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.