media: remove redundant gyp excludes

media: remove redundant gyp excludes

I moved all of these general patterns to common.gypi.
Also rearrange the common.gypi patterns to cover more common
Linux patterns.

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

[awong, 2010-02-17 17:11:53]

LGTM

On Wed, Feb 17, 2010 at 6:31 AM, <evan@chromium.org> wrote:

> Reviewers: bradnelson, Mark Mentovai,
>
> Description:
> media: remove redundant gyp excludes
>
> I moved all of these general patterns to common.gypi.
>
> Please review this at http://codereview.chromium.org/608032
>
> Affected files:
>  M media/media.gyp
>
>
> Index: media/media.gyp
> diff --git a/media/media.gyp b/media/media.gyp
> index
>
f0f15b7bc33f96beacdb4c91320cc01b5e82d2bc..eb5c39210edb411358d066a91be0837e15b9ca2f
> 100644
> --- a/media/media.gyp
> +++ b/media/media.gyp
> @@ -7,14 +7,6 @@
>     'chromium_code': 1,
>     'player_x11_renderer%': 'x11',
>   },
> -  'target_defaults': {
> -    'conditions': [
> -      ['OS!="linux"', {'sources/': [['exclude', '/linux/']]}],
> -      ['OS!="freebsd"', {'sources/': [['exclude', '/freebsd/']]}],
> -      ['OS!="mac"', {'sources/': [['exclude', '/mac/']]}],
> -      ['OS!="win"', {'sources/': [['exclude', '/win/']]}],
> -    ],
> -  },
>   'targets': [
>     {
>       'target_name': 'media',
> @@ -140,22 +132,12 @@
>       },
>       'conditions': [
>         ['OS =="linux"', {
> -          'sources/': [ ['exclude', '_(mac|win)\\.cc$'],
> -                        ['exclude', '\\.mm?$' ] ],
>           'link_settings': {
>             'libraries': [
>               '-lasound',
>             ],
>           },
>         }],
> -        ['OS =="freebsd"', {
> -          'sources/': [ ['exclude', '_(mac|win)\\.cc$'],
> -                        ['exclude', '\\.mm?$' ] ],
> -          'link_settings': {
> -            'libraries': [
> -            ],
> -          },
> -        }],
>         ['OS =="mac"', {
>           'link_settings': {
>             'libraries': [
> @@ -163,12 +145,6 @@
>               '$(SDKROOT)/System/Library/Frameworks/CoreAudio.framework',
>             ],
>           },
> -          'sources/': [ ['exclude', '_(linux|win)\\.cc$'],
> -          ],
> -        }],
> -        [ 'OS == "win"', {
> -          'sources/': [ ['exclude', '_(linux|mac|posix)\\.cc$'],
> -                        ['exclude', '\\.mm?$' ] ],
>         }],
>       ],
>     },
>
>
>

[Mark Mentovai, 2010-02-17 17:36:16]

LGTM.  The comment isn't something to necessarily fix, but it's something that
stuck out as an oddity.

http://codereview.chromium.org/608032/diff/2002/3001
File build/common.gypi (right):

http://codereview.chromium.org/608032/diff/2002/3001#newcode468
build/common.gypi:468: ['exclude',
'_(chromeos|gtk|linux|x|x11)(_unittest)?\\.cc$'],
Seems incongruous that _linux.cc means "Linux and a couple BSDs" but /linux/
means "Linux and nothing else."

[fbarchard, 2010-02-17 17:43:34]

Looks cleaner, if we want to continue encouraging platform excludes.  We
historically did it so visual studio would allow us to see all the files for all
platforms, even if not built for windows.  Plus it looks clean.
On ffmpeg we used to use too many excludes, and the gyp became more readable by
including just what is needed for each platform.
I had some minor concerns, but a quick test build on windows worked.
lgtm.

http://codereview.chromium.org/608032/diff/2002/3001
File build/common.gypi (right):

