Add a flag to enable the fix for r1717

Add a flag to enable the fix for r1717

It seems like Chrome (and potentially other projects using gyp) depend on the
bug that was fixed in r1717.  So for now, only enable the fix if the target has
'allow_sharedlib_linksettings_propagation' explicitly set to False (by default,
it is True).

This will allow Chrome and other projects to continue building until they are
fixed to not depend on this bug, at which point, we could remove the flag.

BUG=
R=mark@chromium.org

Committed: https://code.google.com/p/gyp/source/detail?r=1722

[Shezan Baig (Bloomberg), 2013-09-07 10:31:03]

Setting the flag to True-by-default for now.  Not sure if I should reverse it
(make it False-by-default and force other projects to explicitly opt-in for the
buggy behavior).

[Nico, 2013-09-08 23:43:02]

I'll leave this to Mark.

[Mark Mentovai, 2013-09-09 15:50:08]

Before continuing further with this, which modifies link_settings in ways that
are now known to be incompatible with previous usage, I’d like to see a survey
of the failures encountered in https://chromiumcodereview.appspot.com/23991003/
to determine what sorts of changes in Chrome’s .gyp files will be necessary.

I agree with Shezan that the new meaning of link_settings is the better
definition in a vacuum, but since it’s now known to expose incompatibilities
with existing .gyp usage, it’s prudent to get a better understanding of those
incompatibilities before proceeding.

If we can’t audit existing usage for this quickly, we should back out r1717
temporarily so that GYP rolls aren’t otherwise held up by this.

[Shezan Baig (Bloomberg), 2013-09-09 16:49:04]

On 2013/09/09 15:50:08, Mark Mentovai wrote:
> Before continuing further with this, which modifies link_settings in ways that
> are now known to be incompatible with previous usage, I’d like to see a survey
> of the failures encountered in
https://chromiumcodereview.appspot.com/23991003/
> to determine what sorts of changes in Chrome’s .gyp files will be necessary.
> 
> I agree with Shezan that the new meaning of link_settings is the better
> definition in a vacuum, but since it’s now known to expose incompatibilities
> with existing .gyp usage, it’s prudent to get a better understanding of those
> incompatibilities before proceeding.
> 
> If we can’t audit existing usage for this quickly, we should back out r1717
> temporarily so that GYP rolls aren’t otherwise held up by this.


I did a quick survey of the win failures this weekend, my findings are below:

5 undefined symbols in the 'ui/views/views.gyp:views' target (shared_library):

* AccessibleObjectFromWindow, defined in oleacc.lib
* ImmReleaseContext, defined in imm32.lib
* ImmGetCompositionStringW, defined in imm32.lib
* ImmGetContext, defined in imm32.lib
* LresultFromObject, defined in oleacc.lib

Right now, it is "inheriting" these .libs from the 'ui/ui.gyp:ui' target (also
shared_library), which it directly depends on.

To fix this, views.gyp will need to be modified so that the 'views' target has
the following:
'link_settings': {
  'libraries': [ '-loleacc.lib', '-limm32.lib' ]
}

This makes sense because the undefined symbols are being referenced by .obj
files in views.dll, so 'views' should not depend on one of its dependencies
declaring the .libs for it.

I'll do a survey of the failures on the other platforms, but I suspect the gyp
changes needed to fix them, like this one, would be minor.

[Shezan Baig (Bloomberg), 2013-09-09 18:20:03]

On Linux, this takes a bit more grepping/analysis to figure out what's going on,
because the link error happens when linking the executable, instead of happening
when building the shared_library.

There are 3 undefined symbols when linking
'remoting/remoting.gyp:remoting_start_host' (executable):

lib/libcrssl.so: error: undefined reference to 'dlclose'
lib/libcrssl.so: error: undefined reference to 'dlopen'
lib/libcrssl.so: error: undefined reference to 'dlsym'


After some grepping/analysis, I found these symbols being referenced by
'net/third_party/nss/ssl/ssl3con.c', which is one of the sources in
'net/third_party/nss/ssl.gyp:libssl', which is a shared_library with
product_name set to 'crssl'.

The dependency tree from 'remoting_start_host' to 'crssl' looks like:

    remoting/remoting.gyp:remoting_start_host (executable)
    |-> remoting/remoting.gyp:remoting_host_setup_base (static_library)
        |-> remoting/remoting.gyp:remoting_host (static_library)
        |   |-> crypto/crypto.gyp:crypto (shared_library, product_name:
crcrypto)
        |       |-> build/linux/system.gyp:ssl (none)
        |           |-> net/third_party/nss/ssl.gyp:libssl (shared_library,
product_name: crssl)
        |-> base/base.gyp:base (shared_library)

These symbols require '-ldl' on the link line, and right now, this is provided
by 'base/base.gyp:base's link_settings.  However, after r1717, base's
link_settings are no longer propagated to 'remoting_start_host', so we get this
error.

In order to fix this, we will need to add the following to
'net/third_party/nss/ssl.gyp:libssl':

'link_settings': { 'libraries': [ '-ldl' ] },

