Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2138693002:
[tools] Build generator script (Closed)
|
Created:
4 years, 5 months ago by Michael Achenbach Modified:
4 years, 5 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[tools] Build generator script
Helper script to generate gn arguments based on common
developer defaults or builder configurations.
BUG=chromium:625791
NOTRY=true
Committed: https://crrev.com/7f07809c2c91d7000a4f957df5e804611444def3
Cr-Commit-Position: refs/heads/master@{#37947}
Patch Set 1 #Patch Set 2 : [tools] Build generator script #Patch Set 3 : Sanification #
Total comments: 2
Patch Set 4 : Updates - workdir finder and comments #Patch Set 5 : goma detection + optional gn gen #
Total comments: 20
Patch Set 6 : Review #
Total comments: 7
Patch Set 7 : Review + better docu #Messages
Total messages: 29 (8 generated)
machenbach@chromium.org changed reviewers: + jochen@chromium.org, vogelheim@chromium.org
FYI quick-n-dirty showcase how using the mb_config.pyl could look like. Will work more on this to make it a real prototype. Or add it to Daniel's prototype.
Patch 3 is a ready-to-use prototype.
https://codereview.chromium.org/2138693002/diff/40001/tools/tool.py File tools/tool.py (right): https://codereview.chromium.org/2138693002/diff/40001/tools/tool.py#newcode141 tools/tool.py:141: self.call_cmd(['gn', 'gen', gn_outdir]) i'd be happier without this call... but otherwise, lgtm
Btw: I also need a name and location for the tool. I don't like adding more tools into the toplevel tools directory. I wonder if we should have a separate directory for a set of tools that folks can add to their PATH? @Daniel: Would this script integrate into the workflow of your script? I.e. could we wrap it easily as the 'v8 gen' part of your script? Or is there anything in the approach not compatible? https://codereview.chromium.org/2138693002/diff/40001/tools/tool.py File tools/tool.py (right): https://codereview.chromium.org/2138693002/diff/40001/tools/tool.py#newcode141 tools/tool.py:141: self.call_cmd(['gn', 'gen', gn_outdir]) On 2016/07/12 15:09:07, jochen wrote: > i'd be happier without this call... but otherwise, lgtm I thought it might be better to warn for potential errors early. E.g. a simple typo in the additional gn args. Otherwise, the used'd see it when calling ninja - is that acceptable? Then I could make this optional.
On 2016/07/13 at 07:35:19, machenbach wrote: > Btw: I also need a name and location for the tool. I don't like adding more tools into the toplevel tools directory. I wonder if we should have a separate directory for a set of tools that folks can add to their PATH? > > @Daniel: Would this script integrate into the workflow of your script? I.e. could we wrap it easily as the 'v8 gen' part of your script? Or is there anything in the approach not compatible? > > https://codereview.chromium.org/2138693002/diff/40001/tools/tool.py > File tools/tool.py (right): > > https://codereview.chromium.org/2138693002/diff/40001/tools/tool.py#newcode141 > tools/tool.py:141: self.call_cmd(['gn', 'gen', gn_outdir]) > On 2016/07/12 15:09:07, jochen wrote: > > i'd be happier without this call... but otherwise, lgtm > > I thought it might be better to warn for potential errors early. E.g. a simple typo in the additional gn args. Otherwise, the used'd see it when calling ninja - is that acceptable? Then I could make this optional. I guess it's acceptable
On 2016/07/13 15:20:44, jochen wrote: > On 2016/07/13 at 07:35:19, machenbach wrote: > > Btw: I also need a name and location for the tool. I don't like adding more > tools into the toplevel tools directory. I wonder if we should have a separate > directory for a set of tools that folks can add to their PATH? > > > > @Daniel: Would this script integrate into the workflow of your script? I.e. > could we wrap it easily as the 'v8 gen' part of your script? Or is there > anything in the approach not compatible? > > > > https://codereview.chromium.org/2138693002/diff/40001/tools/tool.py > > File tools/tool.py (right): > > > > https://codereview.chromium.org/2138693002/diff/40001/tools/tool.py#newcode141 > > tools/tool.py:141: self.call_cmd(['gn', 'gen', gn_outdir]) > > On 2016/07/12 15:09:07, jochen wrote: > > > i'd be happier without this call... but otherwise, lgtm > > > > I thought it might be better to warn for potential errors early. E.g. a simple > typo in the additional gn args. Otherwise, the used'd see it when calling ninja > - is that acceptable? Then I could make this optional. > > I guess it's acceptable Patches 3 and 4 add some more features. Auto detect of workdir and goma. Also optional gn execution.
machenbach@chromium.org changed reviewers: + tandrii@chromium.org
+ tandrii for Python readability.
https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py File tools/dev/v8gen.py (right): https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:51: self.gn_args = args[index + 1:] here and also for self.options: since you are marking protected functions with underscores, why not do the same with attributes as s/self.gn_args/self._gn_args https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:53: parser = argparse.ArgumentParser( i'd put all code from here to end of __init__ into its own func parse_arguments(), perhaps in module level, to make it easy to see what local attrs are in GenerateGnArgs. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:86: assert (self.options.outdir or self.options.builder), ( if ...: parser.error(...) # quits with ret code 2 here. will result in much better UX https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:97: 'only output directories relative to %s are supported' % OUT_DIR) ditto, use parser.error() https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:105: print '#################################################################' ah, so verbose_print_1 means a section title? then '#' * 80 is cooler https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:110: print text after reading the whole class, I see that print-1 includes things like "cd xxx". In which case, maybe don't use '#' * 80 above, but instead indent the text here with: indent = ' ' * 4 for l in text.splitlines(): print indent + l end result would be easier to read just verbose level=1 (-v) and still readable "-v -v" mode. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:125: """Find the closest v8 root to 'path'.""" nit: I think `path` is proper way to mark vars, but just path is also OK. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:159: not [x for x in self.gn_args if re.match(r'use_goma\s*=.*', x)]): maybe all(not re.match(r'use_goma\s*=.*', x) for x in self.gn_args) or not any(re.match(r'use_goma\s*=.*', x) for x in self.gn_args) https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:171: self.verbose_print_1("Appending \"\"\"\n%s\n\"\"\" to %s." % ( use singular quotes and string would be easier to read.
Done. PTAL at patch 6 https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py File tools/dev/v8gen.py (right): https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:51: self.gn_args = args[index + 1:] On 2016/07/14 12:45:28, tandrii(chromium) wrote: > here and also for self.options: since you are marking protected functions with > underscores, why not do the same with attributes as s/self.gn_args/self._gn_args Done. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:53: parser = argparse.ArgumentParser( On 2016/07/14 12:45:28, tandrii(chromium) wrote: > i'd put all code from here to end of __init__ into its own func > parse_arguments(), perhaps in module level, to make it easy to see what local > attrs are in GenerateGnArgs. Done. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:86: assert (self.options.outdir or self.options.builder), ( On 2016/07/14 12:45:29, tandrii(chromium) wrote: > if ...: > parser.error(...) # quits with ret code 2 here. > > will result in much better UX Done. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:97: 'only output directories relative to %s are supported' % OUT_DIR) On 2016/07/14 12:45:29, tandrii(chromium) wrote: > ditto, use parser.error() Done. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:105: print '#################################################################' On 2016/07/14 12:45:29, tandrii(chromium) wrote: > ah, so verbose_print_1 means a section title? > then '#' * 80 is cooler > Done. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:110: print text On 2016/07/14 12:45:29, tandrii(chromium) wrote: > after reading the whole class, I see that print-1 includes things like "cd xxx". > In which case, maybe don't use '#' * 80 above, but instead indent the text here > with: > indent = ' ' * 4 > for l in text.splitlines(): > print indent + l > > end result would be easier to read just verbose level=1 (-v) and still readable > "-v -v" mode. Done. But keeping the ### as it is even more readable. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:125: """Find the closest v8 root to 'path'.""" On 2016/07/14 12:45:29, tandrii(chromium) wrote: > nit: I think `path` is proper way to mark vars, but just path is also OK. Done. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:159: not [x for x in self.gn_args if re.match(r'use_goma\s*=.*', x)]): On 2016/07/14 12:45:29, tandrii(chromium) wrote: > maybe > > all(not re.match(r'use_goma\s*=.*', x) for x in self.gn_args) > > or > > not any(re.match(r'use_goma\s*=.*', x) for x in self.gn_args) Done. https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:171: self.verbose_print_1("Appending \"\"\"\n%s\n\"\"\" to %s." % ( On 2016/07/14 12:45:28, tandrii(chromium) wrote: > use singular quotes and string would be easier to read. Weird, I'm using single quotes in the rest of the file, except here were it would have mattered most :)
lgtm https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py File tools/dev/v8gen.py (right): https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:171: self.verbose_print_1("Appending \"\"\"\n%s\n\"\"\" to %s." % ( On 2016/07/15 14:18:09, Michael Achenbach wrote: > On 2016/07/14 12:45:28, tandrii(chromium) wrote: > > use singular quotes and string would be easier to read. > > Weird, I'm using single quotes in the rest of the file, except here were it > would have mattered most :) yep, i wondered too :) https://codereview.chromium.org/2138693002/diff/80001/tools/dev/v8gen.py#newc... tools/dev/v8gen.py:204: more_gn_args = '\n'.join(self.gn_args) good, i didn't notice.
@jochen,vogelheim: maybe take another look at patch 6?
still lgtm https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py File tools/dev/v8gen.py (right): https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py#new... tools/dev/v8gen.py:15: v8gen.py -b x64.release foo shouldn't that be something like -b 'V8 Linux64 - builder'? https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py#new... tools/dev/v8gen.py:18: v8gen.py x64.optdebug -- use_goma=true use_goma would be -g - maybe use another example here?
lgtm https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py File tools/dev/v8gen.py (right): https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py#new... tools/dev/v8gen.py:42: return ''.join(c if (c.isalnum() or c == '.') else '_' for c in text) I think I'd find an re-based version more readable. Matter of taste, though. return re.sub(r'[^a-zA-Z0-9.]', '_', text)
any eta for landing? :)
Patch 7 addresses review comments and tidies the documentation. The script now reuses the python doc comment as help output. Tried to make the doc comment more concise and complete. https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py File tools/dev/v8gen.py (right): https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py#new... tools/dev/v8gen.py:15: v8gen.py -b x64.release foo On 2016/07/18 11:28:29, jochen wrote: > shouldn't that be something like -b 'V8 Linux64 - builder'? No. All our standard configs have their own shortcuts. In most cases they are equal to the respective bots, but sometimes the bots do a few more things, e.g. additional things to pack in swarming isolates. https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py#new... tools/dev/v8gen.py:18: v8gen.py x64.optdebug -- use_goma=true On 2016/07/18 11:28:29, jochen wrote: > use_goma would be -g - maybe use another example here? I'll update this example with another flag than goma and add extra goma examples below. https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py#new... tools/dev/v8gen.py:42: return ''.join(c if (c.isalnum() or c == '.') else '_' for c in text) On 2016/07/18 18:28:26, vogelheim wrote: > I think I'd find an re-based version more readable. Matter of taste, though. > > return re.sub(r'[^a-zA-Z0-9.]', '_', text) Not sure if I understand what you mean by re-based here. I added your suggestion.
Landing now as is - will rework more in follow ups if necessary.
Description was changed from ========== [tools] Build generator script BUG= ========== to ========== [tools] Build generator script BUG=chromium:625791 NOTRY=true ==========
Description was changed from ========== [tools] Build generator script BUG=chromium:625791 NOTRY=true ========== to ========== [tools] Build generator script Helper script to generate gn arguments based on common developer defaults or builder configurations. BUG=chromium:625791 NOTRY=true ==========
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, vogelheim@chromium.org, tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/2138693002/#ps120001 (title: "Review + better docu")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py File tools/dev/v8gen.py (right): https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py#new... tools/dev/v8gen.py:42: return ''.join(c if (c.isalnum() or c == '.') else '_' for c in text) On 2016/07/21 13:13:29, Michael Achenbach (slow) wrote: > On 2016/07/18 18:28:26, vogelheim wrote: > > I think I'd find an re-based version more readable. Matter of taste, though. > > > > return re.sub(r'[^a-zA-Z0-9.]', '_', text) > > Not sure if I understand what you mean by re-based here. I added your > suggestion. Sorry for being unclear. What I meant: Based on regular expressions / the 're' module.
Message was sent while issue was closed.
Description was changed from ========== [tools] Build generator script Helper script to generate gn arguments based on common developer defaults or builder configurations. BUG=chromium:625791 NOTRY=true ========== to ========== [tools] Build generator script Helper script to generate gn arguments based on common developer defaults or builder configurations. BUG=chromium:625791 NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
On 2016/07/21 13:19:54, vogelheim wrote: > https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py > File tools/dev/v8gen.py (right): > > https://codereview.chromium.org/2138693002/diff/100001/tools/dev/v8gen.py#new... > tools/dev/v8gen.py:42: return ''.join(c if (c.isalnum() or c == '.') else '_' > for c in text) > On 2016/07/21 13:13:29, Michael Achenbach (slow) wrote: > > On 2016/07/18 18:28:26, vogelheim wrote: > > > I think I'd find an re-based version more readable. Matter of taste, though. > > > > > > return re.sub(r'[^a-zA-Z0-9.]', '_', text) > > > > Not sure if I understand what you mean by re-based here. I added your > > suggestion. > > Sorry for being unclear. > > What I meant: Based on regular expressions / the 're' module. Ha - nice. My brain didn't parse the dash :)
Message was sent while issue was closed.
Description was changed from ========== [tools] Build generator script Helper script to generate gn arguments based on common developer defaults or builder configurations. BUG=chromium:625791 NOTRY=true ========== to ========== [tools] Build generator script Helper script to generate gn arguments based on common developer defaults or builder configurations. BUG=chromium:625791 NOTRY=true Committed: https://crrev.com/7f07809c2c91d7000a4f957df5e804611444def3 Cr-Commit-Position: refs/heads/master@{#37947} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7f07809c2c91d7000a4f957df5e804611444def3 Cr-Commit-Position: refs/heads/master@{#37947} |
