Add support for reading .gypi files.

Add support for reading .gypi files.

Adds support for a new value type, a dictionary. To script it looks the same as a scope, but the existing scope didn't have the right memory management semantics for this.

Adds a new input conversion mode to interpret the result of a script as a GN block (basically an eval) and get the result in a dictionary. This allows returning named sets of things from a script. Updates the accessor and defined stuff to work with this new type.

Adds some features to the input file manager so that when we do dynamic reading from a script we keep the text of the read around which means we can give proper errors that refer to the script output, and don't have to do the weird recursive set origin thing to clear out the pointers that became invalid when the InputFile went out of scope.

This allows us to give better error messages when parsing input. The error handling in the input conversion code is changed accordingly.

Checks for unused variables both in code that instantiates a template, and in the template code itself.

Adds a shared python script for outputting values from Python to GN.

Adds a python script to read .gypi files and return the value as a GN scope.

BUG=
R=dpranke@chromium.org, scottmg@chromium.org

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

[brettw, 2014-04-03 18:32:36]

Dirk: Python code
Scott: C++ code

[scottmg, 2014-04-03 20:44:40]

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py
File build/gypi_to_gn.py (right):

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py#newc...
build/gypi_to_gn.py:51: ExceptionAppend(e, 'while reading ' + path)
this isn't imported from gn_helpers

https://codereview.chromium.org/223783005/diff/20001/tools/gn/value.h
File tools/gn/value.h (right):

https://codereview.chromium.org/223783005/diff/20001/tools/gn/value.h#newcode31
tools/gn/value.h:31: // scopes from files and we actually want to copy and own
the values. Both
Adding a second almost-identical seems pretty awkward. Doesn't scope have a copy
mode already that could be used instead?

[Dirk Pranke, 2014-04-03 22:01:28]

Here's a bunch of comments on the python side.

https://codereview.chromium.org/223783005/diff/20001/build/gn_helpers.py
File build/gn_helpers.py (right):

https://codereview.chromium.org/223783005/diff/20001/build/gn_helpers.py#newc...
build/gn_helpers.py:10: class Error(Exception):
"Error" is kind of a generic concept in python used to distinguish runtime
errors in the interpreter from user/script-level exceptions.

So, I wouldn't use the name "Error" here. I'd probably call it "GNException" or
something clever like that :). It is annoying to have to define your own
exceptions this way, but I don't know of a better alternative.

https://codereview.chromium.org/223783005/diff/20001/build/gn_helpers.py#newc...
build/gn_helpers.py:14: def ExceptionAppend(e, msg):
I think you only use this in one place in gypi_to_gn, right?

I tend to think of Exceptions as const objects, and wrap errors in other errors,
so I wouldn't encourage you to have this function at all (see the comment below
as to how I'd wrap one error in another).

