This changes variable expansion so that it is recursive

This changes variable expansion so that it is recursive.
For instance, now you can write <(foo <(bar)) and it will work
correctly.
I added a debugging switch for gyp so that we can start
it with different debugging modes to get different output.
Right now it just has "general" and "variables", but others can
easily be added (it just checks to see what's in the gyp.debug
set).  Call gyp.DebugOutput.
Also added a very rudimentary testing script.  It doesn't use
gtest or anything fancy, it just runs a test for variable expansions
with debugging output, and verifies that the debugging output
matches the golden files.  It uses gypd output to verify that
everything was parsed correctly.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=564

[Michael Moss, 2009-07-29 22:36:51]

You might want sgk or mark to evaluate the python-correctness, but in general,
LGTM, and thanks for adding the test cases.

[Greg Spencer, 2009-07-30 00:12:17]

On 2009/07/29 22:36:51, mmoss wrote:
> You might want sgk or mark to evaluate the python-correctness, but in general,
> LGTM, and thanks for adding the test cases.

OK, Added Mark and Steven to reviewers.  Guys, could one of you take a look?

[sgk, 2009-07-30 16:06:05]

lgtm

I'd be inclined to wrap the debug traces in a function instead of having to
hand-code each one with an "if 'variables" in block", but that's okay as a later
cleanup.

Re: test infrastructure, I'd say go ahead and check this in (so the underlying
problem is fixed) and I can adapt what you have when I land a gyptest.py
wrapper.

[Mark Mentovai, 2009-07-30 16:20:21]

LG for the most part, most of these are nitty.

http://codereview.chromium.org/159605/diff/1012/7
File pylib/gyp/__init__.py (right):

http://codereview.chromium.org/159605/diff/1012/7#newcode9
Line 9: import sets
imports were already sorted, keep them sorted, please

http://codereview.chromium.org/159605/diff/1012/7#newcode105
Line 105: action='append', default = [], help='add a mode to the '
We haven't been using space around = for arguments.

http://codereview.chromium.org/159605/diff/1012/7#newcode135
Line 135: print "GENERAL: running with these options: "
I'd think we'd want to route all debugging messages through a common function
instead of "print".

http://codereview.chromium.org/159605/diff/1012/10
File tests/run_tests.py (right):

http://codereview.chromium.org/159605/diff/1012/10#newcode1
Line 1: #!/usr/bin/python24
#!/usr/bin/env python

http://codereview.chromium.org/159605/diff/1012/10#newcode3
Line 3: import os
sort (for my OCD)