http://codereview.chromium.org/608032/diff/2002/3001#newcode466
build/common.gypi:466: ['OS!="linux" and OS!="freebsd" and OS!="openbsd"', {
should this include 'OS!="solaris"

http://codereview.chromium.org/608032/diff/2002/3001#newcode475
build/common.gypi:475: ['exclude', '/linux/'],
In media, there are also excludes on _linux.cc etc
Should those also be here?
The downside, I'm guessing, is that more complex excludes will slow down builds
in general, and only media takes advantage of them.

[Evan Martin, 2010-02-17 17:45:07]

On 2010/02/17 17:36:16, Mark Mentovai wrote:
> http://codereview.chromium.org/608032/diff/2002/3001#newcode468
> build/common.gypi:468: ['exclude',
> '_(chromeos|gtk|linux|x|x11)(_unittest)?\\.cc$'],
> Seems incongruous that _linux.cc means "Linux and a couple BSDs" but /linux/
> means "Linux and nothing else."

Yeah, I noticed that too.  I intend to clean it up in a followup.
(The original BSD patches hacked up the _linux files but I've been telling them
to rename them to _posix or whatever if necessary.)

[Evan Martin, 2010-02-17 17:45:57]

On 2010/02/17 17:43:34, fbarchard wrote:
> should this include 'OS!="solaris"

I haven't committed the solaris patch yet (but they touched this file, which is
why I'm cleaning it up now -- they were adding more solaris tests below but I'm
deleting them all because they belong in common.gypi)

> 
> http://codereview.chromium.org/608032/diff/2002/3001#newcode475
> build/common.gypi:475: ['exclude', '/linux/'],
> In media, there are also excludes on _linux.cc etc
> Should those also be here?
> The downside, I'm guessing, is that more complex excludes will slow down
builds
> in general, and only media takes advantage of them.

[Evan Martin, 2010-02-17 17:46:20]

On 2010/02/17 17:43:34, fbarchard wrote:
> The downside, I'm guessing, is that more complex excludes will slow down
builds
> in general, and only media takes advantage of them.

Excludes are expanded at gyp time, so it's not so bad.

[Evan Martin, 2010-02-17 17:52:48]

On 2010/02/17 17:46:20, Evan Martin wrote:
> On 2010/02/17 17:43:34, fbarchard wrote:
> > The downside, I'm guessing, is that more complex excludes will slow down
> builds
> > in general, and only media takes advantage of them.
> 
> Excludes are expanded at gyp time, so it's not so bad.

On second thought, I'm doing Mark/Frank's suggestion in this change as well.

[fbarchard, 2010-02-17 18:12:49]

notice audio not working on http://chromium.jaggeri.com/

http://codereview.chromium.org/608032/diff/7001/7002
File build/common.gypi (right):

http://codereview.chromium.org/608032/diff/7001/7002#newcode473
build/common.gypi:473: ['OS!="linux"', {
for freebsd, openbsd, and solaris, is it your intent to exclude _linux and
/linux/?

I'm thinking this is part of the reason openbsd/freebsd don't work for audio in
media.gyp?
Not a big deal.. we can add those platforms later.  But it doesn't seem fully
thought out.

[Evan Martin, 2010-02-17 18:23:56]

On 2010/02/17 18:12:49, fbarchard wrote:
> notice audio not working on http://chromium.jaggeri.com/

freebsd doesn't support ALSA, which is what media uses.
(ALSA = "advanced *linux* sound architecture")

> Not a big deal.. we can add those platforms later.  But it doesn't seem fully
> thought out.

Yeah, they're unsupported, so I don't feel too bad breaking them.

[fbarchard, 2010-02-17 18:38:53]

On 2010/02/17 18:23:56, Evan Martin wrote:
> On 2010/02/17 18:12:49, fbarchard wrote:
> > notice audio not working on http://chromium.jaggeri.com/
> 
> freebsd doesn't support ALSA, which is what media uses.
> (ALSA = "advanced *linux* sound architecture")

(I am not a linux guru)
There was a comment on that site that a port libasound is available, but not
tested.  asound = alsa?
We might want to treat this like nss and have a USE_ALSA that can be enabled.
> 
> > Not a big deal.. we can add those platforms later.  But it doesn't seem
fully
> > thought out.
> 
> Yeah, they're unsupported, so I don't feel too bad breaking them.

I don't think you're breaking it; You're just not adding the support.  Which is
fine.  Someone with the ability to test these platforms should file a bug and
submit the patch.
Its a step in the right direction, making media more platform agnostic.
I've been building various versions of windows the last hour, and so far all is
fine with your patches.
LGTM