Bug 58301 – Extract map from port to builder name.

• Component: New Bugs
• Status: RESOLVED FIXED

[James Kozianski]

Extract map from port to builder name.

[James Kozianski]

Created attachment 89164 [details]
Patch

[James Kozianski]

Ojan, is this what you had in mind?

[Eric Seidel (no email)]

Comment on attachment 89164 [details]
Patch

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

OMG this is so useful.  You might want to clarify where these builders live (build.webkit.org) in a small comment.

Is it possible to look up a builder by name?  What do we use thsi for?

> Tools/Scripts/webkitpy/layout_tests/port/builders.py:45
> +    'chromium-mac-snowleopard': 'Mac_10_6_Tests__dbg__1_',

Do we really have builders named this?

[Eric Seidel (no email)]

I also suspect it's possible to generate some of this list dynamically by scraping buildbot.  We would do that lazilly and then cache it of course.

[Tony Chang]

Comment on attachment 89164 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/builders.py:45
>> +    'chromium-mac-snowleopard': 'Mac_10_6_Tests__dbg__1_',
> 
> Do we really have builders named this?

Yes.  The actual name is "Mac 10.6 Tests (dbg)(1)", but all the punctuation and spaces get converted to _ because they are also directories on disk.

[Eric Seidel (no email)]

Comment on attachment 89164 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/port/builders.py:45
>>> +    'chromium-mac-snowleopard': 'Mac_10_6_Tests__dbg__1_',
>> 
>> Do we really have builders named this?
> 
> Yes.  The actual name is "Mac 10.6 Tests (dbg)(1)", but all the punctuation and spaces get converted to _ because they are also directories on disk.

So why do we see "Leopard Intel Debug (Tests)" in this list, and the gobbledy-gook above?  Should we be writing the real name, and converting to _ in a separate step?

[James Kozianski]

Created attachment 89330 [details]
Patch

[James Kozianski]

> So why do we see "Leopard Intel Debug (Tests)" in this list, and the gobbledy-gook above?  Should we be writing the real name, and converting to _ in a separate step?

This list is actually a combination of Chromium and WebKit builders (see latest patch). Is there a use for the non underscored builder names (these names are based off URLs)? Otherwise perhaps we should just leave them as is.

[Eric Seidel (no email)]

It's unclear what the use of a mixed list is.  Don't we need the information about which buildbot they came from?  Or should we have two separate lists?  (And a single lookup funciton which knows how to check both?) I think I'm missing some information here. :)

[James Kozianski]

(In reply to comment #9)
> It's unclear what the use of a mixed list is.  Don't we need the information about which buildbot they came from?  Or should we have two separate lists?  (And a single lookup funciton which knows how to check both?) I think I'm missing some information here. :)

I'm not sure either :-) For my rebaseline tool, all I need is a big tree indicating the fallback orders of all the ports. Currently it constructs that by getting a Port for each item in the mixed list and asking it for its baseline_search_path(). Grepping through the webkitpy code, it seems that this is what layout_tests/deduplicate_tests.py uses it for as well (and is the only caller of the function).

[Eric Seidel (no email)]

The rebaselining stuff is a rather new addition to webkitpy. :)  It was imported from Chrome's repo, so I can't say I know the code very well.

It seems reasonable to have a mapping from bots to their respective ports.  If that's not available on the bots themselves, than it's OK to hard code that into webkitpy.

I could see it being reasonable to be able to ask such a question of the Port object, and that the Builder object should be able to tell you what Port the builder is.

In any case, I'm not sure my suggestions were very helpful. , I'm glad to see you're tryign to clean up the rebaseline code, it was not written for re-use (as was much of the code we imported from Chrome, sadly).

[Ojan Vafai]

Comment on attachment 89330 [details]
Patch

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

This looks great. Please address my comments then feel free to commit.

Can you add a FIXME to make the values in this maps be lists? That way we can encode the fact that each port corresponds to 0-n bots (e.g. release/debug bots).

> Tools/Scripts/webkitpy/layout_tests/port/builders.py:30
> +# Compiled manually from http://build.chromium.org/p/chromium/json/builders/help?as_text=1

