Issue 5996 - Release consistent versions across WebExtensions

releaseAutomation.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 from __future__ import print_function
 
 import os
+import operator
 import re
 import codecs
 import logging
 import subprocess
 import sys
 import tarfile
 import json
 
 from packager import readMetadata, getDefaultFileName, get_extension
 from localeTools import read_locale_config
 
-WEBEXT_PLATFORMS = {'chrome', 'gecko'}
-WEBEXT_SOURCE_ARCHIVE = 'adblockpluswebext-{}-source.tgz'
+SOURCE_ARCHIVE = 'adblockplus-{}-source.tgz'
 
 
 def get_dependencies(prefix, repos):
     from ensure_dependencies import read_deps, safe_join
     repo = repos[prefix]
     deps = read_deps(repo)
     if deps:
         for subpath in deps:
             if subpath.startswith('_'):
                 continue
@@ skipping 81 matching lines @@
     """Run repository-checks in order to bail out early if necessary"""
     if repo_has_uncommitted():
         return False
     if repo_has_incoming(*repo_paths):
         return False
     if repo_has_outgoing():
         return continue_with_outgoing()
     return True
 
 
-def previously_released(version, downloads_repo):
-    regex = re.compile(r'^' + WEBEXT_SOURCE_ARCHIVE.format('(.*)'),
-                       re.MULTILINE)
-
-    existing = regex.findall(os.linesep.join(os.listdir(downloads_repo)))
-    return version in existing

[Wladimir Palant, 2017/11/20 15:33:17]

I consider that quite a hack, why use regular expressions in the first place?
WEBEXT_SOURCE_ARCHIVE.format(version) in os.listdir(downloads_repo) should do...

[tlucas, 2017/11/23 12:05:52]

This is replaced completely, according to our discussion in IRC.

+def compare_versions(a, b):
+    """Compare two version numbers."""
+    a_digits = [int(v) for v in a.split('.')]
+    b_digits = [int(v) for v in b.split('.')]
+
+    def safe_get(items, index):
+        return items[index] if index < len(items) else 0
+
+    for i in range(max(len(a_digits), len(b_digits))):
+        result = safe_get(a_digits, i) - safe_get(b_digits, i)
+        if result != 0:
+            return result
+    return 0
+
+
+def release_combination_is_possible(version, platforms, base_dir):
+    """Determine whether a release for the given parameters is possible.
+
+    Examine existing tags in order to find either higher or matching versions.
+    The release is impossible if a) a higher version for a requested platform
+    exists, or if b) a matching version exists and the requested set of
+    platforms differs from what was already released.
+    """
+    def higher_tag_version(tag, version, platforms):
+        return (compare_versions(tag[0], version) > 0 and
+                set(tag[1:]).intersection(platforms))
+
+    def incomplete_platforms_for_version(tag, version, platforms):
+        intersection = set(tag[1:]).intersection(platforms)
+        return (compare_versions(tag[0], version) == 0 and
+                intersection and set(platforms) != set(tag[1:]))
+
+    # only consider tags of the form "1.2[.x ...]-platform[-platform ...]
+    platform_tags = re.compile(r'^(\d+(?:(?:\.\d+)*)(?:-\w+)+)$', re.MULTILINE)
+    tags = [
+        c for c in [
+            t.split('-') for t in
+            platform_tags.findall(subprocess.check_output(
+                ['hg', 'tags', '-R', base_dir, '-q']))
+        ] if compare_versions(c[0], version) >= 0
+    ]
+
+    for tag in tags:
+        if higher_tag_version(tag, version, platforms):
+            reason = ('The higher version {} has already been released for '
+                      'the platforms {}.').format(tag[0], ', '.join(platforms))
+            return False, reason, None
+
+        if incomplete_platforms_for_version(tag, version, platforms):
+            reason = ('You have to re-release version {} for exactly all '
+                      'of: {}').format(version, ', '.join(tag[1:]))
+            return False, reason, None
+
+    return (True, None,
+            any(compare_versions(tag[0], version) == 0 for tag in tags))
 
 
 def update_metadata(metadata, version):
-    # Replace version number in metadata file "manually", ConfigParser will
-    # mess up the order of lines.
+    """Replace version number in metadata file "manually".
+
+    The ConfigParser would mess up the order of lines.
+    """
     with open(metadata.option_source('general', 'version'), 'r+b') as fp:
         rawMetadata = fp.read()
         rawMetadata = re.sub(
             r'^(\s*version\s*=\s*).*', r'\g<1>%s' % version,
             rawMetadata, flags=re.I | re.M
         )
 
         fp.seek(0)
         fp.write(rawMetadata)
         fp.truncate()
 
 
 def create_build(platform, base_dir, target_path, version, key_file=None):
