Issue 2432 - Auto-generate browser versions on requirements page

globals/get_browser_versions.py

 import re
 import os
 import sys
 import json
 import urllib2
 import errno
+import logging
 from xml.dom import minidom
 
 from jinja2 import contextfunction
+
+BROWSERS = {}
 
 CHROME_UPDATE_XML = '''\
 <?xml version="1.0" encoding="UTF-8"?>
 <request protocol="3.0" ismachine="0">
   <os platform="win" version="99" arch="x64"/>
   <app appid="{4DC8B4CA-1BDA-483E-B5FA-D3C12E15B62D}">
     <updatecheck/>
   </app>
   <app appid="{4DC8B4CA-1BDA-483E-B5FA-D3C12E15B62D}" ap="x64-beta-multi-chrome">
     <updatecheck/>
   </app>
   <app appid="{4DC8B4CA-1BDA-483E-B5FA-D3C12E15B62D}" ap="x64-dev-multi-chrome">
     <updatecheck/>
   </app>
 </request>'''
 
-def get_mozilla_update(subdomain, product, version, build, channel):
-  response = urllib2.urlopen('https://%s.mozilla.org/update/3/%s/%s/%s/WINNT_x86-msvc/en-US/%s/-/default/default/update.xml?force=1' % (subdomain, product, version, build, channel))
+def get_mozilla_version(product, origin_version, channel,
+                        minor=False, subdomain='aus4', origin_build='-',
+                        attribute='appVersion', platform='WINNT_x86-msvc'):
+  response = urllib2.urlopen('https://%s.mozilla.org/update/3/%s/%s/%s/%s/en-US/%s/-/default/default/update.xml?force=1' % (
+    subdomain,
+    product,
+    origin_version,
+    origin_build,
+    platform,
+    channel
+  ))
   try:
     doc = minidom.parse(response)
   finally:
     response.close()
 
-  return doc.getElementsByTagName('update')[0]

[Wladimir Palant, 2015/04/30 15:34:47]

Unfortunately, you forgot my comment on code locality on the previous changeset.
With this patch it got worse - related code isn't grouped together.

[Sebastian Noack, 2015/04/30 20:26:20]

Here you have your code locality. ;P

[Wladimir Palant, 2015/05/13 13:17:23]

Thank you, much better.

