Bug 63872 – Add trivial garden-o-matic command (with server)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Add trivial garden-o-matic command (with server)

[Adam Barth]

Created attachment 99571 [details]
Patch

[James Robinson]

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).

[Adam Barth]

For realz?  Ok.  I'll fix it on landing.

[Eric Seidel (no email)]

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__.

[Adam Barth]

(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.

[Adam Barth]

Created attachment 99753 [details]
Patch

[Eric Seidel (no email)]

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?

[Adam Barth]

Committed r90410: <http://trac.webkit.org/changeset/90410>