Add support for component=shared_library for base_unittests_run.

Add support for component=shared_library for base_unittests_run.

Specifically, this adds support to run base_unittests isolated
via 'isolate.py run' when built with component build.

This requires rolling icu @ 246118 to get the new icu.isolate.

R=thakis@chromium.org,jshin@chromium.org,frankf@chromium.org,rsleevi@chromium.org
BUG=336439
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246247

[M-A Ruel, 2014-01-21 17:53:55]

jshin for DEPS
thakis for base/ and build/isolate.py
frankf for build/android/gtest/setup.py
rsleevi for net/third_party/nss/ssl.isolate

https://codereview.chromium.org/132233030/diff/1/DEPS
File DEPS (right):

https://codereview.chromium.org/132233030/diff/1/DEPS#newcode101
DEPS:101: "/trunk/deps/third_party/icu46@244135", # DO NOT SUBMIT.
This depends on https://codereview.chromium.org/143903004/ being checked in.

https://codereview.chromium.org/132233030/diff/1/build/android/pylib/gtest/se...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/132233030/diff/1/build/android/pylib/gtest/se...
build/android/pylib/gtest/setup.py:118: '--outdir', constants.ISOLATE_DEPS_DIR,
Sort the flags, general flags first, then config flags. There will be a lot of
these in the near future.

https://codereview.chromium.org/132233030/diff/1/build/isolate.gypi
File build/isolate.gypi (right):

https://codereview.chromium.org/132233030/diff/1/build/isolate.gypi#newcode75
build/isolate.gypi:75: # Variables should use the -V FOO=<(FOO) form so frequent
values,
Try to increase the maintainability of this file by adding more explanations and
separating the configurations per purpose.

[Nico, 2014-01-21 17:57:46]

Please improve the cl description to say "isolate" somewhere. I was like "huh
base_unittests already builds fine in a components build" when I read the cl
description.

[M-A Ruel, 2014-01-21 18:01:34]

On 2014/01/21 17:57:46, Nico wrote:
> Please improve the cl description to say "isolate" somewhere. I was like "huh
> base_unittests already builds fine in a components build" when I read the cl
> description.

Done.

[Nico, 2014-01-21 18:02:03]

…other than that, my files lgtm

[frankf, 2014-01-21 19:00:36]

build/android lgtm

[Ryan Sleevi, 2014-01-21 19:43:50]

https://codereview.chromium.org/132233030/diff/1/base/base.isolate
File base/base.isolate (right):

https://codereview.chromium.org/132233030/diff/1/base/base.isolate#newcode35
base/base.isolate:35: '../net/third_party/nss/ssl.isolate',
Why are you including this isolate unconditionally.

base.gyp depends on system.gyp:ssl, which handles use_openssl=1 and
use_openssl=0.

This will unconditionally include the NSS bits on OpenSSL, which doesn't seem
correct.

https://codereview.chromium.org/132233030/diff/1/build/android/pylib/gtest/se...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/132233030/diff/1/build/android/pylib/gtest/se...
build/android/pylib/gtest/setup.py:125: '--config-variable', 'use_openssl', '0',
Surely this isn't correct for Android, which is always use_openssl=1

https://codereview.chromium.org/132233030/diff/1/build/isolate.gypi
File build/isolate.gypi (right):

https://codereview.chromium.org/132233030/diff/1/build/isolate.gypi#newcode100
build/isolate.gypi:100: #'--config-variable',
'use_system_ssl=<(use_system_ssl)',
Filed http://crbug.com/336511 - this isn't something we really want to support
:)

https://codereview.chromium.org/132233030/diff/1/net/third_party/nss/ssl.isolate
File net/third_party/nss/ssl.isolate (right):

https://codereview.chromium.org/132233030/diff/1/net/third_party/nss/ssl.isol...
net/third_party/nss/ssl.isolate:6: # TODO(maruel): Add 'and use_system_ssl==0'
once it is consistently defined.
?

[M-A Ruel, 2014-01-21 19:54:44]

https://codereview.chromium.org/132233030/diff/1/base/base.isolate
File base/base.isolate (right):

https://codereview.chromium.org/132233030/diff/1/base/base.isolate#newcode35
base/base.isolate:35: '../net/third_party/nss/ssl.isolate',
On 2014/01/21 19:43:50, Ryan Sleevi wrote:
> Why are you including this isolate unconditionally.
> 
> base.gyp depends on system.gyp:ssl, which handles use_openssl=1 and
> use_openssl=0.
> 
> This will unconditionally include the NSS bits on OpenSSL, which doesn't seem
> correct.