-
-def get_mozilla_version(product, version, channel):
-  update = get_mozilla_update('aus4', product, version, '-', channel)
-  return update.getAttribute('appVersion').split('.')[0]
-
-def get_mozilla_versions(product, version):
-  return {
-    'current': get_mozilla_version(product, version, 'release'),
+  update = doc.getElementsByTagName('update')[0]
+  full_version = update.getAttribute(attribute)
+
+  match = re.search(r'^(\d+)(?:\.\d+)?', full_version)
+  if minor:
+    return match.group(0)
+  return match.group(1)
+
+def get_mozilla_versions(product, origin_version, release_minor=False):
+  return {
+    'current': get_mozilla_version(product, origin_version, 'release', release_minor),
     'unreleased': [
-      get_mozilla_version(product, version, 'beta'),
-      get_mozilla_version(product, version, 'aurora'),
-      get_mozilla_version(product, version, 'nightly'),
+      get_mozilla_version(product, origin_version, 'beta'),
+      get_mozilla_version(product, origin_version, 'aurora'),
+      get_mozilla_version(product, origin_version, 'nightly'),
     ]
   }
 
-def get_seamonkey_version(channel, build):
-  update = get_mozilla_update('aus2-community', 'SeaMonkey', '2.32', build, channel)

[Wladimir Palant, 2015/04/30 15:34:47]

Build ID is tied to the version number - if you are passing in build ID then
version number should be passed in as well.

[Sebastian Noack, 2015/04/30 20:26:20]

Well, I just tried to don't repeat myself. But it doesn't seem to be a big deal
here.

-  return re.search(r'^^\d+\.\d+', update.getAttribute('version')).group(0)
+BROWSERS['firefox'] = lambda: get_mozilla_versions('Firefox', '37.0')
+BROWSERS['thunderbird'] = lambda: get_mozilla_versions('Thunderbird', '31.0', True)
+
+def get_seamonkey_version(origin_version, origin_build, channel, **kw):
+  return get_mozilla_version('SeaMonkey', origin_version, channel, True,
+                             'aus2-community', origin_build, 'version', **kw)
+
+def get_seamonkey_versions():
+  return {
+    'current': get_seamonkey_version('2.32', '20150112201917', 'release'),
+    'unreleased': [
+      get_seamonkey_version('2.32', '20150101215737', 'beta'),
+
+      # Aurora and Nightly builds for Windows are currently broken.
+      # https://bugzilla.mozilla.org/show_bug.cgi?id=1086553
+      get_seamonkey_version('2.32', '-', 'aurora', platform='Linux_x86-gcc3'),
+      get_seamonkey_version('2.32', '-', 'nightly', platform='Linux_x86-gcc3')
+    ]
+  }
+
+BROWSERS['seamonkey'] = get_seamonkey_versions
 
 def get_chrome_version(manifest):
   return manifest.getAttribute('version').split('.')[0]
 
+def get_chrome_versions():
+  response = urllib2.urlopen(urllib2.Request('https://tools.google.com/service/update2', CHROME_UPDATE_XML))
+  try:
+    doc = minidom.parse(response)
+  finally:
+    response.close()
+
+  manifests = doc.getElementsByTagName('manifest')
+  return {
+    'current': get_chrome_version(manifests[0]),
+    'unreleased': map(get_chrome_version, manifests[1:])
+  }
+
+BROWSERS['chrome'] = get_chrome_versions
+
 def get_opera_version(channel):
   response = urllib2.urlopen('https://autoupdate.geo.opera.com/netinstaller/' + channel)
   try:
     spec = json.load(response)
   finally:
     response.close()
 
   return re.search(r'\d+', spec['installer_filename']).group(0)
+
+def get_opera_versions():
+  return {
+    'current': get_opera_version('Stable'),
+    'unreleased': [
+      get_opera_version('Beta'),
+      get_opera_version('Developer')
+    ]
+  }
+
+BROWSERS['opera'] = get_opera_versions
 
 def get_yandex_version(suffix):
   response = urllib2.urlopen('https://api.browser.yandex.ru/update-info/browser/yandex%s/win-yandex.xml' % suffix)
   try:
     doc = minidom.parse(response)
   finally:
     response.close()
 
   item = doc.getElementsByTagName('item')[0]
   description = item.getElementsByTagName('description')[0]
   return re.search(r'\d+\.\d+', description.firstChild.nodeValue).group(0)
 
-def open_cache_file(source):
-  filename = os.path.join(source.get_cache_dir(), 'browsers.json')
+def get_yandex_versions():
+  return {
+    'current': get_yandex_version(''),
+    'unreleased': [get_yandex_version('-beta')]
+  }
+
+BROWSERS['yandex'] = get_yandex_versions
+
+def open_cache_file(filename):
   flags = os.O_RDWR | os.O_CREAT
   try:
     fd = os.open(filename, flags)
   except OSError as e:
     if e.errno != errno.ENOENT:
       raise
     os.makedirs(os.path.dirname(filename))
     fd = os.open(filename, flags)
   return os.fdopen(fd, 'w+')
 
-class BrowserVersions:
-  def get_firefox_versions(self):
-    return get_mozilla_versions('Firefox', '37.0')
-
-  def get_thunderbird_versions(self):
-    return get_mozilla_versions('Thunderbird', '31.0')
-
-  def get_seamonkey_versions(self):
-    return {
-      'current': get_seamonkey_version('release', '20150112201917'),
-      'unreleased': [get_seamonkey_version('beta', '20150101215737')]

[Wladimir Palant, 2015/04/30 15:34:47]

What about Aurora and Nightly? Note that these update channels don't seem to
care about build IDs.

[Sebastian Noack, 2015/04/30 20:26:20]

They do not exist.

[Wladimir Palant, 2015/05/13 13:17:23]

That would be news to me.

https://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/
https://aus2-community.mozilla.org/update/3/SeaMonkey/2.13.1/20120909051705/L...
https://aus2-community.mozilla.org/update/3/SeaMonkey/2.13.1/20120909051705/L...

[Sebastian Noack, 2015/05/13 18:35:11]

You seem to be right. But for reference, Aurora builds for Windows and nightlies
for all platforms are currently broken. I considered that case in the code.

However currently, this gets 2.33 both as stable and beta (seems to be right
according to Wikipedia) and 2.36 as Aurora. But I have no idea where the gap is
coming from. Even if beta is supposed to be 2.34, shouldn't Aurora be not higher
than 2.35? IMO, the whole situation with SeaMonkey versions is currently
extremly confusing, to users reading our requirements page, as well for
developers debugging this code.

So maybe we should just list the current (and previous) release version of
SeaMonkey, adding a note below, along the lines of:

"The Adblock Plus development builds should work as well with SeaMonkey Beta,
Aurora and Nightly. However, SeaMonkey pre-releases aren't available as the time
of writing due to Bug 1086553."

[Wladimir Palant, 2015/05/14 19:53:19]

Firefox 37 is the current stable version - that would correspond to SeaMonkey
2.34 (it's always "2.{ff_version - 3}"). Aurora being SeaMonkey 2.36 makes sense
then. The strange part is rather that SeaMonkey 2.34 isn't released yet and that
their beta channel is outdated. According to their Wiki
(https://wiki.mozilla.org/SeaMonkey:Home_Page) the 2.34 release has been
canceled due to issues with their build infrastructure
(https://bugzilla.mozilla.org/show_bug.cgi?id=1114876). The same issue seems to
be responsible for missing beta builds
(https://groups.google.com/forum/#!topic/mozilla.support.seamonkey/d95E6knhHTM).
It should be fine if we only list the versions that one can update to. I don't
see a big point for this message.

[Sebastian Noack, 2015/05/15 10:24:54]

Nighties are back. Now, we get following versions:

2.33 (Release and Beta)
2.36 (Aurora)
2.38 (Nightly)

Does that sound right?

-    }
-
-  def get_chrome_versions(self):
-    response = urllib2.urlopen(urllib2.Request('https://tools.google.com/service/update2', CHROME_UPDATE_XML))
+@contextfunction
+def get_browser_versions(context, browser):
+  func = BROWSERS[browser]
+  exc_info = None
+  try:
+    versions = func()
+  except Exception:
+    exc_info = sys.exc_info()
+
+  filename = os.path.join(context['source'].get_cache_dir(), 'browsers.json')
+  with open_cache_file(filename) as file:
     try:
-      doc = minidom.parse(response)
-    finally:
-      response.close()
-
-    manifests = doc.getElementsByTagName('manifest')
-    return {
-      'current': get_chrome_version(manifests[0]),
-      'unreleased': map(get_chrome_version, manifests[1:])
-    }
-
-  def get_opera_versions(self):
-    return {
-      'current': get_opera_version('Stable'),
-      'unreleased': [
-        get_opera_version('Beta'),
-        get_opera_version('Developer')
-      ]
-    }
-
-  def get_yandex_versions(self):
-    return {
-      'current': get_yandex_version(''),
-      'unreleased': [get_yandex_version('-beta')]
-    }
-
-  @contextfunction
-  def get_versions(self, context, browser):
-    method = getattr(self, 'get_%s_versions' % browser)
-    exception = None
-    try:
-      versions = method()
-    except Exception as e:
-      exception = e
-
-    with open_cache_file(context['source']) as file:
-      try:
-        cache = json.load(file)
-      except ValueError:
-        if file.tell() > 0:
-          raise
-        cache = {}

[Wladimir Palant, 2015/04/30 15:34:47]

Honestly, that's an extremely complicated way of doing this. My understanding is
that the idea was the following:

cachedir = source.get_cache_dir()
try:
  os.makedirs(cachedir)
except OSError:
  pass

filename = os.path.join(cachedir, 'browsers.json')
try:
  file = open(filename, 'r+')
except OSError:
  file = open(filename, 'w')

with file:
  cache = json.load(file) if file.mode == 'r+' else {}

No messing with file descriptors, no retries, no attempts to distinguish JSON
reading from a newly created file...

[Sebastian Noack, 2015/04/30 20:26:20]

Yes your approach is slightly simpler, but IMO not great. If you want to open a
file for reading and writing, creating it if it doesn't exist, using
O_RDWR|O_CREAT is exactly how you are supposed to do it. No unnecessary
round-trips to the kernel, no race conditions, and not even more complex than
the alternative:

  os.fdopen(os.open(filename, os.O_RDWR | os.O_CREAT), 'w+')

vs.

  try:
    file = open(filename, 'r+')
  except OSError:
    file = open(filename, 'w')

It can be argued whether attempting to create the directory before the file, or
catching ENOENT when opening the file is prefereable. I prefer the latter, we
already used that pattern a couple of times in our code and the complexity added
here is neglectable.

But how is a bogus check for the file mode based on a previous branch, to detect
an empty file any better than checking the position of the file after a failed
attempt of reading it? Not to mention that I'm not a fan of the logic switching
the modes in the first place.

-
-      cached_versions = cache.get(browser)
-      if exception:
-        if not cached_versions:
-          raise exception
-
-        print >>sys.stderr, "Warning: Failed to get %s versions, falling back to cached versions" % browser

[Wladimir Palant, 2015/04/30 15:34:47]

Use logging.warning?

[Sebastian Noack, 2015/04/30 20:26:20]

I almost used the logging module. But then I looked into the CMS itself, because
I want to generate consistent output. And this is apparently the way we log
warning in the CMS.

[Sebastian Noack, 2015/05/05 13:34:35]

Done, after making the cms use the logging module as well.

While on it, I also added another warning when the previous version can't be
determined.

-        return cached_versions
-
+      cache = json.load(file)
+    except ValueError:
+      if file.tell() > 0:
+        raise
+      cache = {}
+
+    cached_versions = cache.get(browser)
+    if exc_info:
+      if not cached_versions:
+        raise exc_info[0], exc_info[1], exc_info[2]
+
+      versions = cached_versions
+      logging.warning('Failed to get %s versions, falling back to '
+                      'cached versions', browser, exc_info=exc_info)
+    else:
       # Determine previous version: If we recorded the version before and it
       # changed since then, the old current version becomes the new previous
       # version. If the version didn't change, use the cached previous version.
       current = versions['current']
+      previous = None
       if cached_versions:
         cached_current = cached_versions['current']
         if cached_current != current:
-          versions['previous'] = cached_current
+          previous = cached_current
         else:
-          cached_previous = cached_versions.get('previous')
-          if cached_previous:
-            versions['previous'] = cached_previous

[Wladimir Palant, 2015/04/30 15:34:47]

This algorithm won't work for Thunderbird. Here the releases go like this:
31.6.0 => 38.0 => 38.1.0 => 38.2.0. Maybe we should just include the second
version number part for Thunderbird stable meaning that we'll list 38.1 and 38.2
as supported once 38.2 comes out.

Also a nit:

if cached_current != current:
  ...
elif 'previous' in cached_versions:
  versions['previous'] = cached_versions['previous']

It must hurt to do two lookups instead of one but the current code really makes
the logic harder to follow ;)

[Sebastian Noack, 2015/04/30 20:26:20]

Alternatively, we could stop listening the previous major version, when beta is
two major versions ahead of stable? This should match what we currently list on
the requirements page. I don't mind.
Yes it hurts, and makes the code here inconsistent.

[Sebastian Noack, 2015/04/30 22:39:41]

Never mind. I found a way to include the minor version for Thunderbird stable
releases, I'm quite happy with.

[Sebastian Noack, 2015/05/05 13:34:35]

I made this and some other checks obsolete, by always setting "previous" (to
None if it can't be determined).

-
-      # Remove duplicated from unreleased versions. Occasionally,
+          previous = cached_versions['previous']
+      versions['previous'] = previous
+
+      # Remove duplicates from unreleased versions. Occasionally,
       # different channels are on the same version, but we want
       # to list each version only once.
-      unreleased = versions['unreleased']
-      previous = versions.get('previous')
-      for i, version in list(enumerate(unreleased))[::-1]:
-        if version == current or previous and version == previous:
-          del unreleased[i]

[Wladimir Palant, 2015/04/30 15:34:47]

I doubt that this will work correctly if you need to remove more than one
element. Either way, this seems to be way simpler:

versions['unreleased'] = sorted(set(versions['unreleased']) - {current,
previous})

[Sebastian Noack, 2015/04/30 20:26:20]

It does work correctly for any number of elements, because I iterate from the
end to the beginning of the list. Hence if we remove an item the next item is
still at current index - 1.
Without a fairly complex sort function, this will potentially mess up the order,
e.g: sorted(['2.2', '2.10']) -> ['2.10', '2.2']

Using a sorted set, this code would rather look like:

  unreleased = set(versions['unreleased'])
  unreleased -= {current}
  previous = versions.get('previous')
  if previous:
    unreleased -= {previous}
  versions['unreleased'] = sorted(unreleased, key=lambda ver: [
    int(num) if num else s for num, s
    in re.findall(r'(\d+)|([a-z]+)', ver, re.I)
  ])

However, it can be simplified if we assume that that we only have strings
consisting of dots and digits, but I'm not sure whether this is a good idea:

  unreleased = set(versions['unreleased'])
  unreleased -= {current, versions.get('previous')}
  versions['unreleased'] = sorted(unreleased,
                                  key=lambda ver: map(int, ver.split('.'))

[Sebastian Noack, 2015/05/05 13:34:35]

Never mind, I am using a set now. As this will not only simplify the logic, but
also make sure that there are no duplicates among the unreleased versions.

+      versions['unreleased'] = sorted(
+        set(versions['unreleased']) - {current, previous},
+        key=lambda ver: map(int, ver.split('.'))
+      )
 
       cache[browser] = versions
       file.seek(0)

[Wladimir Palant, 2015/04/30 15:34:47]

Truncate file?

[Sebastian Noack, 2015/04/30 20:26:20]

Done.

       json.dump(cache, file)
-
-    return versions
-
-get_browser_versions = BrowserVersions().get_versions
+      file.truncate()
+
+  if not versions['previous']:
+    logging.warning("Couldn't determine previous browser version, "
+                    'please set %s.previous in %s', browser, filename)
+
+  return versions