Linux: add option to use system nspr.

Linux: add option to use system nspr.

Note that for NaCl bundles nspr files will be used anyway, since NaCl toolchain doesn't looks for libraries in system directories.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175539

[jar (doing other things), 2012-12-27 18:08:58]

Wan Teh: Can you comment on the wisdom of this change, or any potential problems
with various NSPR revs?

Ryan: Can you review the gyps and approach taken?

[Ryan Sleevi, 2012-12-27 18:39:48]

NACK.

This will break component builds.

The reason that they are in the same target (base) is that our modified prtime
makes use of Chromium constructs, ergo it depends on base.

You cannot have a dependency of base -> nspr -> base, and your change explictly
does not have nspr -> base.

You will first need to change the NSPR patches to have no Chromium dependencies.

[Ryan Sleevi, 2012-12-27 18:41:26]

https://codereview.chromium.org/11685002/diff/1004/base/third_party/nspr/nspr...
File base/third_party/nspr/nspr.gyp (right):

https://codereview.chromium.org/11685002/diff/1004/base/third_party/nspr/nspr...
base/third_party/nspr/nspr.gyp:20: 'prtime.cc',
You also dropped prtime.h and prcpucfg from the 'sources' that were in base.gypi

The omission from base.gypi also will break Nacl win64 builds.

[Paweł Hajdan Jr., 2013-01-03 18:04:17]

PTAL

[Ryan Sleevi, 2013-01-03 18:51:35]

https://codereview.chromium.org/11685002/diff/9001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/11685002/diff/9001/base/base.gyp#newcode213
base/base.gyp:213: }],
Why is this dependency not expressed in base.gypi, since that is what handles
the inclusion of the NSPR sources?