There's no way to include conditionally. It is true of GYP too btw.
The condition in ssl.isolate prevails.

[Ryan Sleevi, 2014-01-21 19:56:47]

On 2014/01/21 19:54:44, M-A Ruel wrote:
> https://codereview.chromium.org/132233030/diff/1/base/base.isolate
> File base/base.isolate (right):
> 
> https://codereview.chromium.org/132233030/diff/1/base/base.isolate#newcode35
> base/base.isolate:35: '../net/third_party/nss/ssl.isolate',
> On 2014/01/21 19:43:50, Ryan Sleevi wrote:
> > Why are you including this isolate unconditionally.
> > 
> > base.gyp depends on system.gyp:ssl, which handles use_openssl=1 and
> > use_openssl=0.
> > 
> > This will unconditionally include the NSS bits on OpenSSL, which doesn't
seem
> > correct.
> 
> There's no way to include conditionally. It is true of GYP too btw.
> The condition in ssl.isolate prevails.

Fair enough.

There's still the question about use_openssl=0 in the Android file, however.

[M-A Ruel, 2014-01-21 20:00:28]

https://codereview.chromium.org/132233030/diff/1/build/android/pylib/gtest/se...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/132233030/diff/1/build/android/pylib/gtest/se...
build/android/pylib/gtest/setup.py:125: '--config-variable', 'use_openssl', '0',
On 2014/01/21 19:43:50, Ryan Sleevi wrote:
> Surely this isn't correct for Android, which is always use_openssl=1

While technically false, it's true from an isolate perspective. We do not need
to map openssl's library, it is using the system one.

https://codereview.chromium.org/132233030/diff/1/net/third_party/nss/ssl.isolate
File net/third_party/nss/ssl.isolate (right):

https://codereview.chromium.org/132233030/diff/1/net/third_party/nss/ssl.isol...
net/third_party/nss/ssl.isolate:6: # TODO(maruel): Add 'and use_system_ssl==0'
once it is consistently defined.
On 2014/01/21 19:43:50, Ryan Sleevi wrote:
> ?

use_system_ssl is defined build/linux/system.gyp but this is not consistently
defined on other platforms. This results in gyp throwing up on non-linux
platforms.

[jungshik at Google, 2014-01-21 20:20:57]

icu-related changes, LGTM

[Ryan Sleevi, 2014-01-21 20:39:08]

https://codereview.chromium.org/132233030/diff/1/base/base.isolate
File base/base.isolate (right):

https://codereview.chromium.org/132233030/diff/1/base/base.isolate#newcode35
base/base.isolate:35: '../net/third_party/nss/ssl.isolate',
On 2014/01/21 19:54:45, M-A Ruel wrote:
> On 2014/01/21 19:43:50, Ryan Sleevi wrote:
> > Why are you including this isolate unconditionally.
> > 
> > base.gyp depends on system.gyp:ssl, which handles use_openssl=1 and
> > use_openssl=0.
> > 
> > This will unconditionally include the NSS bits on OpenSSL, which doesn't
seem
> > correct.
> 
> There's no way to include conditionally. It is true of GYP too btw.
> The condition in ssl.isolate prevails.

Can we not preserve the same level of 'indirection' that base.gyp has - namely,
it #includes build/linux/system.isolate, which can then unconditionally include
net/third_party/nss/ssl.isolate or any other isolates?

The reason I ask is that this flattens a layer of the dependency, which makes it
harder to maintain, in terms of the .isolate files living next to their .gyp
files. I agree it creates more maintenance to have an extra .isolate file
around, but at least the chain of .isolates match their chain of .gyp, and as we
cleanup .gyp, we remember to clean up .isolate

[M-A Ruel, 2014-01-21 21:01:54]

https://codereview.chromium.org/132233030/diff/1/base/base.isolate
File base/base.isolate (right):

