FFmpeg in-tree build for mac.

FFmpeg in-tree build for mac.

BUG=26509
TEST=none

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

[awong, 2009-11-03 02:22:33]

Close...but no cigar.

Issue with PIC again.  Any ideas?

If we could make xcode build with gcc 4.0, things would be happier...

Also, there's a problem where -lffmpegasm.a (the hand-rolled static library we
made) doesn't show up in the link line.

[awong, 2009-11-03 13:25:47]

Found the xcode_settings block to be a gold-mine. :)

Anyways, new patch.  gcc now set to 4.0.  It will eventually link...if I can get
the static library into the link files.

I tried using the path to the library similar to how you did it in WebCore.gyp,
but that iddn't seem to work.

On 2009/11/03 02:22:33, awong wrote:
> Close...but no cigar.
> 
> Issue with PIC again.  Any ideas?
> 
> If we could make xcode build with gcc 4.0, things would be happier...
> 
> Also, there's a problem where -lffmpegasm.a (the hand-rolled static library we
> made) doesn't show up in the link line.

[awong, 2009-11-03 13:44:31]

One more update...I got the path wrong initially, but after fixing it, I
only get a "-lffmpegasm" in the link command.  I need a "-L......" with the
path as well, otherwise it doesn't work. :(

-Albert

On Tue, Nov 3, 2009 at 5:25 AM, <ajwong@chromium.org> wrote:

> Found the xcode_settings block to be a gold-mine. :)
>
> Anyways, new patch.  gcc now set to 4.0.  It will eventually link...if I
> can get
> the static library into the link files.
>
> I tried using the path to the library similar to how you did it in
> WebCore.gyp,
> but that iddn't seem to work.
>
>
> On 2009/11/03 02:22:33, awong wrote:
>
>> Close...but no cigar.
>>
>
>  Issue with PIC again.  Any ideas?
>>
>
>  If we could make xcode build with gcc 4.0, things would be happier...
>>
>
>  Also, there's a problem where -lffmpegasm.a (the hand-rolled static
>> library we
>> made) doesn't show up in the link line.
>>
>
>
>
> http://codereview.chromium.org/340072
>

[Mark Mentovai, 2009-11-03 13:51:59]

