Build libvpx on non-Mac POSIX platforms

Build libvpx on non-Mac POSIX platforms also, by reusing the linux config; this patch was tested on FreeBSD, Solaris, and NetBSD.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113567

[ruben, 2011-11-23 04:21:25]

Please review.  I had to use the nested variables in order to get it to work,
let me know if there's a simpler way.

[Ami GONE FROM CHROMIUM, 2011-11-23 09:14:11]

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp
File libvpx.gyp (right):

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp#newcode28
libvpx.gyp:28: '_OS%': 'linux',
I'm shocked that reusing the linux config happens to work.
I'm not aware of any invariants that imply that posix&&!mac implies one of
linux,freebsd,openbsd,solaris (and I fear that others can slide in here) so your
testing is insufficient for this technique.

What are you trying to accomplish?  If it's just to enable the 3 systems you
checked, why not test for exactly those?

FWIW since there are no bots for those OSs odds are pretty good that in the
future these setups will silently break.  I wonder why you want this in the
first place :)  (AFAIK only remoting/ using libvpx; have you got that working on
one of {free,open}bsd/solaris?

[ruben, 2011-11-23 10:07:41]

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp
File libvpx.gyp (right):

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp#newcode28
libvpx.gyp:28: '_OS%': 'linux',
On 2011/11/23 09:14:11, Ami Fischman wrote:
> I'm shocked that reusing the linux config happens to work.
> I'm not aware of any invariants that imply that posix&&!mac implies one of
> linux,freebsd,openbsd,solaris (and I fear that others can slide in here) so
your
> testing is insufficient for this technique.
The linux and mac ia32 configs only differ by 6 lines, so I don't think it's
that surprising that other POSIX platforms also work. :) Obviously the os_posix
would be open-ended for other POSIX platforms, but that's sort of the point, to
leave it open for future ports.  The gyp file for ffmpeg has a similar setup and
I've been making the same switch all over Chromium, to make future POSIX ports
easier.  Since none of these POSIX platforms are officially supported, I don't
think it much matters if it breaks.