http://codereview.chromium.org/159605/diff/1012/10#newcode9
Line 9: p = subprocess.Popen(['python', '../gyp', '--depth', '..', test_name,
sys.executable

and

don't rely on the working directory being set to any specific value.  Instead,
work out the directory you want, it should be os.path.dirname(sys.argv[0])

http://codereview.chromium.org/159605/diff/1012/10#newcode11
Line 11: shell=True,
Do we really want shell=True?

http://codereview.chromium.org/159605/diff/1012/10#newcode15
Line 15: (p_stdout, p_stderr) = p.communicate("")
Why ""?

http://codereview.chromium.org/159605/diff/1012/10#newcode25
Line 25: if not gf:
Do you need this?  Won't open just raise before you'd ever hit this?

http://codereview.chromium.org/159605/diff/1012/10#newcode36
Line 36: for d in  difflib.context_diff(p_stdout, golden):
Extra space

http://codereview.chromium.org/159605/diff/1012/10#newcode51
Line 51: print "%d tests SUCCEEDED" % (len(tests) - failures)
sys.exit(nonzero)

http://codereview.chromium.org/159605/diff/1012/10#newcode53
Line 53: print "ALL %d tests SUCCEEDED" % len(tests)
sys.exit(zero)

[Greg Spencer, 2009-07-30 17:44:40]

Also, I just learned about "gypd" output format, so I changed the test so that
it runs with gypd as the output format so I can compare that output to a golden
file too.

http://codereview.chromium.org/159605/diff/1012/7
File pylib/gyp/__init__.py (right):

http://codereview.chromium.org/159605/diff/1012/7#newcode9
Line 9: import sets
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> imports were already sorted, keep them sorted, please

Done.

http://codereview.chromium.org/159605/diff/1012/7#newcode105
Line 105: action='append', default = [], help='add a mode to the '
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> We haven't been using space around = for arguments.

Sorry, missed that one.  Done.

http://codereview.chromium.org/159605/diff/1012/7#newcode135
Line 135: print "GENERAL: running with these options: "
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> I'd think we'd want to route all debugging messages through a common function
> instead of "print".

Yeah, sgk had a similar comment.  I've rerouted the debugging output through a
single function.

http://codereview.chromium.org/159605/diff/1012/10
File tests/run_tests.py (right):

http://codereview.chromium.org/159605/diff/1012/10#newcode1
Line 1: #!/usr/bin/python24
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> #!/usr/bin/env python

Done.

http://codereview.chromium.org/159605/diff/1012/10#newcode3
Line 3: import os
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> sort (for my OCD)

No, I should be sorting.  No OCD required.. :-)

http://codereview.chromium.org/159605/diff/1012/10#newcode9
Line 9: p = subprocess.Popen(['python', '../gyp', '--depth', '..', test_name,
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> sys.executable
> 
> and
> 
> don't rely on the working directory being set to any specific value.  Instead,
> work out the directory you want, it should be os.path.dirname(sys.argv[0])

Done.

http://codereview.chromium.org/159605/diff/1012/10#newcode11
Line 11: shell=True,
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> Do we really want shell=True?

Yes, on Windows (there are no executable equivalents to shell builtins, like
/bin/echo and /bin/pwd like there are on Unix).  I fixed it to only be true on
Windows.

http://codereview.chromium.org/159605/diff/1012/10#newcode15
Line 15: (p_stdout, p_stderr) = p.communicate("")
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> Why ""?

Because I was trying to debug a problem on Windows python (it was hanging
waiting for input when run under cygwin).  I'll remove it.

http://codereview.chromium.org/159605/diff/1012/10#newcode25
Line 25: if not gf:
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> Do you need this?  Won't open just raise before you'd ever hit this?

Yes, it will.  :-)  Fixed it by wrapping in try/except instead.

http://codereview.chromium.org/159605/diff/1012/10#newcode36
Line 36: for d in  difflib.context_diff(p_stdout, golden):
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> Extra space

Done.

http://codereview.chromium.org/159605/diff/1012/10#newcode51
Line 51: print "%d tests SUCCEEDED" % (len(tests) - failures)
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> sys.exit(nonzero)

Done.

http://codereview.chromium.org/159605/diff/1012/10#newcode53
Line 53: print "ALL %d tests SUCCEEDED" % len(tests)
On 2009/07/30 16:20:21, Mark Mentovai wrote:
> sys.exit(zero)

Done.

[Greg Spencer, 2009-07-30 17:45:29]

On 2009/07/30 16:06:05, sgk wrote:
> I'd be inclined to wrap the debug traces in a function instead of having to
> hand-code each one with an "if 'variables" in block", but that's okay as a
later
> cleanup.

Did that.  All output goes through gyp.DebugOutput now.

[Mark Mentovai, 2009-07-30 18:45:01]

lgtm

http://codereview.chromium.org/159605/diff/1016/1017
File pylib/gyp/__init__.py (right):

http://codereview.chromium.org/159605/diff/1016/1017#newcode18
Line 18: def DebugOutput(mode, message):
I would suggest that this probably belongs in gyp.common...
...but if it's used frequently enough, I can see it becoming a pain to have to
type |gyp.common.| instead of |gyp.|...
...so we can leave it here.

http://codereview.chromium.org/159605/diff/1016/1017#newcode114
Line 114: 'DEBUG GYP variable for debugging GYP.  Supported '
DEBUG isn't really a GYP variable in the sense of a 'variables' section, which
is usually what we mean when we say "GYP variable."  Can you revise this?

http://codereview.chromium.org/159605/diff/1016/1020
File tests/run_tests.py (right):

http://codereview.chromium.org/159605/diff/1016/1020#newcode78
Line 78: success_plural = "s"
Thanks for this!

http://codereview.chromium.org/159605/diff/1016/1020#newcode82
Line 82: print "ALL %d tests SUCCEEDED" % len(tests)
You could do the test%s thing here too.

[Greg Spencer, 2009-07-30 19:16:57]

http://codereview.chromium.org/159605/diff/1016/1017
File pylib/gyp/__init__.py (right):

http://codereview.chromium.org/159605/diff/1016/1017#newcode114
Line 114: 'DEBUG GYP variable for debugging GYP.  Supported '
On 2009/07/30 18:45:01, Mark Mentovai wrote:
> DEBUG isn't really a GYP variable in the sense of a 'variables' section, which
> is usually what we mean when we say "GYP variable."  Can you revise this?

Yeah, I don't know what I was thinking when I wrote that.  It really makes no
sense.  I revised it.

http://codereview.chromium.org/159605/diff/1016/1020
File tests/run_tests.py (right):

http://codereview.chromium.org/159605/diff/1016/1020#newcode82
Line 82: print "ALL %d tests SUCCEEDED" % len(tests)
On 2009/07/30 18:45:01, Mark Mentovai wrote:
> You could do the test%s thing here too.

Yeah, I tried that but then it says "ALL 1 test SUCCEEDED", which really is no
better than "ALL 1 tests SUCCEEDED".  I started to have it just say "The test
SUCCEEDED" for one test, but as soon as we add one more test, it'll never get
printed again, so I figured it didn't matter.

[Greg Spencer, 2009-07-30 22:46:06]

http://codereview.chromium.org/159605/diff/29/1026
File pylib/gyp/input.py (right):

http://codereview.chromium.org/159605/diff/29/1026#newcode409
Line 409: (p_stdout, p_stderr) = p.communicate('')
Can you guys take a quick look at this? -- during my testing on Linux and Mac I
had to do the above to get the "<!(['cmd', 'arg'])" execution form to work.

Also, on Linux and cygwin, I had to set the stdin to PIPE and then send it an
empty string before it would stop hanging waiting for input.

[Mark Mentovai, 2009-07-31 00:57:18]

gspencer@google.com wrote:
> http://codereview.chromium.org/159605/diff/29/1026
> File pylib/gyp/input.py (right):
>
> http://codereview.chromium.org/159605/diff/29/1026#newcode409
> Line 409: (p_stdout, p_stderr) = p.communicate('')
> Can you guys take a quick look at this? -- during my testing on Linux
> and Mac I had to do the above to get the "<!(['cmd', 'arg'])" execution
> form to work.
>
> Also, on Linux and cygwin, I had to set the stdin to PIPE and then send
> it an empty string before it would stop hanging waiting for input.

OK, we should take a look at your script then.  You shouldn't need to
specify stdin=PIPE or need to pass anything over stdin.  If the script
expects something on stdin now, it should probably be revised to not
do that.

Mark

[Mark Mentovai, 2009-07-31 02:58:02]

Interesting.  This may be specific to your Python version?  I can run
run_tests.py and have it pass without that stdin=3DPIPE line or
specifying any input.

Actually, the test fails because I have an include.gypi file, so the
diffs don't match:

- VARIABLES: Expanding '../../../../../.gyp/include.gypi' to
'../../../../../.gyp/include.gypi'

but this failure has nothing to do with Popen, and I'm not
experiencing any hangs at all in any case.

Python 2.5.1 on Mac OS X 10.5.7.  I'll build a 2.4 to see what happens
with that (I keep a 2.6 and a 3.0 around, I guess I should keep a 2.4
too.)

Anyway, it looks like you missed this review comment based on what got
checked in:

http://codereview.chromium.org/159605/diff/1012/10#newcode9

If you'd indulge me, would you mind running through to make sure that
you actually hit all of the rest of the little things like that?

Mark

Greg Spencer wrote:
> On Thu, Jul 30, 2009 at 5:56 PM, Mark Mentovai <mark@chromium.org> wrote:
>>
>> > Also, on Linux and cygwin, I had to set the stdin to PIPE and then sen=
d
>> > it an empty string before it would stop hanging waiting for input.
>>
>> OK, we should take a look at your script then. =A0You shouldn't need to
>> specify stdin=3DPIPE or need to pass anything over stdin. =A0If the scri=
pt
>> expects something on stdin now, it should probably be revised to not
>> do that.
>
> Well, in this case the entire "script" is: python -c "import math; print
> math.pi"
>
> so it's not expecting anything.=A0 And when gyp is being run from a DOS s=
hell,
> python doesn't hang (even when stdin isn't specified).
>
> I agree that it shouldn't be necessary.
>
> -Greg.
>

[Mark Mentovai, 2009-07-31 03:16:30]

I guess it's not Python version, I've had success with this now on Mac
with Python 2.4.6.

I also tried it out with 2.6.2 on Linux.  Even with no stdin=3DPIPE,
there's no hang:

VARIABLES: Expanding '["python", "-c", "<(pi)"]' to '["python", "-c",
"import math; print math.pi"]'
VARIABLES: Executing command '['python', '-c', 'import math; print math.pi'=
]'
VARIABLES: Expanding '<!(["python", "-c", "<(pi)"])' to '3.14159265359'

