Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
896663002:
gyp_chromium: Better parsing of -G command line flag. (Closed)
Descriptiongyp_chromium: Better parsing of -G command line flag.
Handle -G output_dir=foo as well as -Goutput_dir=foo.
I'm not sure how common this case is but we have
NaCl SDK bot that do this.
Getting this wrong results in windows toolchain being
installed in 'out/' (which doesn't necessarily exist)
rather than the output_dir tree and gyp uses.
Add unittests for the affected parts of gyp_chromium.
BUG=454594
Committed: https://crrev.com/10ef1e2aa8ebb806ccfe7c52235f0fb76564ee2c
Cr-Commit-Position: refs/heads/master@{#314381}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 2
Messages
Total messages: 27 (8 generated)
sbc@chromium.org changed reviewers: + jamesr@chromium.org
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
https://codereview.chromium.org/896663002/diff/20001/build/gyp_chromium File build/gyp_chromium (right): https://codereview.chromium.org/896663002/diff/20001/build/gyp_chromium#newco... build/gyp_chromium:145: # want to ignore all arguments other than "-G". I'm a little skeptical of this comment (though since you cc'd me, maybe I was the author of it...) Since the function's getting a bit long, could you do something like http://stackoverflow.com/questions/1885161/how-can-i-get-optparses-optionpars... instead to more closely match what gyp is going to do?
jamesr@chromium.org changed reviewers: - scottmg@chromium.org
jamesr@chromium.org changed reviewers: + scottmg@chromium.org - jamesr@chromium.org
scottmg@ is the reviewer you want
On 2015/02/03 00:21:32, scottmg wrote: > https://codereview.chromium.org/896663002/diff/20001/build/gyp_chromium > File build/gyp_chromium (right): > > https://codereview.chromium.org/896663002/diff/20001/build/gyp_chromium#newco... > build/gyp_chromium:145: # want to ignore all arguments other than "-G". > I'm a little skeptical of this comment (though since you cc'd me, maybe I was > the author of it...) I think it was jamesr who wrote this but I noticed it because your code uses it to install the VS toolchain. > > Since the function's getting a bit long, could you do something like > http://stackoverflow.com/questions/1885161/how-can-i-get-optparses-optionpars... > instead to more closely match what gyp is going to do? Good point. Do you know if we can 100% rely on python2.7 on all platforms these days? If so it looks like argparse can do this trivially: http://stackoverflow.com/questions/12818146/python-argparse-ignore-unrecognis...
I'm pretty sure I didn't write that comment (although I'm not 100% sure of that, I forget lots of things)
On 2015/02/03 00:46:17, Sam Clegg wrote: > On 2015/02/03 00:21:32, scottmg wrote: > > https://codereview.chromium.org/896663002/diff/20001/build/gyp_chromium > > File build/gyp_chromium (right): > > > > > https://codereview.chromium.org/896663002/diff/20001/build/gyp_chromium#newco... > > build/gyp_chromium:145: # want to ignore all arguments other than "-G". > > I'm a little skeptical of this comment (though since you cc'd me, maybe I was > > the author of it...) > > I think it was jamesr who wrote this but I noticed it because your code uses it > to install the VS toolchain. > > > > > Since the function's getting a bit long, could you do something like > > > http://stackoverflow.com/questions/1885161/how-can-i-get-optparses-optionpars... > > instead to more closely match what gyp is going to do? > > Good point. > > Do you know if we can 100% rely on python2.7 on all platforms these days? If so > it looks like argparse can do this trivially: > > http://stackoverflow.com/questions/12818146/python-argparse-ignore-unrecognis... Yes, argparse sgtm. AFAIK we've changed everything everywhere to 2.7. Slightly different since gyp isn't 2.7-dependent but shouldn't affect this usage.
On 2015/02/03 00:48:05, jamesr wrote: > I'm pretty sure I didn't write that comment (although I'm not 100% sure of that, > I forget lots of things) It looked authoritative at the time! :) https://codereview.chromium.org/142223002/diff/190001/build/gyp_chromium I swear I forget code I wrote less than a month ago.
jamesr@chromium.org changed reviewers: + jamesr@chromium.org
2014 jamesr was basically a different human - it doesn't count!
Looks like that comment was cargo culted from the GetGypVars().. original author was brettw :) https://codereview.chromium.org/123463005 Updated both locations to use parse_known_args(). Yay for python 2.7. https://codereview.chromium.org/896663002/diff/20001/build/gyp_chromium File build/gyp_chromium (right): https://codereview.chromium.org/896663002/diff/20001/build/gyp_chromium#newco... build/gyp_chromium:145: # want to ignore all arguments other than "-G". On 2015/02/03 00:21:32, scottmg wrote: > I'm a little skeptical of this comment (though since you cc'd me, maybe I was > the author of it...) > > Since the function's getting a bit long, could you do something like > http://stackoverflow.com/questions/1885161/how-can-i-get-optparses-optionpars... > instead to more closely match what gyp is going to do? Done.
lgtm https://codereview.chromium.org/896663002/diff/60001/build/gyp_chromium File build/gyp_chromium (right): https://codereview.chromium.org/896663002/diff/60001/build/gyp_chromium#newco... build/gyp_chromium:141: # handle command line generator flags nit; capital H, end with '.'. https://codereview.chromium.org/896663002/diff/60001/build/gyp_chromium#newco... build/gyp_chromium:146: # handle generator flags from the environment same
I decided to add some unittests here for peace of mind (is that crazy?). If anything blows up due to this change we can add more tests in response. https://codereview.chromium.org/896663002/diff/60001/build/gyp_chromium File build/gyp_chromium (right): https://codereview.chromium.org/896663002/diff/60001/build/gyp_chromium#newco... build/gyp_chromium:141: # handle command line generator flags On 2015/02/03 01:26:45, scottmg wrote: > nit; capital H, end with '.'. Done. https://codereview.chromium.org/896663002/diff/60001/build/gyp_chromium#newco... build/gyp_chromium:146: # handle generator flags from the environment On 2015/02/03 01:26:45, scottmg wrote: > same Done.
nit; "effected" in cl description should be "affected". On 2015/02/03 02:04:13, Sam Clegg wrote: > I decided to add some unittests here for peace of mind (is that crazy?). If > anything blows up due to this change we can add more tests in response. Super, thanks! Tests LGTM. > > https://codereview.chromium.org/896663002/diff/60001/build/gyp_chromium > File build/gyp_chromium (right): > > https://codereview.chromium.org/896663002/diff/60001/build/gyp_chromium#newco... > build/gyp_chromium:141: # handle command line generator flags > On 2015/02/03 01:26:45, scottmg wrote: > > nit; capital H, end with '.'. > > Done. > > https://codereview.chromium.org/896663002/diff/60001/build/gyp_chromium#newco... > build/gyp_chromium:146: # handle generator flags from the environment > On 2015/02/03 01:26:45, scottmg wrote: > > same > > Done.
https://codereview.chromium.org/896663002/diff/80001/build/gyp_chromium_test.py File build/gyp_chromium_test.py (right): https://codereview.chromium.org/896663002/diff/80001/build/gyp_chromium_test.... build/gyp_chromium_test.py:19: gyp_chromium = __import__('gyp_chromium') there is a .py because it's required on windows for multiprocessing... i guess not sufficient to import here instead?
https://codereview.chromium.org/896663002/diff/80001/build/gyp_chromium_test.py File build/gyp_chromium_test.py (right): https://codereview.chromium.org/896663002/diff/80001/build/gyp_chromium_test.... build/gyp_chromium_test.py:19: gyp_chromium = __import__('gyp_chromium') On 2015/02/03 02:09:18, scottmg wrote: > there is a .py because it's required on windows for multiprocessing... i guess > not sufficient to import here instead? Oh yeah! That is funny/strange. Not importable, but we should certainly clean that up in future.
The CQ bit was checked by sbc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/896663002/80001
The CQ bit was unchecked by sbc@chromium.org
The CQ bit was checked by sbc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/896663002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/10ef1e2aa8ebb806ccfe7c52235f0fb76564ee2c Cr-Commit-Position: refs/heads/master@{#314381} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
