Bug 63872 - Add trivial garden-o-matic command (with server)
Summary: Add trivial garden-o-matic command (with server)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-03 04:36 PDT by Adam Barth
Modified: 2011-07-05 15:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (17.11 KB, patch)
2011-07-03 04:38 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (14.26 KB, patch)
2011-07-05 15:26 PDT, Adam Barth
eric: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-07-03 04:36:56 PDT
Add trivial garden-o-matic command (with server)
Comment 1 Adam Barth 2011-07-03 04:38:50 PDT
Created attachment 99571 [details]
Patch
Comment 2 James Robinson 2011-07-03 21:39:17 PDT
Comment on attachment 99571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99571&action=review

> Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:1
> +# Copyright (C) 2010 Google Inc. All rights reserved.

I heard an awful rumor that it's 2011 now.  I also heard a less awful rumor that we're using the 2-clause license nowadays (like http://trac.webkit.org/browser/trunk/Source/WebCore/LICENSE-APPLE, but with only one year in the copyright line and "Google Inc" instead of "Apple Inc" on the first line).
Comment 3 Adam Barth 2011-07-03 22:22:58 PDT
For realz?  Ok.  I'll fix it on landing.
Comment 4 Eric Seidel (no email) 2011-07-04 10:09:50 PDT
Comment on attachment 99571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99571&action=review

> Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:35
> +class AbstractServerCommand(AbstractDeclarativeCommand):

I would have called this AbstractLocalServerCommand or AbstractLocalHTTPServerCommand (yes, both are rather large mouthfuls)

> Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:45
> +    def _prepare_config(self, options, args, tool):
> +        return None

So this is like _prepare_state for AbstractSequenceCommand?

> Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:50
> +        server_url = "http://localhost:%d/" % options.httpd_port

Could default to None and use a random port above 8000 when not specified.

> Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:55
> +        # FIXME: This seems racy.
> +        threading.Timer(0.1, lambda: self._tool.user.open_url(server_url)).start()

Yes, quite racy.  The http server for DRT has an "is it up" method which polls in a loop.

> Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:57
> +        httpd = self.server(httpd_port=options.httpd_port, config=config)

You could **config the config to pass them as keyword arguments intead of as a dictionary.  I'm not sure if that's more clear or not.

> Tools/Scripts/webkitpy/tool/commands/options.py:33
> +    httpd_port = make_option("--httpd-port", action="store", type="int", default=8127, help="Port to use for the HTTP server")

Not sure the default is very helpful.  See above.

I like that we're maing an Options class for commands.  Might be a little premature though.  I wonder how many other commands have options.

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:-84
> -        if options.dry_run:
> -
> -            def no_op_copyfile(src, dest):
> -                pass
> -
> -            def no_op_add(path, return_exit_code=False):
> -                if return_exit_code:
> -                    return 0
> -
> -            filesystem.copyfile = no_op_copyfile
> -            scm.add = no_op_add
> -

So you're just removing this?  Seems these could easily use the mock versions.

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:89
> +        return {

Always feels a bit odd returning a dictionary.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:37
> +        BaseHTTPServer.HTTPServer.__init__(self, ("", httpd_port), GardeningHTTPRequestHandler)

What's the "", port tuple?

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:45
> +    STATIC_FILE_DIRECTORY = os.path.join(os.path.dirname(__file__), "data", "gardeningserver")

Do you want this to always be relative to __file__?  If you use filesystem.path_for_module you can get a mockable value for __file__.
Comment 5 Adam Barth 2011-07-04 11:09:58 PDT
(In reply to comment #4)
> (From update of attachment 99571 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99571&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:35
> > +class AbstractServerCommand(AbstractDeclarativeCommand):
> 
> I would have called this AbstractLocalServerCommand or AbstractLocalHTTPServerCommand (yes, both are rather large mouthfuls)

Ok.

> > Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:45
> > +    def _prepare_config(self, options, args, tool):
> > +        return None
> 
> So this is like _prepare_state for AbstractSequenceCommand?

Precisely.

> > Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:50
> > +        server_url = "http://localhost:%d/" % options.httpd_port
> 
> Could default to None and use a random port above 8000 when not specified.

We could.

> > Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:55
> > +        # FIXME: This seems racy.
> > +        threading.Timer(0.1, lambda: self._tool.user.open_url(server_url)).start()
> 
> Yes, quite racy.  The http server for DRT has an "is it up" method which polls in a loop.

It seems to work ok.

> > Tools/Scripts/webkitpy/tool/commands/abstractservercommand.py:57
> > +        httpd = self.server(httpd_port=options.httpd_port, config=config)
> 
> You could **config the config to pass them as keyword arguments intead of as a dictionary.  I'm not sure if that's more clear or not.

Sure.

> > Tools/Scripts/webkitpy/tool/commands/options.py:33
> > +    httpd_port = make_option("--httpd-port", action="store", type="int", default=8127, help="Port to use for the HTTP server")
> 
> Not sure the default is very helpful.  See above.
> 
> I like that we're maing an Options class for commands.  Might be a little premature though.  I wonder how many other commands have options.

Yeah, it was more useful in an earlier version of the patch, but turns not to be need that much in the final version.

> > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:-84
> > -        if options.dry_run:
> > -
> > -            def no_op_copyfile(src, dest):
> > -                pass
> > -
> > -            def no_op_add(path, return_exit_code=False):
> > -                if return_exit_code:
> > -                    return 0
> > -
> > -            filesystem.copyfile = no_op_copyfile
> > -            scm.add = no_op_add
> > -
> 
> So you're just removing this?  Seems these could easily use the mock versions.

Yeah, that code doesn't seem useful.

> > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:89
> > +        return {
> 
> Always feels a bit odd returning a dictionary.

What else would you return?  It can really be anything,

> > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:37
> > +        BaseHTTPServer.HTTPServer.__init__(self, ("", httpd_port), GardeningHTTPRequestHandler)
> 
> What's the "", port tuple?

No idea.

> > Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:45
> > +    STATIC_FILE_DIRECTORY = os.path.join(os.path.dirname(__file__), "data", "gardeningserver")
> 
> Do you want this to always be relative to __file__?  If you use filesystem.path_for_module you can get a mockable value for __file__.

Ok.
Comment 6 Adam Barth 2011-07-05 15:26:57 PDT
Created attachment 99753 [details]
Patch
Comment 7 Eric Seidel (no email) 2011-07-05 15:49:51 PDT
Comment on attachment 99753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99753&action=review

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:25
> +import BaseHTTPServer

Is this the import you mean?  Is that part of std python?
Comment 8 Adam Barth 2011-07-05 15:52:05 PDT
Committed r90410: <http://trac.webkit.org/changeset/90410>