http://codereview.chromium.org/340072/diff/4001/3005
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/340072/diff/4001/3005#newcode188
Line 188: 'ldflags': [
The Xcode generator doesn't use this.

http://codereview.chromium.org/340072/diff/4001/3005#newcode190
Line 190: '-L<(shared_generated_dir)',
Notwithstanding the above:

I don't think you need -L on the Mac, the Xcode generator and Xcode will work
together to figure out any -L that you might need.

http://codereview.chromium.org/340072/diff/4001/3005#newcode191
Line 191: '-Wl,-read_only_relocs,suppress',
Blech.  I thought you told me that there wouldn't be any silly link flags.  But
the Xcode generator isn't using this, so...?

I guess we can let this slide here because it's only for this module, but as
you're aware, it's not the cleanest way to go.

I think you should put this into a Mac conditional, and -Bsymbolic into a Linux
conditional.  You shouldn't pass flags to linkers that they don't understand.

http://codereview.chromium.org/340072/diff/4001/3005#newcode194
Line 194:
'<(shared_generated_dir)/<(STATIC_LIB_PREFIX)<(asm_library)<(STATIC_LIB_SUFFIX)',
This is the part that's not working for you?

http://codereview.chromium.org/340072/diff/4001/3005#newcode308
Line 308: 'yasm_flags': [ '-DARCH_X86_32', ],
Crap, I'll need to rewrap this for the Mac 32/64 build, since we'll want one
pass to do both 32 and 64 (depending on what's selected in Xcode).  No biggie,
I'll cross that bridge when I get there in a couple of weeks.

http://codereview.chromium.org/340072/diff/4001/3005#newcode309
Line 309: },
No -m x86 like on Linux?

http://codereview.chromium.org/340072/diff/4001/3005#newcode351
Line 351: 'library_path':
'<(shared_generated_dir)/lib<(asm_library)<(STATIC_LIB_SUFFIX)',
lib vs. <(STATIC_LIB_PREFIX)?

[Mark Mentovai, 2009-11-03 13:53:43]

http://codereview.chromium.org/340072/diff/4001/3005
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/340072/diff/4001/3005#newcode172
Line 172: 'xcode_settings': {
Try adding to this block:

'LIBRARY_SEARCH_PATHS': [
  '<(shared_generated_dir)'
],

See xcode.py:1068.  The generator will only tell Xcode to add -L automatically
when given a library by relative path.  <(shared_generated_dir) is an absolute
path.

[awong, 2009-11-03 20:28:33]

Turns out, the gcc-4.2 versus gcc-4.0 seems to be basically the whether
-mdynamic_no_pic is on by default.

I think when it's invoked via xcode in our project, -mdynamic_no_pic is disabled
for both.  However, when invoked via the command line as just "gcc", gcc-4.0, 
has it off by default.

Anyways, the last hurdle is to get the -Wl,-read_only_relocs, suppress into the
link line.

http://codereview.chromium.org/340072/diff/4001/3005
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/340072/diff/4001/3005#newcode172
Line 172: 'xcode_settings': {
On 2009/11/03 13:53:43, Mark Mentovai wrote:
> Try adding to this block:
> 
> 'LIBRARY_SEARCH_PATHS': [
>   '<(shared_generated_dir)'
> ],
> 
> See xcode.py:1068.  The generator will only tell Xcode to add -L automatically
> when given a library by relative path.  <(shared_generated_dir) is an absolute
> path.

Ah...missed the relative path part.

Fixed.

http://codereview.chromium.org/340072/diff/4001/3005#newcode188
Line 188: 'ldflags': [
On 2009/11/03 13:52:00, Mark Mentovai wrote:
> The Xcode generator doesn't use this.

Moved platform specific stuff into conditionals.

http://codereview.chromium.org/340072/diff/4001/3005#newcode190
Line 190: '-L<(shared_generated_dir)',
On 2009/11/03 13:52:00, Mark Mentovai wrote:
> Notwithstanding the above:
> 
> I don't think you need -L on the Mac, the Xcode generator and Xcode will work
> together to figure out any -L that you might need.

Yep...you're right.  This only sits in the linux/freebsd conditional now.

http://codereview.chromium.org/340072/diff/4001/3005#newcode191
Line 191: '-Wl,-read_only_relocs,suppress',
On 2009/11/03 13:52:00, Mark Mentovai wrote:
> Blech.  I thought you told me that there wouldn't be any silly link flags. 
But
> the Xcode generator isn't using this, so...?
> 
> I guess we can let this slide here because it's only for this module, but as
> you're aware, it's not the cleanest way to go.
> 
> I think you should put this into a Mac conditional, and -Bsymbolic into a
Linux
> conditional.  You shouldn't pass flags to linkers that they don't understand.

So....xcode never managed to link my stuff before because of the -L issue in a
previous comment.

As for magic...yeah, I know.  I do not have a solution cause it basically comes
down to "ffmpeg can't do PIC on x86".  :(

So, now I just need to figure out how to pass it to the linker.

http://codereview.chromium.org/340072/diff/4001/3005#newcode194
Line 194:
'<(shared_generated_dir)/<(STATIC_LIB_PREFIX)<(asm_library)<(STATIC_LIB_SUFFIX)',
On 2009/11/03 13:52:00, Mark Mentovai wrote:
> This is the part that's not working for you?

Yep.  The path thing you suggested earlier fixed things.

http://codereview.chromium.org/340072/diff/4001/3005#newcode308
Line 308: 'yasm_flags': [ '-DARCH_X86_32', ],
On 2009/11/03 13:52:00, Mark Mentovai wrote:
> Crap, I'll need to rewrap this for the Mac 32/64 build, since we'll want one
> pass to do both 32 and 64 (depending on what's selected in Xcode).  No biggie,
> I'll cross that bridge when I get there in a couple of weeks.

I took a hypothetical stab at it, but don't have it tested.

http://codereview.chromium.org/340072/diff/4001/3005#newcode309
Line 309: },
On 2009/11/03 13:52:00, Mark Mentovai wrote:
> No -m x86 like on Linux?

Added it in.  Wasn't needed because yasm is x86 by default in the current build.
 But you're right, it's better to be explicit.

http://codereview.chromium.org/340072/diff/4001/3005#newcode351
Line 351: 'library_path':
'<(shared_generated_dir)/lib<(asm_library)<(STATIC_LIB_SUFFIX)',
On 2009/11/03 13:52:00, Mark Mentovai wrote:
> lib vs. <(STATIC_LIB_PREFIX)?

fixed. Nice catch.

[Mark Mentovai, 2009-11-03 20:35:23]

Does that mean that we can leave the compiler at 4.2 and just have it
use -mdynamic-no-pic?

Mark

On Tue, Nov 3, 2009 at 3:28 PM,  <ajwong@chromium.org> wrote:
> Turns out, the gcc-4.2 versus gcc-4.0 seems to be basically the whether
> -mdynamic_no_pic is on by default.
>
> I think when it's invoked via xcode in our project, -mdynamic_no_pic is
> disabled
> for both. =A0However, when invoked via the command line as just "gcc",
> gcc-4.0,
> has it off by default.
>
> Anyways, the last hurdle is to get the -Wl,-read_only_relocs, suppress in=
to
> the
> link line.
>
>
> http://codereview.chromium.org/340072/diff/4001/3005
> File third_party/ffmpeg/ffmpeg.gyp (right):
>
> http://codereview.chromium.org/340072/diff/4001/3005#newcode172
> Line 172: 'xcode_settings': {
> On 2009/11/03 13:53:43, Mark Mentovai wrote:
>>
>> Try adding to this block:
>
>> 'LIBRARY_SEARCH_PATHS': [
>> =A0 '<(shared_generated_dir)'
>> ],
>
>> See xcode.py:1068. =A0The generator will only tell Xcode to add -L
>
> automatically
>>
>> when given a library by relative path. =A0<(shared_generated_dir) is an
>
> absolute
>>
>> path.
>
> Ah...missed the relative path part.
>
> Fixed.
>
> http://codereview.chromium.org/340072/diff/4001/3005#newcode188
> Line 188: 'ldflags': [
> On 2009/11/03 13:52:00, Mark Mentovai wrote:
>>
>> The Xcode generator doesn't use this.
>
> Moved platform specific stuff into conditionals.
>
> http://codereview.chromium.org/340072/diff/4001/3005#newcode190
> Line 190: '-L<(shared_generated_dir)',
> On 2009/11/03 13:52:00, Mark Mentovai wrote:
>>
>> Notwithstanding the above:
>
>> I don't think you need -L on the Mac, the Xcode generator and Xcode
>
> will work
>>
>> together to figure out any -L that you might need.
>
> Yep...you're right. =A0This only sits in the linux/freebsd conditional
> now.
>
> http://codereview.chromium.org/340072/diff/4001/3005#newcode191
> Line 191: '-Wl,-read_only_relocs,suppress',
> On 2009/11/03 13:52:00, Mark Mentovai wrote:
>>
>> Blech. =A0I thought you told me that there wouldn't be any silly link
>
> flags. =A0But
>>
>> the Xcode generator isn't using this, so...?
>
>> I guess we can let this slide here because it's only for this module,
>
> but as
>>
>> you're aware, it's not the cleanest way to go.
>
>> I think you should put this into a Mac conditional, and -Bsymbolic
>
> into a Linux
>>
>> conditional. =A0You shouldn't pass flags to linkers that they don't
>
> understand.
>
> So....xcode never managed to link my stuff before because of the -L
> issue in a previous comment.
>
> As for magic...yeah, I know. =A0I do not have a solution cause it
> basically comes down to "ffmpeg can't do PIC on x86". =A0:(
>
> So, now I just need to figure out how to pass it to the linker.
>
> http://codereview.chromium.org/340072/diff/4001/3005#newcode194
> Line 194:
> '<(shared_generated_dir)/<(STATIC_LIB_PREFIX)<(asm_library)<(STATIC_LIB_S=
UFFIX)',
> On 2009/11/03 13:52:00, Mark Mentovai wrote:
>>
>> This is the part that's not working for you?
>
> Yep. =A0The path thing you suggested earlier fixed things.
>
> http://codereview.chromium.org/340072/diff/4001/3005#newcode308
> Line 308: 'yasm_flags': [ '-DARCH_X86_32', ],
> On 2009/11/03 13:52:00, Mark Mentovai wrote:
>>
>> Crap, I'll need to rewrap this for the Mac 32/64 build, since we'll
>
> want one
>>
>> pass to do both 32 and 64 (depending on what's selected in Xcode). =A0No
>
> biggie,
>>
>> I'll cross that bridge when I get there in a couple of weeks.
>
> I took a hypothetical stab at it, but don't have it tested.
>
> http://codereview.chromium.org/340072/diff/4001/3005#newcode309
> Line 309: },
> On 2009/11/03 13:52:00, Mark Mentovai wrote:
>>
>> No -m x86 like on Linux?
>
> Added it in. =A0Wasn't needed because yasm is x86 by default in the
> current build. =A0But you're right, it's better to be explicit.
>
> http://codereview.chromium.org/340072/diff/4001/3005#newcode351
> Line 351: 'library_path':
> '<(shared_generated_dir)/lib<(asm_library)<(STATIC_LIB_SUFFIX)',
> On 2009/11/03 13:52:00, Mark Mentovai wrote:
>>
>> lib vs. <(STATIC_LIB_PREFIX)?
>
> fixed. Nice catch.
>
> http://codereview.chromium.org/340072
>

[awong, 2009-11-03 20:38:52]

On Tue, Nov 3, 2009 at 12:35 PM, Mark Mentovai <mark@chromium.org> wrote:

> Does that mean that we can leave the compiler at 4.2 and just have it
> use -mdynamic-no-pic?
>

Yep.  I've removed the compiler line from the latest patch.


>
> Mark
>
> On Tue, Nov 3, 2009 at 3:28 PM,  <ajwong@chromium.org> wrote:
> > Turns out, the gcc-4.2 versus gcc-4.0 seems to be basically the whether
> > -mdynamic_no_pic is on by default.
> >
> > I think when it's invoked via xcode in our project, -mdynamic_no_pic is
> > disabled
> > for both.  However, when invoked via the command line as just "gcc",
> > gcc-4.0,
> > has it off by default.
> >
> > Anyways, the last hurdle is to get the -Wl,-read_only_relocs, suppress
> into
> > the
> > link line.
> >
> >
> > http://codereview.chromium.org/340072/diff/4001/3005
> > File third_party/ffmpeg/ffmpeg.gyp (right):
> >
> > http://codereview.chromium.org/340072/diff/4001/3005#newcode172
> > Line 172: 'xcode_settings': {
> > On 2009/11/03 13:53:43, Mark Mentovai wrote:
> >>
> >> Try adding to this block:
> >
> >> 'LIBRARY_SEARCH_PATHS': [
> >>   '<(shared_generated_dir)'
> >> ],
> >
> >> See xcode.py:1068.  The generator will only tell Xcode to add -L
> >
> > automatically
> >>
> >> when given a library by relative path.  <(shared_generated_dir) is an
> >
> > absolute
> >>
> >> path.
> >
> > Ah...missed the relative path part.
> >
> > Fixed.
> >
> > http://codereview.chromium.org/340072/diff/4001/3005#newcode188
> > Line 188: 'ldflags': [
> > On 2009/11/03 13:52:00, Mark Mentovai wrote:
> >>
> >> The Xcode generator doesn't use this.
> >
> > Moved platform specific stuff into conditionals.
> >
> > http://codereview.chromium.org/340072/diff/4001/3005#newcode190
> > Line 190: '-L<(shared_generated_dir)',
> > On 2009/11/03 13:52:00, Mark Mentovai wrote:
> >>
> >> Notwithstanding the above:
> >
> >> I don't think you need -L on the Mac, the Xcode generator and Xcode
> >
> > will work
> >>
> >> together to figure out any -L that you might need.
> >
> > Yep...you're right.  This only sits in the linux/freebsd conditional
> > now.
> >
> > http://codereview.chromium.org/340072/diff/4001/3005#newcode191
> > Line 191: '-Wl,-read_only_relocs,suppress',
> > On 2009/11/03 13:52:00, Mark Mentovai wrote:
> >>
> >> Blech.  I thought you told me that there wouldn't be any silly link
> >
> > flags.  But
> >>
> >> the Xcode generator isn't using this, so...?
> >
> >> I guess we can let this slide here because it's only for this module,
> >
> > but as
> >>
> >> you're aware, it's not the cleanest way to go.
> >
> >> I think you should put this into a Mac conditional, and -Bsymbolic
> >
> > into a Linux
> >>
> >> conditional.  You shouldn't pass flags to linkers that they don't
> >
> > understand.
> >
> > So....xcode never managed to link my stuff before because of the -L
> > issue in a previous comment.
> >
> > As for magic...yeah, I know.  I do not have a solution cause it
> > basically comes down to "ffmpeg can't do PIC on x86".  :(
> >
> > So, now I just need to figure out how to pass it to the linker.
> >
> > http://codereview.chromium.org/340072/diff/4001/3005#newcode194
> > Line 194:
> >
>
'<(shared_generated_dir)/<(STATIC_LIB_PREFIX)<(asm_library)<(STATIC_LIB_SUFFIX)',
> > On 2009/11/03 13:52:00, Mark Mentovai wrote:
> >>
> >> This is the part that's not working for you?
> >
> > Yep.  The path thing you suggested earlier fixed things.
> >
> > http://codereview.chromium.org/340072/diff/4001/3005#newcode308
> > Line 308: 'yasm_flags': [ '-DARCH_X86_32', ],
> > On 2009/11/03 13:52:00, Mark Mentovai wrote:
> >>
> >> Crap, I'll need to rewrap this for the Mac 32/64 build, since we'll
> >
> > want one
> >>
> >> pass to do both 32 and 64 (depending on what's selected in Xcode).  No
> >
> > biggie,
> >>
> >> I'll cross that bridge when I get there in a couple of weeks.
> >
> > I took a hypothetical stab at it, but don't have it tested.
> >
> > http://codereview.chromium.org/340072/diff/4001/3005#newcode309
> > Line 309: },
> > On 2009/11/03 13:52:00, Mark Mentovai wrote:
> >>
> >> No -m x86 like on Linux?
> >
> > Added it in.  Wasn't needed because yasm is x86 by default in the
> > current build.  But you're right, it's better to be explicit.
> >
> > http://codereview.chromium.org/340072/diff/4001/3005#newcode351
> > Line 351: 'library_path':
> > '<(shared_generated_dir)/lib<(asm_library)<(STATIC_LIB_SUFFIX)',
> > On 2009/11/03 13:52:00, Mark Mentovai wrote:
> >>
> >> lib vs. <(STATIC_LIB_PREFIX)?
> >
> > fixed. Nice catch.
> >
> > http://codereview.chromium.org/340072
> >
>

[awong, 2009-11-03 23:17:19]

How's this one look?

Current outstanding issue, for a Chrome branded build, it's not linking in libz.

I think the issue is that all the "-l" lines in the "libraries" section are
ignored in xcode.

[awong, 2009-11-03 23:17:23]

How's this one look?

Current outstanding issue, for a Chrome branded build, it's not linking in libz.

I think the issue is that all the "-l" lines in the "libraries" section are
ignored in xcode.

[awong, 2009-11-04 02:23:32]

Works in max and linux, chrome and chromium. release and debug!

[Mark Mentovai, 2009-11-04 02:45:05]

Overall, this was way easier to read than the Linux changes, mostly because it's
so much smaller.  And my eyes aren't anywhere near being in pain.  Nice work.

http://codereview.chromium.org/340072/diff/8002/1028
File build/all.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1028#newcode55
Line 55: '../third_party/yasm/yasm.gyp:*',
I'd put this in a conditional block gated on:

'OS=="mac" or OS=="linux"'

http://codereview.chromium.org/340072/diff/8002/1029
File chrome/chrome.gyp (left):

http://codereview.chromium.org/340072/diff/8002/1029#oldcode5879
Line 5879: # ffmpeg.gyp so they don't diverge. (BUG=23602)
Preserve this TODO?

http://codereview.chromium.org/340072/diff/8002/1029
File chrome/chrome.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1029#newcode5832
Line 5832: 'destination': '<(PRODUCT_DIR)/$(CONTENTS_FOLDER_PATH)/Libraries',
80, so I'd do a 4-space hanging indent on the next line.

http://codereview.chromium.org/340072/diff/8002/1030
File third_party/ffmpeg/README.chromium (right):

http://codereview.chromium.org/340072/diff/8002/1030#newcode133
Line 133: For mac, execute replace the ".so" in the files above with ".dylib".
Mac is a proper name, it gets a capital M.

http://codereview.chromium.org/340072/diff/8002/1031
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1031#newcode53
Line 53: 'target_name': 'libffmpegsumo',
Why'd you do this?  Don't.

Doesn't it result in liblibffmpegsumo?

Maybe you want

'target_name': 'ffmpegsumo',
'product_name': 'libffmpegsumo',

although I thought that Xcode would prepend "lib" to the product_name too.

http://codereview.chromium.org/340072/diff/8002/1031#newcode241
Line 241: # TODO(ajwong): Check if we can remove this.
Resolve this TODO now by removing it (if you can?)  :)

http://codereview.chromium.org/340072/diff/8002/1031#newcode245
Line 245: # TODO(ajwong): Statically link these once we resolve the
Not sure this'll be a good idea - since ffmpegsumo is a separate module,
statically linking our own z and bz2 libraries means we'll be shipping that code
twice.  Maybe we want to and maybe we don't, but it's not an open-and-shut case.

http://codereview.chromium.org/340072/diff/8002/1031#newcode250
Line 250: 'GCC_SYMBOLS_PRIVATE_EXTERN': 'NO',
I like to comment alongside these saying what the relevant gcc flags are.  See
the big xcode_settings block in build/common.gypi around line 715.

http://codereview.chromium.org/340072/diff/8002/1031#newcode256
Line 256: '-Wl,-read_only_relocs,suppress'
Put a comment here saying that it makes Mark sad but that he's OK with it
because it's isolated to this module.  When Mark finds this in the future and
has forgotten this conversation, the comment will remind him.

http://codereview.chromium.org/340072/diff/8002/1031#newcode261
Line 261: 'postbuild_name': 'Fix FFmpeg installname.',
No period in these usually.

http://codereview.chromium.org/340072/diff/8002/1031#newcode263
Line 263: '/usr/bin/install_name_tool',
Oh!  I made a mental note to remind you to check the output of otool -L and
share, and fix it up with install_name_tool if needed.  But you've already done
it.  Cool.

http://codereview.chromium.org/340072/diff/8002/1031#newcode334
Line 334: 'conditions': [
I'd say that the ia32/x64 conditions should be shared between Mac and
Linux/FreeBSD, but I'm not actually going to say that, because this will need to
be reworked when I do the 64 and then fat Mac builds in a few weeks.

http://codereview.chromium.org/340072/diff/8002/1032
File third_party/ffmpeg/source/config/Chrome/mac/ia32/config.h (right):

http://codereview.chromium.org/340072/diff/8002/1032#newcode1
Line 1: /* Automatically generated by configure - do not modify! */
Not really reading this file, as I'm sure you would have guessed...

http://codereview.chromium.org/340072/diff/8002/1036
File third_party/yasm/README.chromium (right):

http://codereview.chromium.org/340072/diff/8002/1036#newcode4
Line 4: 1) Get a clean version of the yasm source tree. The clean tree can be
found
Oh, so they cleaned up their act?  Nice.

http://codereview.chromium.org/340072/diff/8002/1041
File webkit/tools/test_shell/test_shell.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1041#newcode338
Line 338: '<(PRODUCT_DIR)/libffmpegsumo.dylib',
Uh-oh.  I don't see ffmpeg declared as a dependency anywhere in this file.

[awong, 2009-11-04 03:30:11]

Fixed the issues.  Running through try servers.

http://codereview.chromium.org/340072/diff/8002/1028
File build/all.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1028#newcode55
Line 55: '../third_party/yasm/yasm.gyp:*',
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> I'd put this in a conditional block gated on:
> 
> 'OS=="mac" or OS=="linux"'

Done.

http://codereview.chromium.org/340072/diff/8002/1028#newcode55
Line 55: '../third_party/yasm/yasm.gyp:*',
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> I'd put this in a conditional block gated on:
> 
> 'OS=="mac" or OS=="linux"'

Done.

http://codereview.chromium.org/340072/diff/8002/1029
File chrome/chrome.gyp (left):

http://codereview.chromium.org/340072/diff/8002/1029#oldcode5879
Line 5879: # ffmpeg.gyp so they don't diverge. (BUG=23602)
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> Preserve this TODO?

Done.

http://codereview.chromium.org/340072/diff/8002/1029
File chrome/chrome.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1029#newcode5832
Line 5832: 'destination': '<(PRODUCT_DIR)/$(CONTENTS_FOLDER_PATH)/Libraries',
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> 80, so I'd do a 4-space hanging indent on the next line.

Done.

http://codereview.chromium.org/340072/diff/8002/1030
File third_party/ffmpeg/README.chromium (right):

http://codereview.chromium.org/340072/diff/8002/1030#newcode133
Line 133: For mac, execute replace the ".so" in the files above with ".dylib".
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> Mac is a proper name, it gets a capital M.

Done.

http://codereview.chromium.org/340072/diff/8002/1031
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1031#newcode53
Line 53: 'target_name': 'libffmpegsumo',
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> Why'd you do this?  Don't.
> 
> Doesn't it result in liblibffmpegsumo?
> 
> Maybe you want
> 
> 'target_name': 'ffmpegsumo',
> 'product_name': 'libffmpegsumo',
> 
> although I thought that Xcode would prepend "lib" to the product_name too.

So, it seems that in linux, it's smart enough to not prepend the lib.  In mac,
it seems to need it.  I'll try the product_name change.

http://codereview.chromium.org/340072/diff/8002/1031#newcode241
Line 241: # TODO(ajwong): Check if we can remove this.
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> Resolve this TODO now by removing it (if you can?)  :)

Heh...okay, I'll just whack it.

http://codereview.chromium.org/340072/diff/8002/1031#newcode245
Line 245: # TODO(ajwong): Statically link these once we resolve the
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> Not sure this'll be a good idea - since ffmpegsumo is a separate module,
> statically linking our own z and bz2 libraries means we'll be shipping that
code
> twice.  Maybe we want to and maybe we don't, but it's not an open-and-shut
case.

Moved TODO to the top of the file.

http://codereview.chromium.org/340072/diff/8002/1031#newcode250
Line 250: 'GCC_SYMBOLS_PRIVATE_EXTERN': 'NO',
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> I like to comment alongside these saying what the relevant gcc flags are.  See
> the big xcode_settings block in build/common.gypi around line 715.

Done.

http://codereview.chromium.org/340072/diff/8002/1031#newcode256
Line 256: '-Wl,-read_only_relocs,suppress'
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> Put a comment here saying that it makes Mark sad but that he's OK with it
> because it's isolated to this module.  When Mark finds this in the future and
> has forgotten this conversation, the comment will remind him.

Done.

http://codereview.chromium.org/340072/diff/8002/1031#newcode261
Line 261: 'postbuild_name': 'Fix FFmpeg installname.',
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> No period in these usually.

Done.

http://codereview.chromium.org/340072/diff/8002/1031#newcode263
Line 263: '/usr/bin/install_name_tool',
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> Oh!  I made a mental note to remind you to check the output of otool -L and
> share, and fix it up with install_name_tool if needed.  But you've already
done
> it.  Cool.

:D

The default had the install_name stuck in /usr/local/lib.  This step feels like
it shouldn't need to be here since ideally, I'd think we'd want to pass the
install_name through gyp to the linker.

http://codereview.chromium.org/340072/diff/8002/1036
File third_party/yasm/README.chromium (right):

http://codereview.chromium.org/340072/diff/8002/1036#newcode4
Line 4: 1) Get a clean version of the yasm source tree. The clean tree can be
found
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> Oh, so they cleaned up their act?  Nice.

Kinda...the comment was stale.  Originally, when I was dragging out of svn, they
had autogen.sh which specifically is supposed to create the ./configure +
autotools artifacts inside the directory tree.

When I switched to a tarball (where those area already generated), I forgot to
update this.

http://codereview.chromium.org/340072/diff/8002/1041
File webkit/tools/test_shell/test_shell.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1041#newcode338
Line 338: '<(PRODUCT_DIR)/libffmpegsumo.dylib',
On 2009/11/04 02:45:05, Mark Mentovai wrote:
> Uh-oh.  I don't see ffmpeg declared as a dependency anywhere in this file.

It's pulled in transitively via media.gyp:media.

I think chrome.gyp works the same way...is it okay as is?

[Mark Mentovai, 2009-11-04 03:41:23]

Advance comments to answer your questions, I'll still do another pass with your
changes.

http://codereview.chromium.org/340072/diff/8002/1031
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1031#newcode263
Line 263: '/usr/bin/install_name_tool',
awong wrote:
> On 2009/11/04 02:45:05, Mark Mentovai wrote:
> > Oh!  I made a mental note to remind you to check the output of otool -L and
> > share, and fix it up with install_name_tool if needed.  But you've already
> done
> > it.  Cool.
> 
> :D
> 
> The default had the install_name stuck in /usr/local/lib.  This step feels
like
> it shouldn't need to be here since ideally, I'd think we'd want to pass the
> install_name through gyp to the linker.

OK, let's get rid of this action, and instead, in this target, in
xcode_settings, put:

'DYLIB_INSTALL_NAME_BASE': '@loader_path'

http://codereview.chromium.org/340072/diff/8002/1041
File webkit/tools/test_shell/test_shell.gyp (right):

http://codereview.chromium.org/340072/diff/8002/1041#newcode338
Line 338: '<(PRODUCT_DIR)/libffmpegsumo.dylib',
awong wrote:
> On 2009/11/04 02:45:05, Mark Mentovai wrote:
> > Uh-oh.  I don't see ffmpeg declared as a dependency anywhere in this file.
> 
> It's pulled in transitively via media.gyp:media.
> 
> I think chrome.gyp works the same way...is it okay as is?

Yeah, that's fine.

[Mark Mentovai, 2009-11-04 04:25:48]

Great!  LGTM with these changes and a final successful tryserver run.

http://codereview.chromium.org/340072/diff/2084/6043
File chrome/chrome.gyp (right):

http://codereview.chromium.org/340072/diff/2084/6043#newcode5861
Line 5861: # TODO(ajwong): Find a way to share this path with ffmpeg.gyp so
80 characters

http://codereview.chromium.org/340072/diff/2084/6044
File third_party/ffmpeg/README.chromium (right):

http://codereview.chromium.org/340072/diff/2084/6044#newcode133
Line 133: For Mac, execute replace the ".so" in the files above with ".dylib".
execute replace?

http://codereview.chromium.org/340072/diff/2084/6045
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/340072/diff/2084/6045#newcode157
Line 157: '_ISOC99_SOURCE',
Ineffective and pointless on the Mac.

http://codereview.chromium.org/340072/diff/2084/6045#newcode159
Line 159: '_LARGEFILE_SOURCE',
Ineffective and pointless on the Mac, you can probably move this to the
Linux/FreeBSD section.

http://codereview.chromium.org/340072/diff/2084/6045#newcode162
Line 162: '-std=c99',
Don't do this on the Mac, we already pick it up from build/common.gypi, and it's
already set to 'c99'.  Leaving it here will be ambiguous if one ever changes:
which one wins?

http://codereview.chromium.org/340072/diff/2084/6045#newcode163
Line 163: '-pthread',
Since you'll be peeling things out of here to not use on the Mac, get rid of
this too, it's not functional at all on the Mac.

http://codereview.chromium.org/340072/diff/2084/6045#newcode164
Line 164: '-fno-math-errno',
Same.

http://codereview.chromium.org/340072/diff/2084/6045#newcode227
Line 227: 'libraries': [
Put this in a 'link_settings' section.  It doesn't make a difference because we
know this is the linkable target, but it's a good example to set: 'libraries'
almost always belongs in 'link_settings'.

Same with the Linux/FreeBSD section on line 222.

http://codereview.chromium.org/340072/diff/2084/6045#newcode232
Line 232: 'GCC_DYNAMIC_NO_PIC': 'YES',  # -mdynamic-no-pic (-fPIC equiv)
"-fPIC equiv" is wrong, it's more like -fno-PIC.

http://codereview.chromium.org/340072/diff/2084/6045#newcode248
Line 248: # with this flag.
Very excellent.

http://codereview.chromium.org/340072/diff/2084/6045#newcode252
Line 252: 'postbuilds': [
See my previous e-mail for the xcode_setting that should obsolete this.

http://codereview.chromium.org/340072/diff/2084/6054
File third_party/yasm/yasm.gyp (right):

http://codereview.chromium.org/340072/diff/2084/6054#newcode1
Line 1: # Copyright (c) 2009 The Chromium Authors. All rights reserved.
I'm still impressed by how few changes this file needed.

[awong, 2009-11-04 20:07:49]

Addressed most of the comments, except for moving libraries into link_settings,
which, I think isn't what we want to do...

http://codereview.chromium.org/340072/diff/2084/6043
File chrome/chrome.gyp (right):

http://codereview.chromium.org/340072/diff/2084/6043#newcode5861
Line 5861: # TODO(ajwong): Find a way to share this path with ffmpeg.gyp so
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> 80 characters

Done.

http://codereview.chromium.org/340072/diff/2084/6044
File third_party/ffmpeg/README.chromium (right):

http://codereview.chromium.org/340072/diff/2084/6044#newcode133
Line 133: For Mac, execute replace the ".so" in the files above with ".dylib".
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> execute replace?

Done.

http://codereview.chromium.org/340072/diff/2084/6045
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/340072/diff/2084/6045#newcode157
Line 157: '_ISOC99_SOURCE',
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> Ineffective and pointless on the Mac.

Done.

http://codereview.chromium.org/340072/diff/2084/6045#newcode159
Line 159: '_LARGEFILE_SOURCE',
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> Ineffective and pointless on the Mac, you can probably move this to the
> Linux/FreeBSD section.

Done.

http://codereview.chromium.org/340072/diff/2084/6045#newcode162
Line 162: '-std=c99',
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> Don't do this on the Mac, we already pick it up from build/common.gypi, and
it's
> already set to 'c99'.  Leaving it here will be ambiguous if one ever changes:
> which one wins?

Done.

http://codereview.chromium.org/340072/diff/2084/6045#newcode163
Line 163: '-pthread',
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> Since you'll be peeling things out of here to not use on the Mac, get rid of
> this too, it's not functional at all on the Mac.

Done.

http://codereview.chromium.org/340072/diff/2084/6045#newcode164
Line 164: '-fno-math-errno',
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> Same.

Done.

http://codereview.chromium.org/340072/diff/2084/6045#newcode227
Line 227: 'libraries': [
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> Put this in a 'link_settings' section.  It doesn't make a difference because
we
> know this is the linkable target, but it's a good example to set: 'libraries'
> almost always belongs in 'link_settings'.
> 
> Same with the Linux/FreeBSD section on line 222.

Huh...that surprises me.  I actually think I don't want to do this....there's no
need to leak the dynamic library dependency directly up to the higher level
targets.  Here, libffmpegsumo.so has a dependency on libz.so.  However, the
higher level targets don't need these in their -l lines, and probalby shouldn't
have them right?

http://codereview.chromium.org/340072/diff/2084/6045#newcode232
Line 232: 'GCC_DYNAMIC_NO_PIC': 'YES',  # -mdynamic-no-pic (-fPIC equiv)
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> "-fPIC equiv" is wrong, it's more like -fno-PIC.

Done.

http://codereview.chromium.org/340072/diff/2084/6045#newcode252
Line 252: 'postbuilds': [
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> See my previous e-mail for the xcode_setting that should obsolete this.

Yay!  O3D has a similar hack that could be fixed too. :D :D

http://codereview.chromium.org/340072/diff/2084/6054
File third_party/yasm/yasm.gyp (right):

http://codereview.chromium.org/340072/diff/2084/6054#newcode1
Line 1: # Copyright (c) 2009 The Chromium Authors. All rights reserved.
On 2009/11/04 04:25:51, Mark Mentovai wrote:
> I'm still impressed by how few changes this file needed.

As am I.  +1 for gyp.

[awong, 2009-11-05 00:13:53]

Fixes for scons, and half-fix for the link_settings comment.

Because -lffmpegasm leaks, ffmpegsumo_nolink breaks because the
LIBRARY_SEARCH_PATH in xcode_settings doesn't leak through.

:(

[Mark Mentovai, 2009-11-05 00:19:34]

I'm ready to call it a day.  Let's get this in.  LGTM.