This change supports '!' and '/' operators for 'dependencies' section.

This change supports '!' and '/' operators for 'dependencies' section.

The list filters are not supported for 'dependencies' sections for now, and
this change supports them.
http://code.google.com/p/gyp/wiki/InputFormatReference#List_Filters

Patch by Yuki Shiino <yukishiino@google.com>;
Committed: http://code.google.com/p/gyp/source/detail?r=921

[Mark Mentovai, 2011-04-26 19:12:23]

In Chromium, we treat dependencies as additive, and just write “dependencies”
inside “conditions” sections as needed. We don’t attempt to filter them. Would
that work for you?

http://codereview.chromium.org/6904020/diff/1/pylib/gyp/input.py
File pylib/gyp/input.py (right):

http://codereview.chromium.org/6904020/diff/1/pylib/gyp/input.py#newcode2242
pylib/gyp/input.py:2242: ProcessListFiltersInDict(target, target_dict)
This will process the filters on dependencies again (although at this point GYP
won’t use them for dependencies). That seems wrong.

[Yuki Shiino, 2011-04-27 08:45:37]

We could do the same additive way theoretically, but it's a very straightforward
way to remove a dependency in some cases.  Especially when we remove some source
files and/or flags at the same time.  It's possible that, if we don't need some
source files, we don't need some dependencies, too.

For example,
'conditions': [
  ['FOO=="BAR"', {
    'cflags!': [ ... ],
    'defines!': [ ... ],
    'sources!': [ ... ],
    'dependencies!': [ ... ],
  }],
]

http://codereview.chromium.org/6904020/diff/1/pylib/gyp/input.py
File pylib/gyp/input.py (right):

http://codereview.chromium.org/6904020/diff/1/pylib/gyp/input.py#newcode2242
pylib/gyp/input.py:2242: ProcessListFiltersInDict(target, target_dict)
On 2011/04/26 19:12:24, Mark Mentovai wrote:
> This will process the filters on dependencies again (although at this point
GYP
> won’t use them for dependencies). That seems wrong.

Done.
I changed the code to remove '!' and '/' keys in BuildDependencyList().

[Mark Mentovai, 2011-05-03 18:32:35]

http://codereview.chromium.org/6904020/diff/5001/pylib/gyp/input.py
File pylib/gyp/input.py (right):

http://codereview.chromium.org/6904020/diff/5001/pylib/gyp/input.py#newcode1369
pylib/gyp/input.py:1369: spec['dependencies'] = deps_dict['dependencies']
1. I don’t want to set spec['dependencies'] at all if it wasn’t originally set.

2. Why only dependencies and not all of dependency_sections? You seem to have
accounted for all dependency_sections above.

3. We don’t remove sources! and sources/ from the tree in the other case. We
should be consistent. Perhaps the filter-process routines themselves should be
responsible for deleting the ! and / things. Even better, maybe they should just
save them but rename them so that they won’t be processed on a further round.

[Yuki Shiino, 2011-05-04 10:49:35]

I considered again why the code looks ugly, and found a much simpler solution.

We should apply the filters on an early stage. I've updated the code in this
way.  I think we should apply the filters before the call to
AdjustStaticLibraryDependencies().

Could you take another look?

[Yuki Shiino, 2011-05-04 10:52:11]

http://codereview.chromium.org/6904020/diff/5001/pylib/gyp/input.py
File pylib/gyp/input.py (right):

http://codereview.chromium.org/6904020/diff/5001/pylib/gyp/input.py#newcode1369
pylib/gyp/input.py:1369: spec['dependencies'] = deps_dict['dependencies']
I've changed the way, but I explain what I thought before, just FYI.

On 2011/05/03 18:32:35, Mark Mentovai wrote:
> 1. I don’t want to set spec['dependencies'] at all if it wasn’t originally
set.

This 'else' clause runs only when spec['dependencies'] has originally been set.

> 2. Why only dependencies and not all of dependency_sections? You seem to have
> accounted for all dependency_sections above.

It seems right.

> 3. We don’t remove sources! and sources/ from the tree in the other case. We
> should be consistent. Perhaps the filter-process routines themselves should be
> responsible for deleting the ! and / things. Even better, maybe they should
just
> save them but rename them so that they won’t be processed on a further round.

ProcessListFiltersInDict() removes sources! and sources/ from the tree, if I
understand correctly.  The function makes the 'sources_excluded' key and stores
excluded values, but there seems no other values saved.

[Mark Mentovai, 2011-05-04 15:04:35]

Oh, my mistake, you’re right. I had thought that ProcessListFiltersInDict
preserved old ! and / lists, but it doesn’t, it deletes them. My third comment
from before was incorrect.

I think this will work. LGTM with these minor changes.

http://codereview.chromium.org/6904020/diff/11001/pylib/gyp/input.py
File pylib/gyp/input.py (right):

http://codereview.chromium.org/6904020/diff/11001/pylib/gyp/input.py#newcode2198
pylib/gyp/input.py:2198: if key in target_dict and len(target_dict[key]) > 0:
Get rid of the len check, I think this will be fine with “key in target_dict”.
There’s no guarantee that everything will treat “empty dict or list” identically
to “no dict or list,” so it’s better to keep the data in as close to the
original form as possible.

http://codereview.chromium.org/6904020/diff/11001/pylib/gyp/input.py#newcode2200
pylib/gyp/input.py:2200: del target_dict[key]  # Remove the key applied.
This comment doesn’t make any sense to me. I’m not sure what “applied” refers
to. What this line is doing is obvious to me, though.

[Yuki Shiino, 2011-05-04 15:47:48]

Thanks for the review.
I've updated the code.

http://codereview.chromium.org/6904020/diff/11001/pylib/gyp/input.py
File pylib/gyp/input.py (right):

http://codereview.chromium.org/6904020/diff/11001/pylib/gyp/input.py#newcode2198
pylib/gyp/input.py:2198: if key in target_dict and len(target_dict[key]) > 0:
On 2011/05/04 15:04:35, Mark Mentovai wrote:
> Get rid of the len check, I think this will be fine with “key in target_dict”.
> There’s no guarantee that everything will treat “empty dict or list”
identically
> to “no dict or list,” so it’s better to keep the data in as close to the
> original form as possible.

Done.

http://codereview.chromium.org/6904020/diff/11001/pylib/gyp/input.py#newcode2200
pylib/gyp/input.py:2200: del target_dict[key]  # Remove the key applied.
On 2011/05/04 15:04:35, Mark Mentovai wrote:
> This comment doesn’t make any sense to me. I’m not sure what “applied” refers
> to. What this line is doing is obvious to me, though.

Done.

[Mark Mentovai, 2011-05-04 16:33:45]

LGTM

[Mark Mentovai, 2011-05-04 20:35:11]

I committed this at GYP r921.