Allow chromium to be built with some system libs by introducing...

Allow chromium to be built with some system libs by introducing
a variable called SYSTEM_LIBS, consisting of a comma-separated
list of system libraries. So far, bzip2, libpng, libjpeg,
libxml, libxslt, lzma_sdk, zlib, hunspell, sqlite, libevent are recognized.

BUG=4321

[Evan Martin, 2008-11-12 01:20:14]

http://codereview.chromium.org/10626/diff/1/5
File build/SConscript.main (right):

http://codereview.chromium.org/10626/diff/1/5#newcode32
Line 32: # System libs
This comment isn't useful, as it's identical to the variable name.  It would be
useful to document what these variables are.

http://codereview.chromium.org/10626/diff/1/5#newcode102
Line 102: root_env['system_libs'] = ARGUMENTS.get('SYSTEM_LIBS')
Can you put a comment above this block of code describing what it's doing?

http://codereview.chromium.org/10626/diff/1/5#newcode104
Line 104: root_env['system_libs'] = root_env['system_libs'].split(',')
we use two-space tabs, which needs to be fixed here and elsewhere

http://codereview.chromium.org/10626/diff/1/5#newcode108
Line 108: def WantSystemLib(env, l):
Can you put a docstring on this?
We avoid one-letter variable names unless it's self-evident (like "i" in a
loop).

[Michael Moss, 2008-11-12 03:49:37]

http://codereview.chromium.org/10626/diff/1/5
File build/SConscript.main (right):

http://codereview.chromium.org/10626/diff/1/5#newcode33
Line 33: all_system_libs = [],
To add to Evan, describing the difference between these two very similarly named
vars would be good. Making the difference more self-evident in the names would
be even better.

http://codereview.chromium.org/10626/diff/1/5#newcode689
Line 689: be used from the system (not be built here):
This description is a little vague to me. Maybe something like "... system
libraries to link dynamically (by default they are built in from included
sources)"

[Evan Martin, 2008-11-12 21:42:26]

http://codereview.chromium.org/10626/diff/14/18
File build/SConscript.main (right):

http://codereview.chromium.org/10626/diff/14/18#newcode102
Line 102: # SYSTEM_LIBS variable
Can you put this comment up by where the variables are declared?  And note that
SYSTEM_LIBS comes from the command line, and describe all_system_libs as well.

Then the comment here can be something like "fill in req_system_libs from the
SYSTEM_LIBS command-line arg".

http://codereview.chromium.org/10626/diff/14/18#newcode103
Line 103: root_env['req_system_libs'] = ARGUMENTS.get('SYSTEM_LIBS')
You might be able to get away with
... = ARGUMENTS.get('SYSTEM_LIBS', '').split(',')
and eliminate the if/else below.

http://codereview.chromium.org/10626/diff/14/18#newcode112
Line 112: Also store its name into the 'all_system_libs' env variable so it
could later be
Can you make this fit in 80 columns?

[Evan Martin, 2008-11-12 22:15:10]

LGTM

Looks great.  Can you put yourself in the AUTHORS file once you've done the CLA?
 And I'll commit it.

[Michael Moss, 2008-11-12 22:17:37]

> http://codereview.chromium.org/10626/diff/14/18#newcode103
> Line 103: root_env['req_system_libs'] = ARGUMENTS.get('SYSTEM_LIBS')
> You might be able to get away with
> ... = ARGUMENTS.get('SYSTEM_LIBS', '').split(',')
> and eliminate the if/else below.

Does this really work? I think if 'SYSTEM_LIBS' isn't in ARGUMENTS, get()
returns None, which fails when you call split() on it.

[Evan Martin, 2008-11-12 22:21:09]

On 2008/11/12 22:17:37, mmoss wrote:
> > http://codereview.chromium.org/10626/diff/14/18#newcode103
> > Line 103: root_env['req_system_libs'] = ARGUMENTS.get('SYSTEM_LIBS')
> > You might be able to get away with
> > ... = ARGUMENTS.get('SYSTEM_LIBS', '').split(',')
> > and eliminate the if/else below.
> 
> Does this really work? I think if 'SYSTEM_LIBS' isn't in ARGUMENTS, get()
> returns None, which fails when you call split() on it.

The second argument to get() is the default fallback value.

[fta, 2008-11-12 23:01:16]

The same + AUTHORS