| Summary: | Extract map from port to builder name. | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | James Kozianski <koz> | ||||||||||||
| Component: | New Bugs | Assignee: | James Kozianski <koz> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | commit-queue, eric, ojan | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Other | ||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
James Kozianski
2011-04-11 23:42:48 PDT
Created attachment 89164 [details]
Patch
Ojan, is this what you had in mind? 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? 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. 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. 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? Created attachment 89330 [details]
Patch
> 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.
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. :) (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). 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). 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? 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. Created attachment 91199 [details]
Patch
(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. Comment on attachment 91199 [details]
Patch
This is very hot. Very very hot. Thanks!
Created attachment 91203 [details]
Patch
(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. 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).
Created attachment 91228 [details]
Patch
(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 :-) Comment on attachment 91228 [details]
Patch
Fantastic!
Comment on attachment 91228 [details] Patch Clearing flags on attachment: 91228 Committed r85040: <http://trac.webkit.org/changeset/85040> All reviewed patches have been landed. Closing bug. |