-    if platform in WEBEXT_PLATFORMS:
+    """Create a build for the target platform and version."""
+    if platform == 'edge':
+        import buildtools.packagerEdge as packager
+    else:
         import buildtools.packagerChrome as packager
-    else:
-        import buildtools.packagerEdge as packager
+
     metadata = readMetadata(base_dir, platform)
     update_metadata(metadata, version)
 
     build_path = os.path.join(
         target_path,
         getDefaultFileName(metadata, version,
                            get_extension(platform, key_file is not None))

[Wladimir Palant, 2017/11/20 15:33:16]

I suggest calling these as packager.readMetadata(),
packager.getDefaultFileName() and packager.get_extension(). This is cleaner IMHO
and allows for the scenario where some packager type implements non-default
behavior.

[tlucas, 2017/11/23 12:05:51]

I agree this would be cleaner, but unfortunately there is no consistent
polymorphism in packager, packagerChrome and packagerEdge (i.e. those 3
functions are not part of packagerEdge). 'packagerChrome' is merely importing
the functions, importing them in packagerEdge as well would lead to unused
imports.
Until we might introduce some object-oriented approach for the packagers,
introducing an additional flake8-ignore would IMHO be just as "dirty".

     )
 
     packager.createBuild(base_dir, type=platform, outFile=build_path,
                          releaseBuild=True, keyFile=key_file)
 
     return build_path
 
 
 def release_commit(base_dir, extension_name, version, platforms):
+    """Create a release commit with a representative message."""
+    subprocess.check_output([
+        'hg', 'commit', '-R', base_dir, '-m',
+        'Noissue - Releasing {} {} for {}'.format(
+            extension_name, version,
+            ', '.join([p.capitalize() for p in platforms]))],
+        stderr=subprocess.STDOUT)
+
+
+def release_tag(base_dir, tag_name, extension_name):
+    """Create a tag, along with a commit message for that tag."""
     subprocess.check_call([
-        'hg', 'commit', '-R', base_dir, '-m',
-        'Releasing {} {} for {}'.format(extension_name, version,

[Wladimir Palant, 2017/11/20 15:33:16]

Since you are touching this, how about prefixing the commit message with
"Noissue - "?

[tlucas, 2017/11/23 12:05:51]

Done.

-                                        ', '.join(platforms))])

[Wladimir Palant, 2017/11/20 15:33:16]

If we want to have it extra nice, maybe map(str.capitalize) as well?

[tlucas, 2017/11/23 12:05:51]

Done.

-
-
-def release_tag(base_dir, tag_name):
-        subprocess.check_call(['hg', 'tag', '-R', base_dir, '-f', tag_name])

[Wladimir Palant, 2017/11/20 15:33:17]

Since you are touching this, how about passing '-m', 'Noissue - Adding release
tag for {} {}'

[tlucas, 2017/11/23 12:05:52]

Done.

+        'hg', 'tag', '-R', base_dir, '-f', tag_name,
+        '-m', 'Noissue - Adding release tag for {} {}'.format(
+            extension_name, tag_name)])
 
 
 def run(baseDir, platforms, version, keyFile, downloads_repo):
     if not can_safely_release(baseDir, downloads_repo):
         print('Aborting release.')
         return 1
 
-    if (WEBEXT_PLATFORMS.intersection(platforms) and
-            previously_released(version, downloads_repo)):
-        logging.error(('A release of version {} for WebExtension platforms '
-                       'was already done. Aborting.').format(version))

[Wladimir Palant, 2017/11/20 15:33:17]

I wonder why we are singling out webext here. Shouldn't we have a similar check
for build artifacts on other platforms as well?

Either way, I doubt that this check makes sense anyway. I happened a number of
times that our initial build turned out to be broken (we have some basic smoke
testing for the release build as part of the release checklist). Then we would
fix the issue and run release automation again. So just accept that the build
might be overwritten?

[tlucas, 2017/11/23 12:05:51]

As discussed in IRC, this is now handled differently. Please find the details in
the new code (commented)

+    target_platforms = sorted(platforms)
+    release_identifier = '-'.join([version] + [p for p in target_platforms])
+
+    release_possible, reason, re_release = release_combination_is_possible(
+        version, platforms, baseDir)
+
+    if not release_possible:
+        logging.error(reason)
         return 2
 
     downloads = []
     # Read extension name from first provided platform
