Bug 77725 - perf-o-matic should store chromium svn revision
Summary: perf-o-matic should store chromium svn revision
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: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 77037
  Show dependency treegraph
 
Reported: 2012-02-03 01:59 PST by Ryosuke Niwa
Modified: 2012-02-08 03:40 PST (History)
6 users (show)

See Also:


Attachments
Adds the feature (15.53 KB, patch)
2012-02-03 02:07 PST, Ryosuke Niwa
dpranke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-02-03 01:59:34 PST
Right now, the graph server only stores WebKit's revision. However, for chromium port, it'll be also useful to store chromium revision.

This is also a requirement if we were to use the graph server to view results for Chromium perf bots.
Comment 1 Ryosuke Niwa 2012-02-03 02:07:12 PST
Created attachment 125294 [details]
Adds the feature
Comment 2 Dirk Pranke 2012-02-03 10:46:10 PST
Comment on attachment 125294 [details]
Adds the feature

looks good to me. adam, eric, or ojan might want to glance at it as well.
Comment 3 Eric Seidel (no email) 2012-02-03 11:05:27 PST
Comment on attachment 125294 [details]
Adds the feature

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

> Tools/Scripts/webkitpy/common/checkout/scm/scm.py:188
> +    def svn_revision(self, path):
>          self._subclass_must_implement()

Feels a little odd that this doesn't use any self state.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:906
> +    def repository_paths(self):
> +        """Returns a list of (repository_name, repository_path) tuples of its depending code base.
> +        By default it returns a list that only contains a ('webkit', <webkitRepossitoryPath>) tuple."""
> +        return [('webkit', self.webkit_base())]

Odd that this belongs on base, since multi-checkouts are a chromium-only concept.
Comment 4 Dirk Pranke 2012-02-03 12:46:32 PST
Comment on attachment 125294 [details]
Adds the feature

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

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:906
>> +        return [('webkit', self.webkit_base())]
> 
> Odd that this belongs on base, since multi-checkouts are a chromium-only concept.

Seems like returning a list is a generic concept since the caller doesn't know if the port uses one or more checkouts. Maybe I'm not following you?
Comment 5 Ryosuke Niwa 2012-02-03 13:07:01 PST
Comment on attachment 125294 [details]
Adds the feature

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:340
> +        repos.append(('chromium', self.path_from_chromium_base()))

Oops, a slight fix needed here. Because Source/WebKit/chromium is still in the WebKit repository, it won't work inside WebKit checkout (i.e. chromium-revision will be same as webkit-revision). I'll use self.path_from_chromium_base('build') instead.
Comment 6 Ryosuke Niwa 2012-02-03 13:10:26 PST
Committed r106687: <http://trac.webkit.org/changeset/106687>
Comment 7 Ryosuke Niwa 2012-02-08 03:40:59 PST
Build fix landed in http://trac.webkit.org/changeset/107064.