Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9422027:
frogpad now uses vm to bootstrap instead of node (Closed)
|
Created:
8 years, 10 months ago by mattsh Modified:
8 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptionfrogpad now uses vm to generate initial js
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=4408
Patch Set 1 #Patch Set 2 : fixed args #Patch Set 3 : small fixes #Patch Set 4 : fixed rebuild switch #Patch Set 5 : edits #Patch Set 6 : fixed arg named #
Total comments: 18
Patch Set 7 : code review fixes #Patch Set 8 : small fixes #
Total comments: 11
Patch Set 9 : small fixes #Messages
Total messages: 7 (0 generated)
This removes any dependency from frogpad on node.js. (We now use the dart vm to compile the version of frogpad.dart that runs in frogpad.html.)
https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... File tools/testing/frogpad/command.py (right): https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/command.py:31: """build up a command line string that can be used to start a process""" Capital letter, period. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/command.py:42: def RunCommand(command, args, output_file_path=None): RunCommand ==> run_command ? (Python style -- here and elsewhere in file) https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/command.py:61: if (output_file_path != None): parens not necessary here and below https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... File tools/testing/frogpad/frogpad.py (right): https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:139: self.frog_dir = os.path.abspath(os.path.join(self.frogpad_dir, "../../../frog")) line break needed, > 80 char Also the combination of os.path.join (slashes going \) and "../../.." may cause unexpected behavior on Windows. Not sure without testing there. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:142: self.dart_dir = os.path.abspath(os.path.join(self.frog_dir, "..")) Perhaps obtain location of the root of the dart source repo by using os.path.dirname a number of times, and then append "frog" to the end, and the folders to frogpad_dir. In other words, it feels cleaner if we hold the main root path, and then construct the other paths by adding on to them rather than going backward in paths with '../..' https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:190: # the dart program we're going to run on the dart vm Capital letter for first word, here, and elsewhere, and period. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:194: args.append(command.Arg("libdir", "%s/lib" % self.frog_dir, False)) is creating this command object really gain us anything over simply writing out "--out=frogpad.dart.frogc.js"? We can generate a list of these arguments and use subprocess.Popen instead of the deprecated os.popen on line 65 of command.py. Then the Arg class and _FormatCommand may not actually be needed. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:253: logging.debug("creating File '%s' (%d lines)" % (self.name, len(self.contents))) 80 char https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:288: frogpad.py --main=hello.dart Can we make hello.dart a required positional argument that occurs after frogpad.py? Like "frogpad.py hello.dart"? It seems like there wouldn't be any case where it would be acceptable to omit hello.dart
Thanks for quick review, replies below. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... File tools/testing/frogpad/command.py (right): https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/command.py:31: """build up a command line string that can be used to start a process""" On 2012/02/21 19:05:57, Emily Fortuna wrote: > Capital letter, period. Done. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/command.py:42: def RunCommand(command, args, output_file_path=None): On 2012/02/21 19:05:57, Emily Fortuna wrote: > RunCommand ==> run_command ? (Python style -- here and elsewhere in file) Done. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/command.py:61: if (output_file_path != None): On 2012/02/21 19:05:57, Emily Fortuna wrote: > parens not necessary here and below Done. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... File tools/testing/frogpad/frogpad.py (right): https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:139: self.frog_dir = os.path.abspath(os.path.join(self.frogpad_dir, "../../../frog")) On 2012/02/21 19:05:57, Emily Fortuna wrote: > line break needed, > 80 char > Also the combination of os.path.join (slashes going \) and "../../.." may cause > unexpected behavior on Windows. Not sure without testing there. Good point. Switches to use dirname now. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:142: self.dart_dir = os.path.abspath(os.path.join(self.frog_dir, "..")) On 2012/02/21 19:05:57, Emily Fortuna wrote: > Perhaps obtain location of the root of the dart source repo by using > os.path.dirname a number of times, and then append "frog" to the end, and the > folders to frogpad_dir. In other words, it feels cleaner if we hold the main > root path, and then construct the other paths by adding on to them rather than > going backward in paths with '../..' Done. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:190: # the dart program we're going to run on the dart vm On 2012/02/21 19:05:57, Emily Fortuna wrote: > Capital letter for first word, here, and elsewhere, and period. Done. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:194: args.append(command.Arg("libdir", "%s/lib" % self.frog_dir, False)) On 2012/02/21 19:05:57, Emily Fortuna wrote: > is creating this command object really gain us anything over simply writing out > "--out=frogpad.dart.frogc.js"? We can generate a list of these arguments and use > subprocess.Popen instead of the deprecated os.popen on line 65 of command.py. > Then the Arg class and _FormatCommand may not actually be needed. Good suggestion. command.Arg removed now. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:253: logging.debug("creating File '%s' (%d lines)" % (self.name, len(self.contents))) On 2012/02/21 19:05:57, Emily Fortuna wrote: > 80 char Done. https://chromiumcodereview.appspot.com/9422027/diff/3005/tools/testing/frogpa... tools/testing/frogpad/frogpad.py:288: frogpad.py --main=hello.dart On 2012/02/21 19:05:57, Emily Fortuna wrote: > Can we make hello.dart a required positional argument that occurs after > frogpad.py? Like "frogpad.py hello.dart"? It seems like there wouldn't be any > case where it would be acceptable to omit hello.dart Done.
lgtm https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... File tools/testing/frogpad/frogpad.py (right): https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:28: import logging nice use of logging :) https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:35: class Error(Exception): why not use StandardError? http://docs.python.org/library/exceptions.html https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:144: usage() I'd pull all of the option parsing out of Pad.__init__ and into main -- that way you can use OptionParser to print the help. https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:231: html = html.replace("{{FROGPAD_JS}}", FROGPAD_JS) Another way to do this would be to use "%(FROGPAD_JS)s" and "%(script_tags)s" in the string. Then these two lines become: return html % { 'FROGPAD_JS': FROGPAD_JS, 'script_tags': "".join(tags) } (that's a bit more along the conventional Python way of doing string formatting) https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:337: """) Perhaps use http://docs.python.org/library/optparse.html#generating-help ? That way your -m and -r options will print in the usage.
lgtm with John's comments!
https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... File tools/testing/frogpad/frogpad.py (right): https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:28: import logging On 2012/02/21 21:02:22, John Messerly wrote: > nice use of logging :) thanks https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:35: class Error(Exception): On 2012/02/21 21:02:22, John Messerly wrote: > why not use StandardError? > http://docs.python.org/library/exceptions.html Done. https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:144: usage() On 2012/02/21 21:02:22, John Messerly wrote: > I'd pull all of the option parsing out of Pad.__init__ and into main -- that way > you can use OptionParser to print the help. Yes, but let's do in a later CL if needed. It's slightly handy right now to have the options flags near where they are used if we are changing these much. https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:231: html = html.replace("{{FROGPAD_JS}}", FROGPAD_JS) On 2012/02/21 21:02:22, John Messerly wrote: > Another way to do this would be to use "%(FROGPAD_JS)s" and "%(script_tags)s" in > the string. Then these two lines become: > > return html % { 'FROGPAD_JS': FROGPAD_JS, 'script_tags': "".join(tags) } > > (that's a bit more along the conventional Python way of doing string formatting) I think if the source template has any extra %s or %d in it, then that would hit errors, so that's why it seems better to use a more distinctive pattern. https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:337: """) On 2012/02/21 21:02:22, John Messerly wrote: > Perhaps use http://docs.python.org/library/optparse.html#generating-help ? > That way your -m and -r options will print in the usage. Yes, reasonable for a later CL.
https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... File tools/testing/frogpad/frogpad.py (right): https://chromiumcodereview.appspot.com/9422027/diff/10002/tools/testing/frogp... tools/testing/frogpad/frogpad.py:337: """) On 2012/02/21 21:21:39, mattsh wrote: > On 2012/02/21 21:02:22, John Messerly wrote: > > Perhaps use http://docs.python.org/library/optparse.html#generating-help ? > > That way your -m and -r options will print in the usage. > > Yes, reasonable for a later CL. sgtm. It just occurred to me that I don't think any of our utilities has printed good help -- so it seems unfair to require this from frogpad. ;) |
