Bug 48264 – new-run-webkit-tests: split out webkit-specific configuration stuff into a new module

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

new-run-webkit-tests: split out webkit-specific configuration stuff into a new module

[Dirk Pranke]

Created attachment 71793 [details]
Patch

[Dirk Pranke]

Created attachment 71815 [details]
Patch

[Dirk Pranke]

Note that I'm not wedded to either the name "config" or the location in the tree (under layout_tests/port) for this new class. 

The class doesn't depend on anything else in layout_tests/ and could move to webkitpy/common/config. At some point we should unify those classes, but it's not 100% clear to me how to do so yet.

[Eric Seidel (no email)]

You're aware of webkitpy.common.config right?

[Dirk Pranke]

(In reply to comment #4)
> You're aware of webkitpy.common.config right?

Yes; you replied to the email I sent about that. That said, it's not clear to me what to do about the two classes since they have largely disjoint functionality. That's why there's a FIXME at the top of the file to do something about it at some point ...

[Adam Barth]

Comment on attachment 71815 [details]
Patch

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

I don't really understand this patch.  Seems to replicate functionality that already exists in webkitpy but it doesn't remove that code, so we just end up with more duplicate code.  Without seeing this code in use, it's difficult to know whether this patch is an improvement or not.

> WebKitTools/Scripts/webkitpy/layout_tests/port/config.py:63
> +    _FLAGS_FROM_CONFIGURATIONS = {
> +        "Debug": "--debug",
> +        "Release": "--release",
> +    }

Surely this information exists in webkitpy already.  I don't see any minus lines in this patch...

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 71815 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71815&action=review
> 
> I don't really understand this patch.  Seems to replicate functionality that already exists in webkitpy but it doesn't remove that code, so we just end up with more duplicate code.  Without seeing this code in use, it's difficult to know whether this patch is an improvement or not.
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/config.py:63
> > +    _FLAGS_FROM_CONFIGURATIONS = {
> > +        "Debug": "--debug",
> > +        "Release": "--release",
> > +    }
> 
> Surely this information exists in webkitpy already.  I don't see any minus lines in this patch...

There is a similar chunk of code in common/config/ports.py (build_webkit_command), but it's tied to the build_webkit_command() routine.

This code does not remove any other code. The duplicate code in layout_tests will be removed in the patch in bug 48280. The duplicate code in common/config/ports is not yet removed because the code that uses config/ports will need to be refactored, which I think should be done in a separate patch in order to be manageable (and there is value in this refactoring even without the config/ports refactoring).

Given that, does this patch make more sense now?

[Adam Barth]

Comment on attachment 71815 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/layout_tests/port/config.py:91
> +        ed)turns whether build was successful or is up-to-date.

ed)turns => returns

[Adam Barth]

Comment on attachment 71815 [details]
Patch

I feel sad that we're adding a bunch of redundant code, but Dirk promises me that he'll remove the other instances.

[Dirk Pranke]

Committed r71374: <http://trac.webkit.org/changeset/71374>