Closed Bug 945390 Opened 7 years ago Closed 7 years ago

Autophone - support build selection via revision

Categories

(Testing :: Autophone, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bc, Assigned: bc)

References

Details

Attachments

(4 files)

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.
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.
Attachment #8344681 - Flags: review?(mcote)
Attachment #8344681 - Flags: feedback?(wlachance)
Attached file test script test.sh
I used this script to test the changes.
Attached file test.log
output from test.sh
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.
Attachment #8344681 - Flags: feedback?(wlachance) → feedback+
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".
Attachment #8344681 - Flags: review?(mcote)
Attachment #8344681 - Flags: review+
Attachment #8344681 - Flags: feedback?
Attachment #8344681 - Flags: feedback+
This is what I checked in.
https://github.com/mozilla/autophone/commit/c9ec58c8b0eed2f38452842debf84d7b85f60565
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 948628
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.
Attachment #8344681 - Flags: feedback?
You need to log in before you can comment on or make changes to this bug.