Bug 84642 - Make garden-o-matic work for the Apple Mac port
Summary: Make garden-o-matic work for the Apple Mac port
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: Simon Fraser (smfr)
URL:
Keywords:
Depends on: 89164 89332
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-23 15:21 PDT by Simon Fraser (smfr)
Modified: 2012-06-19 16:54 PDT (History)
15 users (show)

See Also:


Attachments
Work in progress (24.96 KB, patch)
2012-06-12 18:26 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
More work in progress. (33.00 KB, patch)
2012-06-14 22:09 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (44.97 KB, patch)
2012-06-15 23:12 PDT, Simon Fraser (smfr)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2012-04-23 15:21:37 PDT
As discussed at the WebKit Developers Conference,  garden-o-matic needs to work for the Apple Mac port.
Comment 1 Dirk Pranke 2012-04-23 15:26:47 PDT
cc'ing abarth since I believe he is most in the know about what's remaining to support this. I'll be happy to work on it once I can sync up w/ him.
Comment 2 Ojan Vafai 2012-04-25 16:15:14 PDT
I outlined the steps I think are needed in bug 82719. Copy-pasting here for easier reference.

If I were doing this, I'd do the following set of patches:
1. Adding a drop-down in the top-right corner of garden-o-matic to choose your platform and then add a builders.js file for Apple (we can add other ports later when all the kinks are worked out).
2. Fix all the garden-o-matic JS code that uses build.chromium.org to use build.webkit.org for non-chromium platforms. Off the top of my head, the only things that need to change are the paths to the expected/actual results and the "master" query parameter passed to test-results.appspot.com (ChromiumWebkit --> webkit.org).
3. Fix webkit-patch rebaseline-test to do the right thing for non-Chromium bots. I think this is just making it grab actual results off build.webkit.org instead of build.chromium.org.
Comment 3 Simon Fraser (smfr) 2012-06-12 13:44:52 PDT
Going to take a stab at this.
Comment 4 Ojan Vafai 2012-06-12 14:06:20 PDT
As a followup to the steps I outlined, I think we might also want an "all platforms" view for garden-o-matic for the use-case of wanting to rebaseline a test you just committed across all platforms. But I think that's a natural followup change. The primary use-case is wanting to get your platform's builders green and for that we want a dropdown where you pick your platform.
Comment 5 Dirk Pranke 2012-06-12 14:22:35 PDT
it seems to me that it might make sense to specify the default platform on the command line (using the usual -platform/--chromium/--gtk/etc flags) rather than forcing the user to always select from the drop down. I'm not even sure we need/want the dropdown at all (although I do think an "all platforms" is a nice long-term dream).
Comment 6 Ojan Vafai 2012-06-12 14:30:59 PDT
(In reply to comment #5)
> it seems to me that it might make sense to specify the default platform on the command line (using the usual -platform/--chromium/--gtk/etc flags) rather than forcing the user to always select from the drop down. I'm not even sure we need/want the dropdown at all (although I do think an "all platforms" is a nice long-term dream).

I'm not set on the specifics of the UI. We can always tweak it if we find it's less than ideal. Simon, feel free to approach this bit however you want. My point was more that the UI currently assumes one platform (chromium) and making the UI mutli-platform aware is probably not as good as letting you pick the platform given the primary use-cases.
Comment 7 Dirk Pranke 2012-06-12 14:33:10 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > it seems to me that it might make sense to specify the default platform on the command line (using the usual -platform/--chromium/--gtk/etc flags) rather than forcing the user to always select from the drop down. I'm not even sure we need/want the dropdown at all (although I do think an "all platforms" is a nice long-term dream).
> 
> I'm not set on the specifics of the UI. We can always tweak it if we find it's less than ideal. Simon, feel free to approach this bit however you want. My point was more that the UI currently assumes one platform (chromium) and making the UI mutli-platform aware is probably not as good as letting you pick the platform given the primary use-cases.

Oh, yeah, I agree that single-port is a better default than all ports.
Comment 8 Simon Fraser (smfr) 2012-06-12 15:02:53 PDT
I was going to add a platform flag, which sets the popup to the platform. But I agree that switching platforms on the fly isn't that important (other than possibly making development easier).
Comment 9 Adam Barth 2012-06-12 15:11:31 PDT
smfr: Please feel free to grab me on #webkit if you have any questions.  I would probably start by hacking builders.js and looking to see what breaks.
Comment 10 Simon Fraser (smfr) 2012-06-12 18:22:47 PDT
I think we're going to need some server-side changes on b.w.o here. Chromium uses a consistent directory layout, and keeps json results around after zipping up test results:

http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_7/140722/

build.webkit.org, however, has results directories like:
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20Tests/r120136%20%284%29/
then at some point moves things to old-results:
http://build.webkit.org/old-results/Lion%20Intel%20Release%20%28Tests%29/
but doesn't keep the json file separate from the zip file.
Comment 11 Simon Fraser (smfr) 2012-06-12 18:23:21 PDT
Also note the %20 vs underscores in the directory names.
Comment 12 Simon Fraser (smfr) 2012-06-12 18:26:32 PDT
Created attachment 147206 [details]
Work in progress
Comment 13 Simon Fraser (smfr) 2012-06-12 18:27:38 PDT
Comment on attachment 147206 [details]
Work in progress

Comments on the patch welcome, but not ready for review.
Comment 14 WebKit Review Bot 2012-06-12 18:29:08 PDT
Attachment 147206 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/BuildSlaveSupport/build.webkit.org-c..." exit_code: 1
Tools/Scripts/webkitpy/layout_tests/port/builders.py:56:  whitespace before ':'  [pep8/E203] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Dirk Pranke 2012-06-12 18:54:43 PDT
(In reply to comment #10)
> I think we're going to need some server-side changes on b.w.o here. Chromium uses a consistent directory layout, and keeps json results around after zipping up test results:
> 
> http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_7/140722/
> 
> build.webkit.org, however, has results directories like:
> http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20Tests/r120136%20%284%29/
> then at some point moves things to old-results:
> http://build.webkit.org/old-results/Lion%20Intel%20Release%20%28Tests%29/
> but doesn't keep the json file separate from the zip file.

Looks like you're on the right track with this patch. Do we know when things get moved to old-results? I would imagine if it's not for 24 hours or more after the build completes, we can probably ignore its existence.
Comment 16 Adam Barth 2012-06-12 19:25:51 PDT
Comment on attachment 147206 [details]
Work in progress

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

Yep.  Definitely looks like you're on the right track.

Having a directory on the buildbot that accumulates results for each builder is pretty useful.  If we can add that to build.webkit.org, that would be helpful both for the UI and for the backend of the rebaseline functionality.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/config.js:101
> +    config.kCurrentPlatform = platform;

Just a style nit: The "k" in kCurrentPlatform means "constant".  You might want to call this variable config.currentPlatform

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:95
> +        // FIXME: can jQuery do this?

I think so.  If not, we should probably add this sort of function to base.js so we can re-use it elsewhere.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:110
> +        Object.keys(config.kPlatforms).forEach(function(platformName) {

You might want to sort this list before iterating over it.  Object.keys can give things back in a wacky order.
Comment 17 Adam Barth 2012-06-12 19:27:16 PDT
In case you haven't found it yet, you can run the unit test by opening http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/run-unittests.html in a browser.
Comment 18 Simon Fraser (smfr) 2012-06-12 21:41:53 PDT
build.chromium.org has directories which I presume are symlinks to the most recent results, like:
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win/results/

b.w.o has the revision and build number in the url:
http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r120154%20%2816%29/
and no 'latest' symlink.
Comment 19 Adam Barth 2012-06-12 21:48:09 PDT
They're not actually symlinks.  The results of each build are just spammed into the folder, so it accumulates the most recent failure for each test.  Having a symlink to the latest build would probably work fine for garden o matic.  Accumulating the results is most useful for the flakiness dashboard to show you how each test failed during the most recent flake.
Comment 20 Simon Fraser (smfr) 2012-06-12 21:55:36 PDT
I'm starting to think that garden-o-matic requires quite a bit of server-side support. I wish Chromium hadn't forked the tools so long ago :(
Comment 21 Adam Barth 2012-06-12 22:19:07 PDT
Hey now.  Be nice.  :)
Comment 22 Ojan Vafai 2012-06-12 23:22:34 PDT
(In reply to comment #19)
> They're not actually symlinks.  The results of each build are just spammed into the folder, so it accumulates the most recent failure for each test.  Having a symlink to the latest build would probably work fine for garden o matic.  Accumulating the results is most useful for the flakiness dashboard to show you how each test failed during the most recent flake.

TL;DR: Do whatever is easiest for you here.

Historically, the spamming was done because it was easiest to do it that way with some complexity of chromium's buildbot machinery. If you just symlink, all the tools should work fine. The only thing you'd lose is the ability to see actual results for flaky tests (e.g. if a test fails one run, but passes in the most recent run, you wouldn't be able to easily see the output of the failure run in the flakiness dashboard). It's actually no worse than the current state of the flakiness dashboard for the b.w.o. bots.
Comment 23 Ojan Vafai 2012-06-12 23:37:44 PDT
Comment on attachment 147206 [details]
Work in progress

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

I don't have any significant feedback either. This patch is doing it all as I'd expect/want.

> Tools/ChangeLog:14
> +        (.):

Side-nit: we really should change prepare-ChangeLog to understand anonymous top-level JS functions better so we don't get this spam in the output. Obviously, not related to the patch in question. :)

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:95
>> +        // FIXME: can jQuery do this?
> 
> I think so.  If not, we should probably add this sort of function to base.js so we can re-use it elsewhere.

I don't see anything with a quick glance at http://docs.jquery.com/index.php?title=UI. Also, all we really want here is a select element. I'd rather we not add a complex jquery widget here.

Having a generic function that takes a dictionary-style data-structure and spits out a select element (or populates an existing select element?) in base seems reasonable though.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:98
> +                (RegExp(name + '=' + '(.+?)(&|$)').exec(location.search)||[,null])[1]

I like this! It's a bit hard to read, but it's a nice one-line way of doing this.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:107
> +        var platformSelect = document.getElementById('platform-picker');

Looks like you haven't hooked up an onchange handler to this yet. When do you, I think the easiest thing might be to change the platform parameter and just window.location.reload(). That way you don't have to worry about cleaning up global state or other single-platform assumptions garden-o-matic makes.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:108
> +        $(platformSelect).empty();

Nit: I don't think this does anything here since it's already empty.
Comment 24 Ojan Vafai 2012-06-12 23:43:54 PDT
(In reply to comment #20)
> I'm starting to think that garden-o-matic requires quite a bit of server-side support. I wish Chromium hadn't forked the tools so long ago :(

An alternative to changing the server would be having garden-o-matic learn to grab the actual results files using URLs that include the revision number of the latest run. We actually have access to this from the full_results.json file that we already load in order to generate the list of failing tests. The json has a "revision" key that should match the revision number in the b.w.o. urls.

This is what the flakiness dashboard does to handle the b.w.o. bots currently and it works fine. It just makes the JS code a little more complicated. I think changing the server might be simpler in the long-run, but I'm fine with doing this either way.
Comment 25 Adam Barth 2012-06-12 23:51:18 PDT
Comment on attachment 147206 [details]
Work in progress

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

>>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:95
>>> +        // FIXME: can jQuery do this?
>> 
>> I think so.  If not, we should probably add this sort of function to base.js so we can re-use it elsewhere.
> 
> I don't see anything with a quick glance at http://docs.jquery.com/index.php?title=UI. Also, all we really want here is a select element. I'd rather we not add a complex jquery widget here.
> 
> Having a generic function that takes a dictionary-style data-structure and spits out a select element (or populates an existing select element?) in base seems reasonable though.

I think he meant parse query parameters out of URLs.  :)
Comment 26 Ojan Vafai 2012-06-12 23:53:07 PDT
(In reply to comment #25)
> (From update of attachment 147206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147206&action=review
> 
> >>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:95
> >>> +        // FIXME: can jQuery do this?
> >> 
> >> I think so.  If not, we should probably add this sort of function to base.js so we can re-use it elsewhere.
> > 
> > I don't see anything with a quick glance at http://docs.jquery.com/index.php?title=UI. Also, all we really want here is a select element. I'd rather we not add a complex jquery widget here.
> > 
> > Having a generic function that takes a dictionary-style data-structure and spits out a select element (or populates an existing select element?) in base seems reasonable though.
> 
> I think he meant parse query parameters out of URLs.  :)

Lol. That makes more sense.
Comment 27 Simon Fraser (smfr) 2012-06-13 08:00:49 PDT
(In reply to comment #24)
> (In reply to comment #20)
> > I'm starting to think that garden-o-matic requires quite a bit of server-side support. I wish Chromium hadn't forked the tools so long ago :(
> 
> An alternative to changing the server would be having garden-o-matic learn to grab the actual results files using URLs that include the revision number of the latest run. We actually have access to this from the full_results.json file that we already load in order to generate the list of failing tests. The json has a "revision" key that should match the revision number in the b.w.o. urls.
> 
> This is what the flakiness dashboard does to handle the b.w.o. bots currently and it works fine. It just makes the JS code a little more complicated. I think changing the server might be simpler in the long-run, but I'm fine with doing this either way.

Yeah, this was going to be my next approach. We're also getting revs and build numbers from each builder, but I haven't figured out the loading dependencies yet.
Comment 28 Simon Fraser (smfr) 2012-06-14 21:52:44 PDT
Got as far as requesting full_results.json from build.webkit.org, but it doesn't set the Access-Control-Allow-Origin: * header, so I can't load them.
Comment 29 Simon Fraser (smfr) 2012-06-14 22:06:49 PDT
Filed bug 89164 on b.w.o for CORS stuff.
Comment 30 Simon Fraser (smfr) 2012-06-14 22:09:25 PDT
Created attachment 147729 [details]
More work in progress.
Comment 31 Adam Barth 2012-06-15 09:58:02 PDT
(In reply to comment #28)
> Got as far as requesting full_results.json from build.webkit.org, but it doesn't set the Access-Control-Allow-Origin: * header, so I can't load them.

If you want to continue to make progress, you can hack SecurityOrigin::canRequest to always return true, or you can set Settings::disableWebSecurity to true.
Comment 32 Simon Fraser (smfr) 2012-06-15 23:12:56 PDT
Created attachment 147951 [details]
Patch
Comment 33 Simon Fraser (smfr) 2012-06-15 23:14:29 PDT
Patch is good for review. A couple of unit tests fail (checkout: rebaseline, ui.notifications: BuildersFailing). Not sure if that's my fault or not.

The perf panel doesn't work with the Apple builders. Haven't figured out why not yet.
Comment 34 Adam Barth 2012-06-16 00:51:57 PDT
Comment on attachment 147951 [details]
Patch

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

This looks great!  A bunch of minor comments below.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/LayoutTestResultsLoader.js:67
> +        window.console.log('fetching NRWT results ', this._builder.resultsDirectoryURL(buildName) + 'full_results.json')

Should we put these console message behind a debug flag?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/builders.js:81
> +// FIXME: export, called by results.
> +function indexOfMostRecentCompletedBuild(individualBuilderStatus)

Usually the way we export things is with this idiom:

builders.indexOfMostRecentCompletedBuild = function(indexOfMostRecentCompletedBuild)
...

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/builders.js:140
> +    net.get(builderInfoURL, function(builderInfo) {
> +        callback(builderInfo);
> +    });

You can just pass callback directly to net.get.  There's no need for the extra thunk.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/builders.js:152
> +            selectURL += (first ? '?' : '&') + 'select=' + buildNumber;

A slightly better idiom for doing this is to build up a dictionary and then use $.param to convert the dictionary into a query string.  That will handle the escaping correctly in case anything funky is going on.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/builders.js:158
> +        net.get(selectURL, function(cachedBuildsInfo) {
> +            callback(cachedBuildsInfo);
> +        });

Same comment about the extra thunk.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/model.js:174
> +        // if (testName != 'editing/spelling/grammar-edit-word.html')
> +        //     return;

We should probably remove this before landing.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/model.js:184
> -
> +            

This change seem spurious.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/net.js:61
> +            // window.console.log('failed to load', url, textStatus, errorThrown);

We should either remove this line or uncomment it and put it behind a debug flag.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:393
> +                continueWalk();

I always worry that this function is going to overflow its stack, but I think we can wait for that problem to occur before worrying about it too much.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:621
> +        net.jsonp(buildURLs[currentIndex], resultsCallback);

There's probably a way of avoiding repeating this line, but it's probably not worthwhile.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:629
> +        var requestsInFlight = builderNameList.length;

Sorry to have created a merge conflict for you in this file.  You should feel free to revert my change to this function.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:643
> +                    ++requestsInFlight;

Interesting.  I'd probably change this around to use RequestTracker, but you or I can do that in a later patch.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:99
> +        function getURLParameter(name) {
> +            return decodeURI(
> +                (RegExp(name + '=' + '(.+?)(&|$)').exec(location.search)||[,null])[1]
> +            );
> +        }

Can you move this function to base.js?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:106
> +        var platformSelect = document.getElementById('platform-picker');

Typically we'd say $(this, "#platform-picker") to scope this selector to this object.  It's not super important here, though, because this UI element really is a singleton.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:108
> +        Object.keys(config.kPlatforms).forEach(function(platformName) {

You should always call sort() after calling Object.keys so that you iterate over the keys in a predictable order.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:122
> +            var url = window.location.href;
> +            var queryIndex = url.indexOf('?');
> +            if (queryIndex != -1)
> +                url = url.substring(0, queryIndex);
> +            window.location.href = url + '?platform=' + platformSelect.selectedOptions[0]._platform;

Why not just assign to window.location.search?

location.search = '?platform=' + platformSelect.selectedOptions[0]._platform;

That also has the advantage of preserving the fragment, which we use for remembering which tab is currently open.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/styles/notifications.css:93
> +        opacity: 0.2;
> +        -webkit-transition: opacity 0.5s;

Nice idea.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/styles/onebar.css:47
> +  float: right;
> +  margin: 8px;
> +  font-size: larger;

We've been using four space indent for CSS.
Comment 35 Adam Barth 2012-06-16 00:54:52 PDT
> Patch is good for review. A couple of unit tests fail (checkout: rebaseline, ui.notifications: BuildersFailing). Not sure if that's my fault or not.

Don't worry about them.  I'll take a look at these after you land your patch.

> The perf panel doesn't work with the Apple builders. Haven't figured out why not yet.

The perf panel uses http://build.chromium.org/f/chromium/perf/dashboard/overview.html.  We should probably just disable the tab on non-Chromium ports for now.  In the future, we could integrate with or link to http://webkit-perf.appspot.com/
Comment 36 Simon Fraser (smfr) 2012-06-16 21:54:09 PDT
http://trac.webkit.org/changeset/120546
Comment 37 Chris Dumez 2012-06-17 01:41:32 PDT
The following webkitpy tests are failing after this patch for both GTK and EFL ports:
webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_complex_shadowing
webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_mac_future
webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_no_add_mac_future
Comment 38 Csaba Osztrogonác 2012-06-17 23:57:44 PDT
(In reply to comment #37)
> The following webkitpy tests are failing after this patch for both GTK and EFL ports:
> webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_complex_shadowing
> webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_mac_future
> webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_no_add_mac_future

New bug to track these failures: https://bugs.webkit.org/show_bug.cgi?id=89332
Comment 39 Eric Seidel (no email) 2012-06-19 16:54:27 PDT
Super exciting!