Bug 76357 – webkitpy: add determine_full_port_name(), clean up port.__init__()

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

broken out from bug 76215.

[Dirk Pranke]

Created attachment 122587 [details]
Patch

[Adam Barth]

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
> [phone]  Dirk Pranke  <[e-mail]>

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?

[Dirk Pranke]

(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
> > [phone]  Dirk Pranke  <[e-mail]>
> 
> 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.

[Dirk Pranke]

Created attachment 122799 [details]
fix diff, consolidate apple logic

[Dirk Pranke]

Committed r105183: <http://trac.webkit.org/changeset/105183>

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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

[Eric Seidel (no email)]

Ojan and Evan Martin are the folks behind -g, iirc.  I'm just a happy user.  I do encourage you to file bugs.  Thanks!