Maybe add a comment that the None values are ports that don't have a running bot at build.webkit.org and/or build.chromium.org?

> Tools/Scripts/webkitpy/layout_tests/port/builders.py:32
> +    'chromium-gpu-linux': None,

This should be 'Webkit_Linux_-_GPU'. Ditto for the others chromium ports below. You can see the full list here: http://build.chromium.org/f/chromium/layout_test_results/

> Tools/Scripts/webkitpy/layout_tests/port/builders.py:42
> +    'chromium-linux-x86': 'Linux_Tests__dbg__1_',

Can you make the values for the chromium port be the actual bot names and then add a helper function to this file? getBuilderPathFromName?

> Tools/Scripts/webkitpy/layout_tests/port/builders.py:64
> +    'qt-linux': None,

This one should be "Qt Linux Release", right?

[Ojan Vafai]

Comment on attachment 89330 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/builders.py:42
>> +    'chromium-linux-x86': 'Linux_Tests__dbg__1_',
> 
> Can you make the values for the chromium port be the actual bot names and then add a helper function to this file? getBuilderPathFromName?

get_path_from_name might be a better function name.

[James Kozianski]

Created attachment 91199 [details]
Patch

[James Kozianski]

(In reply to comment #12)
> (From update of attachment 89330 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89330&action=review
> 
> This looks great. Please address my comments then feel free to commit.
> 
> Can you add a FIXME to make the values in this maps be lists? That way we can encode the fact that each port corresponds to 0-n bots (e.g. release/debug bots).

Done.

> > Tools/Scripts/webkitpy/layout_tests/port/builders.py:30
> > +# Compiled manually from http://build.chromium.org/p/chromium/json/builders/help?as_text=1
> 
> Maybe add a comment that the None values are ports that don't have a running bot at build.webkit.org and/or build.chromium.org?

Done.

> > Tools/Scripts/webkitpy/layout_tests/port/builders.py:32
> > +    'chromium-gpu-linux': None,
> 
> This should be 'Webkit_Linux_-_GPU'. Ditto for the others chromium ports below. You can see the full list here: http://build.chromium.org/f/chromium/layout_test_results/

Cheers! Done.

> > Tools/Scripts/webkitpy/layout_tests/port/builders.py:42
> > +    'chromium-linux-x86': 'Linux_Tests__dbg__1_',
> 
> Can you make the values for the chromium port be the actual bot names and then add a helper function to this file? getBuilderPathFromName?
> 
> > Tools/Scripts/webkitpy/layout_tests/port/builders.py:64
> > +    'qt-linux': None,
> 
> This one should be "Qt Linux Release", right?

Done.

[Eric Seidel (no email)]

Comment on attachment 91199 [details]
Patch

This is very hot.  Very very hot.  Thanks!

[James Kozianski]

Created attachment 91203 [details]
Patch

[James Kozianski]

(In reply to comment #13)
> (From update of attachment 89330 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89330&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/port/builders.py:42
> >> +    'chromium-linux-x86': 'Linux_Tests__dbg__1_',
> > 
> > Can you make the values for the chromium port be the actual bot names and then add a helper function to this file? getBuilderPathFromName?
> 
> get_path_from_name might be a better function name.

Done.

[Eric Seidel (no email)]

Comment on attachment 91203 [details]
Patch

Still awesome.  Ideally we would unittest get_path_from_name  (Which probably should be an _ function anyway.... and I think WebKit generaly avoids using "get" in function names).

[James Kozianski]

Created attachment 91228 [details]
Patch

[James Kozianski]

(In reply to comment #19)
> (From update of attachment 91203 [details])
> Still awesome.  Ideally we would unittest get_path_from_name  (Which probably should be an _ function anyway.... and I think WebKit generaly avoids using "get" in function names).

Thanks, Eric! I've added a unit test and renamed the function :-)

[Eric Seidel (no email)]

Comment on attachment 91228 [details]
Patch

Fantastic!

[WebKit Commit Bot]

Comment on attachment 91228 [details]
Patch

Clearing flags on attachment: 91228

Committed r85040: <http://trac.webkit.org/changeset/85040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.