https://codereview.chromium.org/223783005/diff/20001/build/gn_helpers.py#newc...
build/gn_helpers.py:27: raise Error("Argument to PrintList is not a Python
list.")
presumably you mean "Python string" here ...

https://codereview.chromium.org/223783005/diff/20001/build/gn_helpers.py#newc...
build/gn_helpers.py:30: sys.stdout.write('"' + value.replace('"', '\\"') + '"')
I would probably inline these functions into PrintValue.

https://codereview.chromium.org/223783005/diff/20001/build/gn_helpers.py#newc...
build/gn_helpers.py:41: sys.stdout.write(']')
assuming you change this to just return a string, the idiomatic way to do this
would be:

return '[%s]' % ', '.join(ToGNString(v) for v in value)

(i.e., one line).

https://codereview.chromium.org/223783005/diff/20001/build/gn_helpers.py#newc...
build/gn_helpers.py:51: sys.stdout.write('\n')
similarly this can be:

return '\n'.join(("%s = %s" % (k, ToGNString(v))) for k, v in value.items())

https://codereview.chromium.org/223783005/diff/20001/build/gn_helpers.py#newc...
build/gn_helpers.py:55: """Prints the given value to stdout.
I would probably separate the string conversion out from the printing, i.e.,
just have this function build up a string and return it, and then rename this to
something like ToGNString().

https://codereview.chromium.org/223783005/diff/20001/build/gn_helpers.py#newc...
build/gn_helpers.py:62: elif isinstance(value, list):
chromium python style prefers early exits where possible. This is even easier if
you change this to ToGNString(), e.g.:

if isinstance(value, str):
  s = <inlined PrintString computation>
  return s
if isinstance(value, list):
  return '[%s]' % ...
...
assert "unsupported type"

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py
File build/gypi_to_gn.py (right):

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py#newc...
build/gypi_to_gn.py:8: # Converts a given gypi file to a python scope and writes
the result to stdout.
It would probably be more Pythonic to make this all a multiline docstring (and
put it right above the import).

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py#newc...
build/gypi_to_gn.py:48: e.filename = path
overriding the filename here in the exception might be a bit wonky, but maybe
this is the right thing to do? (I hardly ever use eval() so I'm not sure what
this does. Perhaps it makes line numbers more meaningful.

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py#newc...
build/gypi_to_gn.py:51: ExceptionAppend(e, 'while reading ' + path)
Typically I would raise a new exception and include str(e) as part of the
message of my exception, something like:

raise Exception("Unexpected error while reading %s: %s" % (path, str(e))

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py#newc...
build/gypi_to_gn.py:51: ExceptionAppend(e, 'while reading ' + path)
On 2014/04/03 20:44:40, scottmg wrote:
> this isn't imported from gn_helpers

Right, "import gn_helpers" doesn't bring names into the namespace automatically
in python, you either need to say

from gn_helpers import Error, ExceptionAppend, ...

or prepend the module name, e.g.:

gn_helpers.ExceptionAppend(...)

Do not use "from gn_helpers import *" ... that's bad style and discouraged.

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py#newc...
build/gypi_to_gn.py:55: raise Error("%s does not evaluate to a dictionary" %
path)
Depending on how you except to handle these exceptions, you might just want to
use assertions here, e.g.:

assert isinstance(file_data, dict), "%s does not evaluation to a dictionary" %
path

and catch assertions down below.

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py#newc...
build/gypi_to_gn.py:71: gn_helpers.PrintValue(data)
as noted in gn_helpers, I'd probably change this to 'print
gn_helpers.ToGNValue(data)'

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py#newc...
build/gypi_to_gn.py:73: main()
The correct style for this would be to change line 73 to:

if __name__ == '__main__':
  main()

However, since the way you're dealing w/ errors above is to just raise
exceptions, you're probably better off catching the exception here so that you
can control what the script actually prints (an uncaught exception will result
in the python interpreter printing a stack trace, not terribly user friendly).

[brettw, 2014-04-03 23:19:24]

https://codereview.chromium.org/223783005/diff/20001/tools/gn/value.h
File tools/gn/value.h (right):

https://codereview.chromium.org/223783005/diff/20001/tools/gn/value.h#newcode31
tools/gn/value.h:31: // scopes from files and we actually want to copy and own
the values. Both
On 2014/04/03 20:44:40, scottmg wrote:
> Adding a second almost-identical seems pretty awkward. Doesn't scope have a
copy
> mode already that could be used instead?

I went around and around on this and at times had implemented it 3 different
ways.


Options:
1) Keep as-is with two confusingly similar modes.

2) Make the value always own a scope. This has the disadvantage that we have to
copy the values each time we instantiate a template (we'll have a lot of
templates, and some of them will be passing large file lists around). A bigger
disadvantage is that we'd lose the ability to track which variables were read
out of the scope by the template. This means we'd lose the ability to catch
misspellings of variables when you instantiate a template. Last, it has the
settings problem I discuss below.

3) Remove scope and keep the dict mode. Copy the values out of the scope into
the dict where we currently use scope. This has the same disadvantages as (2)
although I think it's more clear so would prefer this.

4) Have value able to either copy or reference a scope. Value gets a bit
confusing memory-management wise, but we get back the used variable tracking for
the template case, and also avoid copies. This has the settings problem.


The settings problem:

I started doing (4) at one point but gave up when I realized that I have a
problem because the input conversion executes the scope in the context of an
empty Settings object on the stack. The scope references this, so copying the
scope will resulting in pointers to destructed stack vars floating around.

I execute it in the context of an empty settings object to avoid potentially
confusing global state leaking into the scope when we're evaluating the value.
But upon further reflection the amount of information leakage is minimal to
none.

So I'm leaning to #4. What do you think?

[scottmg, 2014-04-03 23:22:39]

On 2014/04/03 23:19:24, brettw wrote:
> https://codereview.chromium.org/223783005/diff/20001/tools/gn/value.h
> File tools/gn/value.h (right):
> 
>
https://codereview.chromium.org/223783005/diff/20001/tools/gn/value.h#newcode31
> tools/gn/value.h:31: // scopes from files and we actually want to copy and own
> the values. Both
> On 2014/04/03 20:44:40, scottmg wrote:
> > Adding a second almost-identical seems pretty awkward. Doesn't scope have a
> copy
> > mode already that could be used instead?
> 
> I went around and around on this and at times had implemented it 3 different
> ways.
> 
> 
> Options:
> 1) Keep as-is with two confusingly similar modes.
> 
> 2) Make the value always own a scope. This has the disadvantage that we have
to
> copy the values each time we instantiate a template (we'll have a lot of
> templates, and some of them will be passing large file lists around). A bigger
> disadvantage is that we'd lose the ability to track which variables were read
> out of the scope by the template. This means we'd lose the ability to catch
> misspellings of variables when you instantiate a template. Last, it has the
> settings problem I discuss below.
> 
> 3) Remove scope and keep the dict mode. Copy the values out of the scope into
> the dict where we currently use scope. This has the same disadvantages as (2)
> although I think it's more clear so would prefer this.
> 
> 4) Have value able to either copy or reference a scope. Value gets a bit
> confusing memory-management wise, but we get back the used variable tracking
for
> the template case, and also avoid copies. This has the settings problem.
> 
> 
> The settings problem:
> 
> I started doing (4) at one point but gave up when I realized that I have a
> problem because the input conversion executes the scope in the context of an
> empty Settings object on the stack. The scope references this, so copying the
> scope will resulting in pointers to destructed stack vars floating around.
> 
> I execute it in the context of an empty settings object to avoid potentially
> confusing global state leaking into the scope when we're evaluating the value.
> But upon further reflection the amount of information leakage is minimal to
> none.
> 
> So I'm leaning to #4. What do you think?

Yeah, I was thinking #4, but I don't fully understand The Settings Problem. It
could be a non-stack empty Settings object instead?

[brettw, 2014-04-04 04:33:47]

On 2014/04/03 23:22:39, scottmg wrote:
> Yeah, I was thinking #4, but I don't fully understand The Settings Problem. It
> could be a non-stack empty Settings object instead?

Yeah but then I'd have to have some weird singleton. I was thinking most of the
default variables and the root scope come from the settings, but actually these
are hooked to the scope, so I think we'll be OK using the main settings object
and you won't have any variables defined in that scope. I'll try this tomorrow.

[brettw, 2014-04-04 20:04:40]

Dirk, this doesn't change any Python, will do a new patch soon.

Scott: PTAL C++. I found a way to have Value always own the pointer without
causing any extra copies. The result is a little extra trickyness in
termplate.cc but a much simpler Value class.

The new patch is much cleaner and works well.

[scottmg, 2014-04-04 20:15:46]

C++ lgtm with rewritten CL description.

https://codereview.chromium.org/223783005/diff/60001/tools/gn/template.cc
File tools/gn/template.cc (right):

https://codereview.chromium.org/223783005/diff/60001/tools/gn/template.cc#new...
tools/gn/template.cc:86: // This is a bit tricky because it's theoretically
possible for the template
Is there some way to make invoker readonly to avoid this?

[brettw, 2014-04-04 20:32:27]

Dirk: new Python up, PTAL

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py
File build/gypi_to_gn.py (right):

https://codereview.chromium.org/223783005/diff/20001/build/gypi_to_gn.py#newc...
build/gypi_to_gn.py:48: e.filename = path
Dunnow, I copied this from GYP and it seems to make sense.

[brettw, 2014-04-04 20:35:12]

https://codereview.chromium.org/223783005/diff/60001/tools/gn/template.cc
File tools/gn/template.cc (right):

https://codereview.chromium.org/223783005/diff/60001/tools/gn/template.cc#new...
tools/gn/template.cc:86: // This is a bit tricky because it's theoretically
possible for the template
On 2014/04/04 20:15:47, scottmg wrote:
> Is there some way to make invoker readonly to avoid this?

Nope, I don't have support for readonly variables. And I don't want to make it
C++ const because I want to track the used/unused variables in it.

[Dirk Pranke, 2014-04-04 23:01:11]

python code is much cleaner :).

I noted a couple of nits to clean up further, but otherwise the python stuff
lgtm.

https://codereview.chromium.org/223783005/diff/80001/build/gn_helpers.py
File build/gn_helpers.py (right):

https://codereview.chromium.org/223783005/diff/80001/build/gn_helpers.py#newc...
build/gn_helpers.py:7: import sys
nit: I don't think you need this import any more.

https://codereview.chromium.org/223783005/diff/80001/build/gn_helpers.py#newc...
build/gn_helpers.py:15: """Writes a python dictionary as a GN scope."""
nit: delete this function (no longer needed)?

[brettw, 2014-04-06 04:35:19]

Committed patchset #6 manually as r262045 (presubmit successful).