-    locale_config = read_locale_config(baseDir, platforms[0],
-                                       readMetadata(baseDir, platforms[0]))
+    locale_config = read_locale_config(
+        baseDir, target_platforms[0],
+        readMetadata(baseDir, target_platforms[0]))
     default_locale_path = os.path.join(locale_config['base_path'],
                                        locale_config['default_locale'],
                                        'messages.json')
     with open(default_locale_path, 'r') as fp:
         extension_name = json.load(fp)['name']['message']
 
-    webext_builds = WEBEXT_PLATFORMS.intersection(platforms)
-
-    for platform in webext_builds:
+    for platform in target_platforms:
         used_key_file = None
         if platform == 'chrome':
             # Currently, only chrome builds are provided by us as signed
             # packages. Create an unsigned package in base_dir which should be
             # uploaded to the Chrome Web Store
             create_build(platform, baseDir, baseDir, version)
             used_key_file = keyFile
 
         downloads.append(
             create_build(platform, baseDir, downloads_repo, version,
                          used_key_file)
         )
 
-    if webext_builds:
-        # Only create one commit, one tag and one source archive for all
-        # WebExtension platforms
-        archive_path = os.path.join(downloads_repo,
-                                    WEBEXT_SOURCE_ARCHIVE.format(version))
-        create_sourcearchive(baseDir, archive_path)
-        downloads.append(archive_path)
-
-        release_commit(baseDir, extension_name, version, webext_builds)
-        release_tag(baseDir, version)
-
-    for platform in set(platforms).difference(WEBEXT_PLATFORMS):
-        # Create build for platforms not yet using WebExtension
-        downloads.append(
-            create_build(platform, baseDir, downloads_repo, version,
-                         keyFile)
-        )
-
-        archivePath = os.path.splitext(downloads[-1])[0] + '-source.tgz'
-        create_sourcearchive(baseDir, archivePath)
-        downloads.append(archivePath)
-
-        release_commit(baseDir, extension_name, version, {platform})
-        release_tag(baseDir, '{}-{}'.format(version, platform))

[Wladimir Palant, 2017/11/20 15:33:17]

What if we release for Firefox, Chrome and Edge at the same time? There will be
a release commit for Firefox and Chrome above, the release_commit() call here
however will say "nothing changed" (new version number was committed already).
Two tags don't make a lot of sense either in that case.

I'd say, there should always be only one commit and only one tag per release. In
fact, there should probably be only one source file as well - no point uploading
two identical source files for Chrome/Firefox and Edge. Note that our Edge
extension is also a Web Extension, even though it's currently developed on a
branch.

Let's also consider the possibility that we will get an Edge release from the
branch with an Edge-specific version number. I guess that having release tags
contain the platform identifiers (sorted alphabetically) is the way to go:
"3.0.3-chrome-firefox" and "0.9.12-edge". It's probably best to apply the same
approach to the source files rather than using the generic "webext"
pseudo-platform: "adblockplus-chrome-firefox-3.0.2-source.tgz". This will still
work if we decide to have a different 3.0.2 release for Edge.

[tlucas, 2017/11/23 12:05:51]

As discussed in IRC, this is now handled differently. Please find the details in
the new code (commented)

+    # Only create one commit, one tag and one source archive for all
+    # platforms
+    archive_path = os.path.join(
+        downloads_repo,
+        'adblockplus-{}-source.tgz'.format(release_identifier),
+    )
+    create_sourcearchive(baseDir, archive_path)
+    downloads.append(archive_path)
+    try:
+        release_commit(baseDir, extension_name, version, target_platforms)
+    except subprocess.CalledProcessError as e:
+        if not (re_release and 'nothing changed' in e.output):
+            raise
+
+    release_tag(baseDir, release_identifier, extension_name)
 
     # Now add the downloads and commit
     subprocess.check_call(['hg', 'add', '-R', downloads_repo] + downloads)
-    subprocess.check_call([
-        'hg', 'commit', '-R', downloads_repo, '-m',
-        'Releasing {} {} for {}'.format(extension_name, version,
-                                        ', '.join(platforms))])
+    release_commit(downloads_repo, extension_name, version, target_platforms)
 
     # Push all changes
     subprocess.check_call(['hg', 'push', '-R', baseDir])
     subprocess.check_call(['hg', 'push', '-R', downloads_repo])