but when running under 2.6, we get a deprecation warning:

../pylib/gyp/__init__.py:7: DeprecationWarning: the sets module is deprecat=
ed
  import sets

This shows up on stderr, which is enough to cause the test to fail.

run_tests.py then prints stdout twice (oops!) instead of printing
stderr, so this was a little bit annoying to diagnose.  We should
probably check returncode in addition to stderr to determine failure.
Anyway, the point is, no hangs waiting on
(subprocess.Popen).communicate to return.

But like I said, it's late and I'm home now, so I'll help you pick
this back up again tomorrow.

Mark

I wrote:
> Interesting. =A0This may be specific to your Python version? =A0I can run
> run_tests.py and have it pass without that stdin=3DPIPE line or
> specifying any input.
>
> Actually, the test fails because I have an include.gypi file, so the
> diffs don't match:
>
> - VARIABLES: Expanding '../../../../../.gyp/include.gypi' to
> '../../../../../.gyp/include.gypi'
>
> but this failure has nothing to do with Popen, and I'm not
> experiencing any hangs at all in any case.
>
> Python 2.5.1 on Mac OS X 10.5.7. =A0I'll build a 2.4 to see what happens
> with that (I keep a 2.6 and a 3.0 around, I guess I should keep a 2.4
> too.)
>
> Anyway, it looks like you missed this review comment based on what got
> checked in:
>
> http://codereview.chromium.org/159605/diff/1012/10#newcode9
>
> If you'd indulge me, would you mind running through to make sure that
> you actually hit all of the rest of the little things like that?
>
> Mark
>
> Greg Spencer wrote:
>> On Thu, Jul 30, 2009 at 5:56 PM, Mark Mentovai <mark@chromium.org> wrote=
:
>>>
>>> > Also, on Linux and cygwin, I had to set the stdin to PIPE and then se=
nd
>>> > it an empty string before it would stop hanging waiting for input.
>>>
>>> OK, we should take a look at your script then. =A0You shouldn't need to
>>> specify stdin=3DPIPE or need to pass anything over stdin. =A0If the scr=
ipt
>>> expects something on stdin now, it should probably be revised to not
>>> do that.
>>
>> Well, in this case the entire "script" is: python -c "import math; print
>> math.pi"
>>
>> so it's not expecting anything.=A0 And when gyp is being run from a DOS =
shell,
>> python doesn't hang (even when stdin isn't specified).
>>
>> I agree that it shouldn't be necessary.
>>
>> -Greg.
>>
>

