Bug 76357 - webkitpy: add determine_full_port_name(), clean up port.__init__()
Summary: webkitpy: add determine_full_port_name(), clean up port.__init__()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 76356
Blocks: 76215 76360
  Show dependency treegraph
 
Reported: 2012-01-15 20:40 PST by Dirk Pranke
Modified: 2012-01-17 13:05 PST (History)
4 users (show)

See Also:


Attachments
Patch (44.84 KB, patch)
2012-01-15 20:53 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix diff, consolidate apple logic (38.48 KB, patch)
2012-01-17 12:24 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-01-15 20:40:59 PST
broken out from bug 76215.
Comment 1 Dirk Pranke 2012-01-15 20:53:12 PST
Created attachment 122587 [details]
Patch
Comment 2 Adam Barth 2012-01-17 02:05:39 PST
Comment on attachment 122587 [details]
Patch

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

It's slightly hard to read this patch because it has the earlier patch mixed in.  Generally, this looks like a reasonable patch.  My main concern is that the mac and win ports seem to have some very similar looking code that probably should be shared.

> Tools/ChangeLog:101
> +2012-01-15  Dirk Pranke  <dpranke@chromium.org>

Looks like you've got two ChangeLogs here.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:116
> +        assert port_name in ('chromium-linux', 'chromium-gpu-linux',
> +                             'chromium-linux-x86', 'chromium-linux-x86_64',
> +                             'chromium-gpu-linux-x86_64')

Can we avoid listing information about all the subclasses here?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:50
> +            # FIXME: This may be wrong, since options is a global property, and the port_name is specific to this object?
> +            if options and not hasattr(options, 'webkit_test_runner'):
> +                options.webkit_test_runner = True

Yuck.  options is usually immutable.

> Tools/Scripts/webkitpy/layout_tests/port/win.py:59
> +    @classmethod
> +    def determine_full_port_name(cls, host, options, port_name):
> +        if port_name.endswith('-wk2'):
> +            # FIXME: This may be wrong, since options is a global property, and the port_name is specific to this object?
> +            if options and not hasattr(options, 'webkit_test_runner'):
> +                options.webkit_test_runner = True
> +            port_name = port_name.replace('-wk2', '')
> +        if port_name.endswith('win'):
> +            if host.platform.os_name != 'win':
> +                return 'win'
> +            return port_name + '-' + host.platform.os_version
> +        return port_name

Can we share some of this code with MacPort?
Comment 3 Dirk Pranke 2012-01-17 12:10:29 PST
(In reply to comment #2)
> (From update of attachment 122587 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122587&action=review
> 
> It's slightly hard to read this patch because it has the earlier patch mixed in.  Generally, this looks like a reasonable patch.  My main concern is that the mac and win ports seem to have some very similar looking code that probably should be shared.
> 

Whoops. I really need to teach webkit-patch to handle stacked patches, because I often forget to tell it to diff against the tracking branch. Sorry!

> > Tools/ChangeLog:101
> > +2012-01-15  Dirk Pranke  <dpranke@chromium.org>
> 
> Looks like you've got two ChangeLogs here.
>

Fixed as per above (I will post the corrected patch).
 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:116
> > +        assert port_name in ('chromium-linux', 'chromium-gpu-linux',
> > +                             'chromium-linux-x86', 'chromium-linux-x86_64',
> > +                             'chromium-gpu-linux-x86_64')
> 
> Can we avoid listing information about all the subclasses here?
>

I don't think so, because they're not all subclasses.
 
> > Tools/Scripts/webkitpy/layout_tests/port/mac.py:50
> > +            # FIXME: This may be wrong, since options is a global property, and the port_name is specific to this object?
> > +            if options and not hasattr(options, 'webkit_test_runner'):
> > +                options.webkit_test_runner = True
> 
> Yuck.  options is usually immutable.
>

I don't usually view options as immutable. There are other places in the layout_tests package where we write to options because setting up the default value ahead of parsing the arguments is difficult or impossible (as it is here).

We could pull all of the values off of options and on to other objects, but that doesn't seem like a better alternative to me.

Maybe you have a different approach you prefer?
 
> > Tools/Scripts/webkitpy/layout_tests/port/win.py:59
> > +    @classmethod
> > +    def determine_full_port_name(cls, host, options, port_name):
> > +        if port_name.endswith('-wk2'):
> > +            # FIXME: This may be wrong, since options is a global property, and the port_name is specific to this object?
> > +            if options and not hasattr(options, 'webkit_test_runner'):
> > +                options.webkit_test_runner = True
> > +            port_name = port_name.replace('-wk2', '')
> > +        if port_name.endswith('win'):
> > +            if host.platform.os_name != 'win':
> > +                return 'win'
> > +            return port_name + '-' + host.platform.os_version
> > +        return port_name
> 
> Can we share some of this code with MacPort?

Will fix.
Comment 4 Dirk Pranke 2012-01-17 12:24:34 PST
Created attachment 122799 [details]
fix diff, consolidate apple logic
Comment 5 Dirk Pranke 2012-01-17 12:27:47 PST
Committed r105183: <http://trac.webkit.org/changeset/105183>
Comment 6 Eric Seidel (no email) 2012-01-17 12:48:59 PST
I assume your'e aware of the -g option to webkit-patch upload?

webkit-patch -g head

or

webkit-patch -g head..

are super useful for uploading just a single commit, or everything since a commit.
Comment 7 Dirk Pranke 2012-01-17 12:58:39 PST
(In reply to comment #6)
> I assume your'e aware of the -g option to webkit-patch upload?
> 
> webkit-patch -g head
> 
> or
> 
> webkit-patch -g head..
> 
> are super useful for uploading just a single commit, or everything since a commit.

Yes, I'm aware of it, but in my experience it doesn't always work "right" (I need to file bugs for these things to confirm them):

1) I usually need to specify -g branch..HEAD; -g branch or -g branch.. doesn't work for me (haven't tried this one lately, though).

2) The change doesn't pick up any uncommitted files; not usually a problem except when I"m generating a changelog. I wouldn't expect -g branch..HEAD to work in this case, but since I haven't gotten -g branch to work ...

Anyway, what I was actually referring to was getting webkit-patch to look for and recognize the upstream branch tracking properly (it should look for the config setting for branch.$BRANCH.merge and diff against that automatically, I think) so that I don't have to specify the base.
Comment 8 Eric Seidel (no email) 2012-01-17 13:05:34 PST
Ojan and Evan Martin are the folks behind -g, iirc.  I'm just a happy user.  I do encourage you to file bugs.  Thanks!