945390 - Autophone - support build selection via revision

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

mozregression has a cool feature that allows the selection of build by revision:
https://github.com/mozilla/mozregression/blob/master/mozregression/inboundfinder.py#L26

I would like this in Autophone as well since phonedash reports revision when clicking on a data point.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-945390-build-revision.patch v1

This patch removes httplib2 in favor of urllib2 and takes some code and inspiration from mozregression.

I added a class BuildLocation and have Nightly, Tinderbox, InboundArchive inherit from that. I've moved some methods from BuildCache to BuildLocation and forward the calls from BuildCache to BuildLocation.

I've parameterized the product and platform information in BuildLocation which should be easily modified to support other products and platforms. Autophone is somewhat different from other cases in that it runs on a different platform than the test machines and that it may have multiple architectures, arm, armv6, x86, so I don't use mozinfo.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: test script test.sh

I used this script to test the changes.

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: test.log

output from test.sh

Comment 4

[William Lachance (:wlach)]

Comment on attachment 8344681 [details] [diff] [review]
bug-945390-build-revision.patch v1

Review of attachment 8344681 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty reasonable from a 10 minute overview. Hopefully we can incorporate some of these ideas into mozdownload or some other package.

Comment 5

[Mark Côté [:mcote]]

Comment on attachment 8344681 [details] [diff] [review]
bug-945390-build-revision.patch v1

Review of attachment 8344681 [details] [diff] [review]:
-----------------------------------------------------------------

This is nice!  I am excited to see it integrated into a mozbase package (or become its own package).

::: builds.py
@@ +29,5 @@
> +    'mozilla-aurora': 'http://hg.mozilla.org/releases/mozilla-aurora/',
> +    'mozilla-beta': 'http://hg.mozilla.org/releases/mozilla-beta/',
> +    'mozilla-inbound': 'http://hg.mozilla.org/integration/mozilla-inbound/'}
> +
> +urls_repos = dict([[url, repo] for repo, url in repo_urls.items()])

Nit: use tuples for (url, repo), since you aren't modifying it.

@@ +32,5 @@
> +
> +urls_repos = dict([[url, repo] for repo, url in repo_urls.items()])
> +
> +# lifted from mozregression:utils.py:urlLinks
> +def urlLinks(url):

Nit: since this function is just being copied over, might as well follow the standard naming convention and call this url_links().

@@ +51,5 @@
> +    soup = BeautifulSoup(content)
> +    # do not return a generator but an array, so we can store it for later use
> +    return [link for link in soup.findAll('a')
> +            if not link.get('href').startswith('?') and
> +            link.get_text() != 'Parent Directory' ]

Nit: extra space before ].

@@ +53,5 @@
> +    return [link for link in soup.findAll('a')
> +            if not link.get('href').startswith('?') and
> +            link.get_text() != 'Parent Directory' ]
> +
> +def parsedatetime(stringval):

Nit: parse_datetime()?

Aside from that, this function is actually pretty clear considering the variety of values it has to deal with!

What do you think about splitting all the build id/time functions into their own file?

@@ +77,5 @@
> +        floatval = float(stringval)
> +        timestamp = time.mktime(datetime.datetime.now().timetuple())
> +        if floatval > timestamp:
> +            # 20131201030203 - buildid
> +            format = 'buildid'

It might make sense to define these earlier on as global variables to prevent typos, e.g. BUILDID = 'buildid'.  If you accidentally type 'BUILDI' somewhere later on, you'll at least get an exception instead of a potentially silent error somewhere down the line.

@@ +165,5 @@
> +        return None
> +
> +    try:
> +        dateval = datetime.datetime.strptime(buildid, "%Y%m%d%H%M%S")
> +        return  set_time_zone(dateval)

Nit: extra space after "return".

@@ +184,5 @@
> +        return set_time_zone(dateval)
> +    except (TypeError, ValueError):
> +        return None
> +
> +def get_revision_timestamps(repo, first_revision, second_revision):

I think "last_revision" would be better than "second_revision", to imply this is a range.

@@ +210,5 @@
> +        push = pushlog[pushid]
> +        revisions.append((push['changesets'][-1], push['date']))
> +
> +    revisions.sort(key=lambda r: r[1])
> +    if not revisions:

Might as well move this check up.

@@ +239,5 @@
> +        buildfile_pattern = buildfile_pattern.rstrip('|')
> +        buildfile_pattern += ')'
> +        self.buildfile_regex = re.compile(buildfile_pattern)
> +        self.build_regex = re.compile("(%s%s)" % (buildfile_pattern,
> +                                                self.buildfile_ext))

Horizontal alignment.

@@ +264,5 @@
> +                         self.buildtxt_regex.pattern))
> +
> +    def get_search_directories_by_time(self, start_time, end_time):
> +        raise NotImplementedError()
> +        yield None, None

This (and other yields below) will never be called... Put in docstrings if you are trying to document the return values.

@@ +285,5 @@
> +            return None
> +        # Return the most recent builds for each of the architectures.
> +        # The phones will weed out the unnecessary architectures.
> +        builds.sort()
> +        builds.reverse()

Just realized these two lines could also be combined as 'builds.sort(reverse=True)'.

@@ +290,5 @@
> +        multiarch_builds = []
> +        # check the longest strings first
> +        platforms = self.build_platforms
> +        platforms.sort(key=len)
> +        platforms.reverse()

Same.

@@ +324,5 @@
>  
> +                if not build_time:
> +                    continue
> +
> +                if build_time and (build_time < start_time or

No need for the truthy build_time check, since it continues if not build_time above.

@@ +341,5 @@
> +            logger.error('No builds found.')
> +        return builds
> +
> +    def find_builds_by_revision(self, first_revision, second_revision):
> +        logger.debug('Finding build between revisions %s and %s' %

Nit: "Finding builds" (plural).

@@ +363,5 @@
> +                             second_revision, second_datetime))
> +            if not first_datetime or not second_datetime:
> +                continue
> +            for search_directory_repo, search_directory in self.get_search_directories_by_time(first_datetime,
> +                                                                                 second_datetime):

Horizontal spacing.

@@ +381,5 @@
> +
> +                for link in urlLinks(search_directory):
> +                    try:
> +                        datetimestring = link.get('href').strip('/')
> +                        if isinstance(self, Nightly) and repo not in datetimestring:

Ick, explicitly referencing a derived class in a base class is yucky and bug prone.  I would make this a generic check of some sort, maybe a noop in the base class that is overridden in Nightly.

@@ +410,5 @@
> +
> +                start_time = None
> +                end_time = None
> +                for datetimestamp in datetimestamps:
> +                    # for each datetimestamp 

Don't think this comment is necessary.

::: trigger_runs.py
@@ +59,2 @@
>      build_urls = []
> +    if len(args) == 0:

Nit: normally this would be "if not args".

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: bug-945390-build-revision v2

This is what I checked in.

Comment 7

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/c9ec58c8b0eed2f38452842debf84d7b85f60565

Comment 8

[Mark Côté [:mcote]]

Comment on attachment 8344681 [details] [diff] [review]
bug-945390-build-revision.patch v1

Review of attachment 8344681 [details] [diff] [review]:
-----------------------------------------------------------------

Already r+ed so un-f?ing myself.