This will fix the link of the 'remoting_start_host' executable because
libcrssl.so will know where to find these symbols.

On to Mac now :)

[Shezan Baig (Bloomberg), 2013-09-09 18:55:36]

On Mac, we get the following undefined symbols:

* _CFRelease
* _CFStringCreateWithFileSystemRepresentation
* _CFURLCreateWithFileSystemPath
* _CSBackupIsItemExcluded
* _CSBackupSetItemExcluded
* _kCFAllocatorDefault

when linking 'sql/sql.gyp:sql' (shared_library)

These symbols are referenced from 'third_party/sqlite/sqlite.gyp:sqlite'
(static_library)

In order to link successfully, 'sql/sql.gyp:sql' will need the '-framework
CoreFoundation' linker flag.  Right now, it inherits this from
'base/base.gyp:base' (shared_library).  After r1717, we will need to add the
following to 'third_party/sqlite/sqlite.gyp:sqlite':

    'link_settings': {
      'libraries': [
        '$(SDKROOT)/System/Library/Frameworks/CoreFoundation.framework',
      ],
    }

This will make 'sql/sql.gyp:sql' inherit CoreFoundation from
'third_party/sqlite/sqlite.gyp:sqlite' (where these functions are used) instead
of 'base/base.gyp:base'.

[Mark Mentovai, 2013-09-09 19:04:40]

I suspect that this isn’t an exhaustive list of changes required because the
builders gave up after hitting these failures without trying to build the rest
of Chrome, but the changes you suggest sound reasonable and if they’re the sort
of changes that we can expect in the rest of Chrome, they sound like the right
thing to do regardless. It sounds like this is simply an interaction between the
“components” build and having these link dependencies being poorly-specified in
the past because they were able to rely on this GYP bug.

I’m happy to review Chrome CLs that implement your suggestions. Thanks.

[Shezan Baig (Bloomberg), 2013-09-09 19:12:37]

Yes, I can create the chromium CLs to fix these.  As you said, after fixing
these issues, we'll probably run into other (similar) issues.

I'm not sure how we should proceed.  We could either:

1) Commit this CL, roll gyp, fix the issues (probably iteratively), revert this
CL
2) Commit this CL, roll gyp, fix the issues (probably iteratively), change the
flag to False-by-default
3) Don't commit this CL, fix the issues (probably iteratively), roll gyp

I'd rather not do 3), because this effectively blocks gyp rolls until the issues
are completely fixed (which could take a while).

[Mark Mentovai, 2013-09-09 19:20:46]

I’d like to do option 1, but let’s get the first Chrome CL in before committing
this just to be sure. If we fix Chrome, I don’t think we’ll need to maintain a
flag to restore the buggy behavior in GYP long-term.

[Shezan Baig (Bloomberg), 2013-09-09 21:43:33]

On 2013/09/09 19:20:46, Mark Mentovai wrote:
> I’d like to do option 1, but let’s get the first Chrome CL in before
committing
> this just to be sure. If we fix Chrome, I don’t think we’ll need to maintain a
> flag to restore the buggy behavior in GYP long-term.

I'm not sure what you mean.  Option 1 requires that we commit this CL first,
then the Chrome CLs (otherwise, it would be option 3).

[Mark Mentovai, 2013-09-09 21:48:02]

Steps:

1. Land the Chrome CL to fix the first batch of these problems.
2. Run a try job for the GYP roll again.
3. Look at results and make sure the defects are still all of the same character
as what we’e seen so far.
4. Land this CL.
[as needed] roll GYP in Chrome, including this CL.
5. Continue fixing Chrome.
6. Revert this CL.
7. Roll GYP in Chrome.

[Shezan Baig (Bloomberg), 2013-09-09 21:50:30]

On 2013/09/09 21:48:02, Mark Mentovai wrote:
> Steps:
> 
> 1. Land the Chrome CL to fix the first batch of these problems.
> 2. Run a try job for the GYP roll again.
> 3. Look at results and make sure the defects are still all of the same
character
> as what we’e seen so far.
> 4. Land this CL.
> [as needed] roll GYP in Chrome, including this CL.
> 5. Continue fixing Chrome.
> 6. Revert this CL.
> 7. Roll GYP in Chrome.


Ahh ok, will do.  Thanks.

[Mark Mentovai, 2013-09-10 21:02:49]

You don’t need OWNERS review in ui for gyp files.

[Mark Mentovai, 2013-09-11 15:11:58]

LGTM

I saw win7_aura fail in a similar way to its most recent try bot failure earlier
today. It’s probably got nothing to do with your change. Since the other
failures don’t look build-related, I think we can land this for now, and just
continue fixing up the .gyp files in Chrome.

The easiest way to continue testing the .gyp files in Chrome is to have a
not-for-commit Chrome CL that adds 'allow_sharedlib_linksettings_propagation' to
build/common.gypi, and to run try jobs against that CL.

[Shezan Baig (Bloomberg), 2013-09-11 15:21:14]

Committed patchset #1 manually as r1722 (presubmit successful).