https://codereview.chromium.org/132233030/diff/1/base/base.isolate#newcode35
base/base.isolate:35: '../net/third_party/nss/ssl.isolate',
On 2014/01/21 20:39:09, Ryan Sleevi wrote:
> Can we not preserve the same level of 'indirection' that base.gyp has -
namely,
> it #includes build/linux/system.isolate, which can then unconditionally
include
> net/third_party/nss/ssl.isolate or any other isolates?
> 
> The reason I ask is that this flattens a layer of the dependency, which makes
it
> harder to maintain, in terms of the .isolate files living next to their .gyp
> files. I agree it creates more maintenance to have an extra .isolate file
> around, but at least the chain of .isolates match their chain of .gyp, and as
we
> cleanup .gyp, we remember to clean up .isolate

I can't, the language is intentionally not expressive enough. You can't
reference a gyp target, the gyp->isolate jump is one-way. Even if there were a
build/linux/system.isolate, it would contain a single 'includes' in it, which
makes it less maintainable IMHO. The other way is to merge
net/third_party/nss/ssl.isolate right here, which I had initially done and is
even more flat but easy to understand.

[Ryan Sleevi, 2014-01-21 21:50:14]

On 2014/01/21 21:01:54, M-A Ruel wrote:
> https://codereview.chromium.org/132233030/diff/1/base/base.isolate
> File base/base.isolate (right):
> 
> https://codereview.chromium.org/132233030/diff/1/base/base.isolate#newcode35
> base/base.isolate:35: '../net/third_party/nss/ssl.isolate',
> On 2014/01/21 20:39:09, Ryan Sleevi wrote:
> > Can we not preserve the same level of 'indirection' that base.gyp has -
> namely,
> > it #includes build/linux/system.isolate, which can then unconditionally
> include
> > net/third_party/nss/ssl.isolate or any other isolates?
> > 
> > The reason I ask is that this flattens a layer of the dependency, which
makes
> it
> > harder to maintain, in terms of the .isolate files living next to their .gyp
> > files. I agree it creates more maintenance to have an extra .isolate file
> > around, but at least the chain of .isolates match their chain of .gyp, and
as
> we
> > cleanup .gyp, we remember to clean up .isolate
> 
> I can't, the language is intentionally not expressive enough. You can't
> reference a gyp target, the gyp->isolate jump is one-way. 

I wasn't suggesting this.

> Even if there were a
> build/linux/system.isolate, it would contain a single 'includes' in it, which
> makes it less maintainable IMHO. 

I was suggesting this. I think it is more maintainable - especially as we
continue to grow .isolate files that depend on build/linux/system.gyp:ssl

> The other way is to merge
> net/third_party/nss/ssl.isolate right here, which I had initially done and is
> even more flat but easy to understand.

As you extend to other .isolate's, this will continue to grow, and will make
long-term maintenance harder.

[M-A Ruel, 2014-01-22 01:42:02]

On 2014/01/21 21:50:14, Ryan Sleevi wrote:
> On 2014/01/21 21:01:54, M-A Ruel wrote:
> > Even if there were a
> > build/linux/system.isolate, it would contain a single 'includes' in it,
which
> > makes it less maintainable IMHO. 
> 
> I was suggesting this. I think it is more maintainable - especially as we
> continue to grow .isolate files that depend on build/linux/system.gyp:ssl

Done.

[Ryan Sleevi, 2014-01-22 02:21:43]

https://codereview.chromium.org/132233030/diff/210001/net/third_party/nss/ssl...
File net/third_party/nss/ssl.isolate (right):

https://codereview.chromium.org/132233030/diff/210001/net/third_party/nss/ssl...
net/third_party/nss/ssl.isolate:6: # TODO(maruel): Add 'and use_system_ssl==0'
once it is consistently defined.
Feel free to remove this TODO - https://codereview.chromium.org/139743009/

[Ryan Sleevi, 2014-01-22 02:21:51]

LGTM

[M-A Ruel, 2014-01-22 02:23:53]

https://codereview.chromium.org/132233030/diff/210001/net/third_party/nss/ssl...
File net/third_party/nss/ssl.isolate (right):

https://codereview.chromium.org/132233030/diff/210001/net/third_party/nss/ssl...
net/third_party/nss/ssl.isolate:6: # TODO(maruel): Add 'and use_system_ssl==0'
once it is consistently defined.
On 2014/01/22 02:21:44, Ryan Sleevi wrote:
> Feel free to remove this TODO - https://codereview.chromium.org/139743009/

Ah! Removed here and in build/isolate.gypi.

[commit-bot: I haz the power, 2014-01-22 02:47:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/132233030/290001

[commit-bot: I haz the power, 2014-01-22 09:06:09]

Change committed as 246247