> FWIW since there are no bots for those OSs odds are pretty good that in the
> future these setups will silently break.  I wonder why you want this in the
> first place :)  (AFAIK only remoting/ using libvpx; have you got that working
on
> one of {free,open}bsd/solaris?
Yep, it may break.  I assumed that libvpx was still being used by ffmpeg/, but
now that you mention it, I see that the patched-ffmpeg recently switched to
using its own vp8 decoder.  remoting/ compiles but I have never tried it, while
webrtc hasn't been ported to these other POSIX platforms.  You're probably right
that libvpx is useless for these other platforms, but I suppose this patch can
be included just for completeness?  If you think not, I'll remove it.

[Ami GONE FROM CHROMIUM, 2011-11-23 17:03:39]

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp
File libvpx.gyp (right):

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp#newcode28
libvpx.gyp:28: '_OS%': 'linux',
On 2011/11/23 10:07:41, ruben wrote:
> The linux and mac ia32 configs only differ by 6 lines, so I don't think it's
> that surprising that other POSIX platforms also work.

It would be interesting to know whether the other platforms generate different
configs from the linux one (i.e. whether using the linux configs on them is
likely to cause subtle bugs or perf degradation).

> You're probably right
> that libvpx is useless for these other platforms, but I suppose this patch can
> be included just for completeness?  If you think not, I'll remove it.

ISTM that you would be doing those platforms a disservice, having them build
stuff they're not using.

[ruben, 2011-11-23 18:55:53]

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp
File libvpx.gyp (right):

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp#newcode28
libvpx.gyp:28: '_OS%': 'linux',
On 2011/11/23 17:03:39, Ami Fischman wrote:
> It would be interesting to know whether the other platforms generate different
> configs from the linux one (i.e. whether using the linux configs on them is
> likely to cause subtle bugs or perf degradation).
The library is only compiled at this point, so no way to know.  I think it was
compiled AND used by ffmpeg at one point, if so, it worked fine on these other
platforms then.

> ISTM that you would be doing those platforms a disservice, having them build
> stuff they're not using.
Not really as libvpx is a quick compile.  The alternative is to write another
patch to really disable remoting- I say "really" since unsetting the remoting
flag still causes an unmodified libvpx.gyp to be called from remoting.gyp and
then gyp fails- so it's really a question of whether remoting should be disabled
by default on other platforms.  I've been compiling it because it does build
with this patch, but if you think it's better that remoting is disabled on other
platforms, I'll submit a patch for that instead.

My preference is to compile as much Chromium code by default as possible, hence
this patch, but I don't even know what remoting is for so I'm fine with just
removing it also.

[Ami GONE FROM CHROMIUM, 2011-11-23 19:09:43]

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp
File libvpx.gyp (right):

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp#newcode28
libvpx.gyp:28: '_OS%': 'linux',
On 2011/11/23 18:55:53, ruben wrote:
> On 2011/11/23 17:03:39, Ami Fischman wrote:
> > It would be interesting to know whether the other platforms generate
different
> > configs from the linux one (i.e. whether using the linux configs on them is
> > likely to cause subtle bugs or perf degradation).
> The library is only compiled at this point, so no way to know.  I think it was
> compiled AND used by ffmpeg at one point, if so, it worked fine on these other
> platforms then.

FTR chromium switched from libvpx to ffvp8 for video decode in CL 97421.

> > ISTM that you would be doing those platforms a disservice, having them build
> > stuff they're not using.
> Not really as libvpx is a quick compile.  The alternative is to write another
> patch to really disable remoting- I say "really" since unsetting the remoting
> flag still causes an unmodified libvpx.gyp to be called from remoting.gyp and
> then gyp fails

Ah; sounds like you ran into a non-ARM version of
http://code.google.com/p/chromium/issues/detail?id=94418

(remoting is part of the implementation of
https://chrome.google.com/webstore/detail/gbchcmhmhahfdphkhkmpfmihenigjmpp)

I think you've convinced me, with the proviso that there should be an
explanatory comment (both b/c the technique is tricksy and because it might lead
people to believe there's a bigger support commitment than there really is).
WDYT of:
# HACK: freebsd/openbsd/solaris seem to build fine using linux's configs
# so we use them here.  Use of these headers on anything other than linux
# is likely to break (unsupported platform and no bots).  YMMV.

[ruben, 2011-11-23 19:33:12]

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp
File libvpx.gyp (right):

http://codereview.chromium.org/8666017/diff/1/libvpx.gyp#newcode28
libvpx.gyp:28: '_OS%': 'linux',
On 2011/11/23 19:09:43, Ami Fischman wrote:
> FTR chromium switched from libvpx to ffvp8 for video decode in CL 97421.
Yeah, after you mentioned it earlier, I checked that myself.  However, when I
submitted this patch, I was under the incorrect impression ffmpeg was still
using libvpx.
 
> Ah; sounds like you ran into a non-ARM version of
> http://code.google.com/p/chromium/issues/detail?id=94418
Yeah, looks like it.

> (remoting is part of the implementation of
> https://chrome.google.com/webstore/detail/gbchcmhmhahfdphkhkmpfmihenigjmpp)
OK, I missed the news of that release and a quick google search didn't turn it
up.  Maybe I'll try that extension later and see if it works on these
unsupported platforms. :)

> I think you've convinced me, with the proviso that there should be an
> explanatory comment (both b/c the technique is tricksy and because it might
lead
> people to believe there's a bigger support commitment than there really is).
> WDYT of:
> # HACK: freebsd/openbsd/solaris seem to build fine using linux's configs
> # so we use them here.  Use of these headers on anything other than linux
> # is likely to break (unsupported platform and no bots).  YMMV.
Will do.

[ruben, 2011-11-23 19:40:40]

On 2011/11/23 19:33:12, ruben wrote:
> > I think you've convinced me, with the proviso that there should be an
> > explanatory comment (both b/c the technique is tricksy and because it might
> lead
> > people to believe there's a bigger support commitment than there really is).
> > WDYT of:
> > # HACK: freebsd/openbsd/solaris seem to build fine using linux's configs
> > # so we use them here.  Use of these headers on anything other than linux
> > # is likely to break (unsupported platform and no bots).  YMMV.
> Will do.
Added the explanatory comment you asked for.

[Ami GONE FROM CHROMIUM, 2011-11-23 20:07:42]

LGTM with the nit below, but I'd like to get the OK from a gyp expert before
submitting.

Albert: does this way of accomplishing the goal seem reasonable or is there a
more gyp-idiomatic way to go about it?

http://codereview.chromium.org/8666017/diff/4002/libvpx.gyp
File libvpx.gyp (right):

http://codereview.chromium.org/8666017/diff/4002/libvpx.gyp#newcode27
libvpx.gyp:27: # Reuse linux config for unsupported platforms like BSD and
This (and the next line) should be indented 2 extra spaces (like the first
non-comment line below, not like the previous line).

[ruben, 2011-11-23 20:24:39]

On 2011/11/23 20:07:42, Ami Fischman wrote:
> http://codereview.chromium.org/8666017/diff/4002/libvpx.gyp#newcode27
> libvpx.gyp:27: # Reuse linux config for unsupported platforms like BSD and
> This (and the next line) should be indented 2 extra spaces (like the first
> non-comment line below, not like the previous line).
Ah, my mistake, fixed.

[awong, 2011-11-28 18:46:19]

Oh man...this is beyond my gyp-fu.  Adding bradnelson..

On 2011/11/23 20:24:39, ruben wrote:
> On 2011/11/23 20:07:42, Ami Fischman wrote:
> > http://codereview.chromium.org/8666017/diff/4002/libvpx.gyp#newcode27
> > libvpx.gyp:27: # Reuse linux config for unsupported platforms like BSD and
> > This (and the next line) should be indented 2 extra spaces (like the first
> > non-comment line below, not like the previous line).
> Ah, my mistake, fixed.

[awong, 2011-11-28 18:47:07]

Really adding Brad..

Brad...this is moderately complex gyp variable expansion logic.  Do you know if
there's a best practice here?

Thanks,
Albert

On 2011/11/28 18:46:19, awong wrote:
> Oh man...this is beyond my gyp-fu.  Adding bradnelson..
> 
> On 2011/11/23 20:24:39, ruben wrote:
> > On 2011/11/23 20:07:42, Ami Fischman wrote:
> > > http://codereview.chromium.org/8666017/diff/4002/libvpx.gyp#newcode27
> > > libvpx.gyp:27: # Reuse linux config for unsupported platforms like BSD and
> > > This (and the next line) should be indented 2 extra spaces (like the first
> > > non-comment line below, not like the previous line).
> > Ah, my mistake, fixed.

[ruben, 2011-11-28 22:50:34]

On 2011/11/28 18:47:07, awong wrote:
> Really adding Brad..
> 
> Brad...this is moderately complex gyp variable expansion logic.  Do you know
if
> there's a best practice here?
I'll note that all I did initially is copy similar logic from later in the gyp
file, where the _OS variable is used with android.  I had to modify that by
adding another "variables:" sub-scope and then copy the _OS variable out because
if I didn't, I couldn't use _OS for the yasm_flags variable, only for the
include_dirs.  gyp scoping rules seem pretty weird to me, so I just tried
different variations till I found this one that worked.

[ruben, 2011-12-01 21:18:29]

On 2011/11/28 22:50:34, ruben wrote:
> On 2011/11/28 18:47:07, awong wrote:
> > Really adding Brad..
> > 
> > Brad...this is moderately complex gyp variable expansion logic.  Do you know
> if
> > there's a best practice here?
> I'll note that all I did initially is copy similar logic from later in the gyp
> file, where the _OS variable is used with android.  I had to modify that by
> adding another "variables:" sub-scope and then copy the _OS variable out
because
> if I didn't, I couldn't use _OS for the yasm_flags variable, only for the
> include_dirs.  gyp scoping rules seem pretty weird to me, so I just tried
> different variations till I found this one that worked.
Perhaps the wrong email alias was used for Brad?  Redirecting review to his more
commonly used email.

[bradn, 2011-12-02 01:18:00]

lgtm

http://codereview.chromium.org/8666017/diff/6002/libvpx.gyp
File libvpx.gyp (right):

http://codereview.chromium.org/8666017/diff/6002/libvpx.gyp#newcode28
libvpx.gyp:28: # Solaris.  This compiles but no guarantee it'll run.
Other than maybe selecting a more descriptive name than _OS, lg.
Maybe, GENERAL_OS_TYPE or something.
I don't feel strongly though.

[ruben, 2011-12-02 20:30:52]

On 2011/12/02 01:18:00, bradn wrote:
> lgtm
I tried checking the commit bit on rietveld but nothing happened, can one of the
reviewers commit manually?  Thanks.

[ruben, 2011-12-06 21:29:58]

On 2011/12/02 20:30:52, ruben wrote:
> On 2011/12/02 01:18:00, bradn wrote:
> > lgtm
> I tried checking the commit bit on rietveld but nothing happened, can one of
the
> reviewers commit manually?  Thanks.
Can someone with commit access apply this manually?  I can apply patches through
rietveld for Chromium src/ but not for deps apparently.  I'd like for this to
get in before Chromium 17.0 is branched.

[scherkus (not reviewing), 2011-12-08 06:05:57]

Committed as r113567.