xcode_settings entries with list values override entries with the same key

When adding a new xcode build setting, if the current value is a list,
append the new value to it.

[Cătălin Badea, 2013-01-30 13:51:59]

When using xcode_settings entries with list values such as
GC_PREPROCESSOR_DEFINITIONS, the new value should be appended to the list. 

Please review.

[Mark Mentovai, 2013-01-30 13:54:17]

Can you show me an example of how you’re using this?

GYP should already take care of this as long as you always set
GCC_PREPROCESSOR_DEFINITIONS to a list.

[Cătălin Badea, 2013-01-30 14:52:08]

On 2013/01/30 13:54:17, Mark Mentovai wrote:
> Can you show me an example of how you’re using this?
> 
> GYP should already take care of this as long as you always set
> GCC_PREPROCESSOR_DEFINITIONS to a list.

Here's an example of a gyp configuration that demonstrates the bug:
      'targets' : [
        {
          'target_name' : 't1',
          'type': 'static_library',
          'sources': [
            'some_source.c',
          ],
          'xcode_settings': {
            'GCC_PREPROCESSOR_DEFINITIONS': [
              'T1_DEFINE',
            ],
          },
          'direct_dependent_settings': {
            'xcode_settings': {
              'GCC_PREPROCESSOR_DEFINITIONS': [
                'T1_DEPENDENT_DEFINE',
              ],
            },
          },
        },
        {
          'target_name' : 't2',
          'type': 'executable',
          'sources' : [
            'dummy.c',
          ],
          'dependencies': [
            't1',
          ],
          'xcode_settings': {
            'GCC_PREPROCESSOR_DEFINITIONS': [
              'T2_DEFINE',
            ],
          },
        },
      ],

When building t2, dummy.c will be compiled with -DT1_DEPENDENT_DEFINE and
*without* -DT2_DEFINE.
This happens because when 'xcode_settings' is parsed, gyp will call
SetBuildSetting for each (key, value) pair which overwrites the current entry no
matter what.

From pylib/gyp/generator/xcode.py:1205
    if 'defines' in configuration:
        for define in configuration['defines']:
          set_define = EscapeXCodeArgument(define)
          xcbc.AppendBuildSetting('GCC_PREPROCESSOR_DEFINITIONS', set_define)
      if 'xcode_settings' in configuration:
        for xck, xcv in configuration['xcode_settings'].iteritems():
          xcbc.SetBuildSetting(xck, xcv)

This does not that happen for the 'defines' key, which also uses
GCC_PREPROCESSOR_DEFINITIONS. When 'defines' is parsed, each new value is
appended to the current list.

How I encountered the problem: I've used GCC_PREPROCESSOR_DEFINITIONS[arch=i386]
and GCC_PREPROCESSOR_DEFINITIONS[arch=x86_64] to generate xcode projects that
build a fat Chromium and noticed that targets dependencies overwrite the build
flags.

[Sam Clegg, 2013-01-30 16:08:59]

https://codereview.chromium.org/12094059/diff/1/pylib/gyp/xcodeproj_file.py
File pylib/gyp/xcodeproj_file.py (right):

https://codereview.chromium.org/12094059/diff/1/pylib/gyp/xcodeproj_file.py#n...
pylib/gyp/xcodeproj_file.py:1562:
self._properties['buildSettings'][key].extend(value)
nit: I think you can write these three lines as one using
the .setdefault() method on the dict.

Actually there is an existing method below called
AppendBuildSettings() that already does this (and should
probably use setdefault() itself).

[Cătălin Badea, 2013-01-31 12:33:37]

Update: Using .setdefault for adding new entries.

Note that AppendBuildSetting does not take lists as a value parameter.

[Cătălin Badea, 2013-02-06 12:52:59]

The patch set is updated.

[Sam Clegg, 2013-02-06 17:30:51]

On 2013/02/06 12:52:59, Cătălin Badea wrote:
> The patch set is updated.

Looks good from a readability POV.  Mark, do you still have concerns
about the necessity the change?