I don't like the disconnect from having the dependency in one file, but the
sources directive that necessitates or obviates that dependency in another.

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi#newcode730
base/base.gypi:730: ['<(use_system_nspr)==1', {
You're in a conditions block, do not use this variable syntax, since it's
implicit. (The same applies to the os_bsd and chromeos of lines 710/720,
respectively).

['use_system_nspr == 1', [
  [ ... ]
]

Also, why do you not need to check that >(nacl_untrusted_build) == 0? When
inside a NACL untrusted build, you surely don't intend to link in all of NSPR,
do you? If so, that should definitely get nacl & security-team review.

https://codereview.chromium.org/11685002/diff/9001/base/base_untrusted.gyp
File base/base_untrusted.gyp (right):

https://codereview.chromium.org/11685002/diff/9001/base/base_untrusted.gyp#ne...
base/base_untrusted.gyp:29: # does not look for libraries in system directories.
I don't think this comment is entirely accurate, since even if it did, using the
same config is not necessary correct. Compare prcpucfg_nacl.h vs
prcpucfg_linux.h under base/third_party/nspr

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
File base/third_party/nspr/nspr.gyp (right):

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
base/third_party/nspr/nspr.gyp:15: 'header_filenames': [
And why not prcpucfg.h?

[wtc, 2013-01-04 16:55:28]

Review comments on patch set 3:

This CL seems correct to me, but I don't know GYP as well as
rsleevi does, so please wait for his approval. Thanks.

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi#newcode22
base/base.gypi:22: 'third_party/nspr/prcpucfg_linux.h',

Please add prtypes.h and prcpucfg.h.

I wonder if we should also add the other prcpucfg_xxx.h files.
It seems wrong to only list prcpucfg_linux.h.

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi#newcode732
base/base.gypi:732: ['exclude', '^third_party/nspr/'],

This will exclude prtime.cc and use the PR_ParseTimeString
function from the system nspr library instead, right?

If so, our bundled prtime.h needs to declare the
PR_ParseTimeString function with extern "C" to avoid
potential name mangling issues. I suggest copying over
the PR_BEGIN_EXTERN_C and PR_END_EXTERN_C from upstream
prtime.h:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/prtime...

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/prtime...

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
File base/third_party/nspr/nspr.gyp (right):

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
base/third_party/nspr/nspr.gyp:7: ['use_system_nspr==1', {

In base/base.gyp, you added a dependency on this .gyp file
under the use_system_nspr==1 condition. So it seems
redundant to check the use_system_nspr==1 condition here.

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
base/third_party/nspr/nspr.gyp:15: 'header_filenames': [

I don't understand how this works. Could you explain why we
need to list the headers but not the C/C++ source files?
Thanks.

[Paweł Hajdan Jr., 2013-01-04 18:01:31]

Thank you for the comments, PTAL.

https://codereview.chromium.org/11685002/diff/9001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/11685002/diff/9001/base/base.gyp#newcode213
base/base.gyp:213: }],
On 2013/01/03 18:51:35, Ryan Sleevi wrote:
> Why is this dependency not expressed in base.gypi, since that is what handles
> the inclusion of the NSPR sources?
> 
> I don't like the disconnect from having the dependency in one file, but the
> sources directive that necessitates or obviates that dependency in another.

This is a problem I've hit multiple times with gyp. Generally having
dependencies in .gypi files doesn't seem to work. Here is an example how gyp
freaks out if I do that:

$ build/gyp_chromium -Duse_system_nspr=1
Updating projects from gyp files...
Traceback (most recent call last):
  File "build/gyp_chromium", line 144, in <module>
    sys.exit(gyp.main(args))
  File
"/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/__init__.py",
line 524, in main
    return gyp_main(args)
  File
"/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/__init__.py",
line 509, in gyp_main
    generator.GenerateOutput(flat_list, targets, data, params)
  File
"/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/generator/make.py",
line 2046, in GenerateOutput
    for target in gyp.common.AllTargets(target_list, target_dicts, build_file):
  File
"/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/common.py",
line 302, in AllTargets
    deptargets = DeepDependencyTargets(target_dicts, bftargets)
  File
"/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/common.py",
line 286, in DeepDependencyTargets
    spec = target_dicts[r]
KeyError: 'third_party/nspr/nspr.gyp:nspr'

Maybe I'm doing things wrong, so I'm happy to learn the correct way.

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi#newcode22
base/base.gypi:22: 'third_party/nspr/prcpucfg_linux.h',
On 2013/01/04 16:55:28, wtc wrote:
> 
> Please add prtypes.h and prcpucfg.h.
> 
> I wonder if we should also add the other prcpucfg_xxx.h files.
> It seems wrong to only list prcpucfg_linux.h.

Thanks, done.

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi#newcode730
base/base.gypi:730: ['<(use_system_nspr)==1', {
On 2013/01/03 18:51:35, Ryan Sleevi wrote:
> You're in a conditions block, do not use this variable syntax, since it's
> implicit. (The same applies to the os_bsd and chromeos of lines 710/720,
> respectively).
> 
> ['use_system_nspr == 1', [
>   [ ... ]
> ]

Nope. If I don't do that build/gyp_chromium bails out with:

gyp: name 'use_system_nspr' is not defined while evaluating condition
'use_system_nspr==1' in
/usr/local/google/home/phajdan/chromium/src/base/base.gyp

I may be doing something wrong, in which case I'm happy to learn. However, these
two are clearly *not* equivalent (which may be a bug).

> Also, why do you not need to check that >(nacl_untrusted_build) == 0? When
> inside a NACL untrusted build, you surely don't intend to link in all of NSPR,
> do you? If so, that should definitely get nacl & security-team review.

Note this conditional section excludes things, so it's not possible to drag more
things when excluding things.

By the way, linking to "outside world" is not possible or at least not easy in
NACL untrusted build. This change certainly doesn't do that, and continues to
use bundled prtime.cc for NaCl untrusted part.

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi#newcode732
base/base.gypi:732: ['exclude', '^third_party/nspr/'],
On 2013/01/04 16:55:28, wtc wrote:
> 
> This will exclude prtime.cc and use the PR_ParseTimeString
> function from the system nspr library instead, right?

Yep. We're using system headers though. That's the whole trick here: when using
system libs, we should always use system headers, and when using bundled libs we
should be using bundled headers.

The conclusion is that no change is necessary here, since we always use matching
headers, _and_ this is confirmed by the change working both for
use_system_nspr=0 and 1. Otherwise it would fail to link.

https://codereview.chromium.org/11685002/diff/9001/base/base_untrusted.gyp
File base/base_untrusted.gyp (right):

https://codereview.chromium.org/11685002/diff/9001/base/base_untrusted.gyp#ne...
base/base_untrusted.gyp:29: # does not look for libraries in system directories.
On 2013/01/03 18:51:35, Ryan Sleevi wrote:
> I don't think this comment is entirely accurate, since even if it did, using
the
> same config is not necessary correct. Compare prcpucfg_nacl.h vs
> prcpucfg_linux.h under base/third_party/nspr

It is - I've asked the nacl toolchain for searched library paths and compared
that with normal toolchain.

For prcpucfg_nacl.h vs. different things - I have no idea, I'm not changing
anything there.

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
File base/third_party/nspr/nspr.gyp (right):

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
base/third_party/nspr/nspr.gyp:7: ['use_system_nspr==1', {
On 2013/01/04 16:55:28, wtc wrote:
> 
> In base/base.gyp, you added a dependency on this .gyp file
> under the use_system_nspr==1 condition. So it seems
> redundant to check the use_system_nspr==1 condition here.

Actually, it's not! :)

Without this condition, gyp would execute command expansions below, which may
fail on systems which do not have nspr installed. Worse, I think it would
execute the commands on non-unix platforms, which don't have pkg-config.

Furthermore, command expansions would slow down gyp for everyone, for no reason
since use_system_nspr is not going to be used for Google Chrome.

In summary, this is a non-obvious consequence of how gyp works.

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
base/third_party/nspr/nspr.gyp:15: 'header_filenames': [
On 2013/01/03 18:51:35, Ryan Sleevi wrote:
> And why not prcpucfg.h?

Thanks, added.

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
base/third_party/nspr/nspr.gyp:15: 'header_filenames': [
On 2013/01/04 16:55:28, wtc wrote:
> I don't understand how this works. Could you explain why we
> need to list the headers but not the C/C++ source files?

Good question. See https://code.google.com/p/chromium/issues/detail?id=165264
for reference. Generally the idea is that when using system library we'd remove
all non-gyp files from the directory. To make the same #includes in .cc files
work, shim_headers.gypi uses a code generator to re-create the directory
structure in the build output directory and add forwarding headers there, which
just #include corresponding system headers. To let the generator know what paths
and file names to use (since the original files won't be there), we list them
here in variables.

Feel free to also take a look at shim_headers.gypi for reference. This solution
is intended to be transparent for people who work with the rest of the codebase.

[wtc, 2013-01-05 16:23:34]

I am happy with patch set 4. Please wait for the approval by
rsleevi.

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi#newcode732
base/base.gypi:732: ['exclude', '^third_party/nspr/'],

On 2013/01/04 18:01:31, Paweł Hajdan Jr. wrote:
>
> The conclusion is that no change is necessary here, since we always use
matching
> headers,

You're right. After I wrote my previous comment, I realized
my suggestion was wrong because we're always using matching
headers. Unfortunately I could not send you a new comment
until now.

> _and_ this is confirmed by the change working both for
> use_system_nspr=0 and 1. Otherwise it would fail to link.

Some compilers may not require extern "C" when it should be
used, so in general a successful build under one compiler
is not sufficient to confirm extern "C" is used correctly.
But I remember gcc requires proper use of extern "C".

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
File base/third_party/nspr/nspr.gyp (right):

https://codereview.chromium.org/11685002/diff/9001/base/third_party/nspr/nspr...
base/third_party/nspr/nspr.gyp:15: 'header_filenames': [

On 2013/01/04 18:01:31, Paweł Hajdan Jr. wrote:
>
> Feel free to also take a look at shim_headers.gypi for reference. This
solution
> is intended to be transparent for people who work with the rest of the
codebase.

Thank you for the explanation. This explains why this CL
doesn't need to update the
  #include "base/third_party/nspr/prtime.h"
line in base/time.cc.

https://codereview.chromium.org/11685002/diff/19001/base/third_party/nspr/nsp...
File base/third_party/nspr/nspr.gyp (right):

https://codereview.chromium.org/11685002/diff/19001/base/third_party/nspr/nsp...
base/third_party/nspr/nspr.gyp:16: 'prcpucfg.h',

Do you know why the use_system_nspr=1 build still succeeded
before you added prcpucfg.h here? It seems to show that we
can still use the bundled prcpucfg.h file if a shim header for
prcpucfg.h is not generated.

[Ryan Sleevi, 2013-01-07 00:04:10]

Mostly GYP nits and clarifications. I understand how this CL works now and -
more importantly - why it's working, but a comment below about how NACL is being
handled.

https://codereview.chromium.org/11685002/diff/9001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/11685002/diff/9001/base/base.gyp#newcode213
base/base.gyp:213: }],
On 2013/01/04 18:01:31, Paweł Hajdan Jr. wrote:
> On 2013/01/03 18:51:35, Ryan Sleevi wrote:
> > Why is this dependency not expressed in base.gypi, since that is what
handles
> > the inclusion of the NSPR sources?
> > 
> > I don't like the disconnect from having the dependency in one file, but the
> > sources directive that necessitates or obviates that dependency in another.
> 
> This is a problem I've hit multiple times with gyp. Generally having
> dependencies in .gypi files doesn't seem to work. Here is an example how gyp
> freaks out if I do that:
> 
> $ build/gyp_chromium -Duse_system_nspr=1
> Updating projects from gyp files...
> Traceback (most recent call last):
>   File "build/gyp_chromium", line 144, in <module>
>     sys.exit(gyp.main(args))
>   File
> "/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/__init__.py",
> line 524, in main
>     return gyp_main(args)
>   File
> "/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/__init__.py",
> line 509, in gyp_main
>     generator.GenerateOutput(flat_list, targets, data, params)
>   File
>
"/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/generator/make.py",
> line 2046, in GenerateOutput
>     for target in gyp.common.AllTargets(target_list, target_dicts,
build_file):
>   File
> "/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/common.py",
> line 302, in AllTargets
>     deptargets = DeepDependencyTargets(target_dicts, bftargets)
>   File
> "/usr/local/google/home/phajdan/chromium/src/tools/gyp/pylib/gyp/common.py",
> line 286, in DeepDependencyTargets
>     spec = target_dicts[r]
> KeyError: 'third_party/nspr/nspr.gyp:nspr'
> 
> Maybe I'm doing things wrong, so I'm happy to learn the correct way.

You cannot modify dependencies while evaluating a target_conditions block, which
is what of base.gypi is executing under, since the dependency evaluation has
already been computed.

My problem in general with this is that it's the GYP code-smell equivalent to
having an extern'd variable, since it splits up the dependency and
that-which-has-the-dependency.

Regrettably, however, it seems necessary here and consistent with the other
bits, so I suppose this will do.

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/11685002/diff/9001/base/base.gypi#newcode730
base/base.gypi:730: ['<(use_system_nspr)==1', {
On 2013/01/04 18:01:31, Paweł Hajdan Jr. wrote:
> On 2013/01/03 18:51:35, Ryan Sleevi wrote:
> > You're in a conditions block, do not use this variable syntax, since it's
> > implicit. (The same applies to the os_bsd and chromeos of lines 710/720,
> > respectively).
> > 
> > ['use_system_nspr == 1', [
> >   [ ... ]
> > ]
> 
> Nope. If I don't do that build/gyp_chromium bails out with:
> 
> gyp: name 'use_system_nspr' is not defined while evaluating condition
> 'use_system_nspr==1' in
> /usr/local/google/home/phajdan/chromium/src/base/base.gyp
> 
> I may be doing something wrong, in which case I'm happy to learn. However,
these
> two are clearly *not* equivalent (which may be a bug).

Ah, it's because it's within a target_conditions block, which is forcing a
deferral of the context of the variables. That's still likely a bug in GYP,
since they're supposed to be equivalent.

> 
> > Also, why do you not need to check that >(nacl_untrusted_build) == 0? When
> > inside a NACL untrusted build, you surely don't intend to link in all of
NSPR,
> > do you? If so, that should definitely get nacl & security-team review.
> 
> Note this conditional section excludes things, so it's not possible to drag
more
> things when excluding things.
> 
> By the way, linking to "outside world" is not possible or at least not easy in
> NACL untrusted build. This change certainly doesn't do that, and continues to
> use bundled prtime.cc for NaCl untrusted part.

I just think it's a bad idea not to be checking the >(nacl_untrusted_build) bit
here when removing. You can see in base_untrusted.gyp that it gets re-added
back, which I think is confusing compared to having the source files fully
enumerated within this GYP. 

If your condition was:
<(use_system_nspr)==1 and >(nacl_untrusted_build)==0

Then you wouldn't have to re-add the source back in base_untrusted

[Paweł Hajdan Jr., 2013-01-08 00:12:36]

PTAL

[Ryan Sleevi, 2013-01-08 00:14:31]

lgtm