[Mark Mentovai, 2009-07-31 14:13:06]

Greg Spencer wrote:
>> Anyway, it looks like you missed this review comment based on what got
>> checked in:
>>
>> http://codereview.chromium.org/159605/diff/1012/10#newcode9
>>
>> If you'd indulge me, would you mind running through to make sure that
>> you actually hit all of the rest of the little things like that?
>
> Hmm.=A0 I did fix that (at least if you're talking about the path issue -=
- I'm
> not sure which one that link goes to).=A0=A0 In the checked in version I =
have:
>
> =A0 gyp_script =3D os.path.normpath(
> =A0=A0=A0 os.path.join(os.path.dirname(sys.argv[0]), '../gyp'))
>
> =A0 p =3D subprocess.Popen(['python', gyp_script, '--depth', '..', test_n=
ame,
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 '--=
debug', 'variables', '--debug', 'general',
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 '--=
format', 'gypd'],
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 shell=
=3D(sys.platform =3D=3D 'win32'),
> stdin=3Dsubprocess.PIPE,
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 stdout=
=3Dsubprocess.PIPE, stderr=3Dsubprocess.PIPE)

I'm talking about the first part of my comment - you should use
sys.executable instead of a hardcoded 'python'.

Now that we're scrutinizing, though, you should also split '..' and
'gyp' into separate arguments to os.path.join, or do
join(dirname(dirname(argv[0])), 'gyp')

Mark