[Mark Mentovai, 2013-02-06 17:47:37]

Yeah, I’m still not satisfied.

Catalin, I tried out your sample .gyp file in test.gyp, ran "gyp --depth .", and
made some source files with 'touch some_source.c' and 'echo "int main(int argc,
char* argv[]) {}" > dummy.c'. Then I ran xcodebuild.

dummy.c in static library target t1 was built with -DT1_DEFINE.

some_source.c in executable target t2 was built with -DT2_DEFINE
-DT1_DEPENDENT_DEFINE.

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
… -DT1_DEFINE … -c /tmp/some_source.c …
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
… -DT2_DEFINE -DT1_DEPENDENT_DEFINE … -c /tmp/dummy.c …

This appears correct to me, and it doesn’t match what you said you observed.

[Cătălin Badea, 2013-02-07 11:31:01]

Yes, you're right - the given example is working correctly.  Try adding a
'defines' entry to the second target and you'll notice it will not modify the
build flags.
Example:
{
  'targets': [
  ],
  'conditions': [
    ['OS=="mac"', {
      'targets' : [
        {
          'target_name' : 't1',
          'type': 'static_library',
          'toolsets': ['host', 'target'],
          'sources': [
            'main_t1.c',
          ],
          'xcode_settings': {
            'GCC_PREPROCESSOR_DEFINITIONS': [
              'T1_DEFINE',
            ],
          },
          'direct_dependent_settings': {
            'xcode_settings': {
              'GCC_PREPROCESSOR_DEFINITIONS': [
                'T1_DEPENDENT_DEFINE',
              ],
            },
          },
        },
        {
          'target_name' : 't2',
          'type': 'executable',
          'toolsets': ['host', 'target'],
          'sources' : [
            'main_t2.c',
          ],
          'dependencies': [
            't1',
          ],
          'defines': [
            'T2_DEFINE',
          ],
        },
      ],
    }],
  ],
}

t1 is compiled with -DT1_DEFINE
t2 is compiled with -DT1_DEPENDENT_DEFINE, thus missing the 'T2_DEFINE'

On 2013/02/06 17:47:37, Mark Mentovai wrote:
> Yeah, I’m still not satisfied.
> 
> Catalin, I tried out your sample .gyp file in test.gyp, ran "gyp --depth .",
and
> made some source files with 'touch some_source.c' and 'echo "int main(int
argc,
> char* argv[]) {}" > dummy.c'. Then I ran xcodebuild.
> 
> dummy.c in static library target t1 was built with -DT1_DEFINE.
> 
> some_source.c in executable target t2 was built with -DT2_DEFINE
> -DT1_DEPENDENT_DEFINE.
> 
>
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
> … -DT1_DEFINE … -c /tmp/some_source.c …
>
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
> … -DT2_DEFINE -DT1_DEPENDENT_DEFINE … -c /tmp/dummy.c …
> 
> This appears correct to me, and it doesn’t match what you said you observed.

[Mark Mentovai, 2013-02-07 16:45:52]

I see. This is about mixing 'defines' with the 'GCC_PREPROCESSOR_DEFINITIONS'
xcode_setting.

LGTM to fix this bug in the general case.

However, you mention that the reason you’re using GCC_PREPROCESSOR_DEFINITIONS
is to be able to use the [arch=] syntax in Xcode. We’re no longer recommending
this as it’s not compatible with ninja. We’ve removed the only uses of this
syntax from Chrome and don’t plan on adding any more.

https://chromiumcodereview.appspot.com/10828060 contains an example. The
recommended approach is to #define the necessary macros conditionally in a
header. That example contains -include, which isn’t optimal, and was only used
there because it involves third-party code that did not already #include a
common header on its own. In the majority of cases, it’s preferable to make the
code #include the header that contains the architecture-specific #defines on its
own.

[M-A Ruel, 2013-02-08 16:59:50]

FTR, there's no commit queue for gyp, please have it be committed on your
behalf. Thanks.

[Sam Clegg, 2015-02-27 21:39:05]

Are you still interested in having this committed?