Issue 5996 - Release consistent versions across WebExtensions

releaseAutomation.py

Index: releaseAutomation.py
diff --git a/releaseAutomation.py b/releaseAutomation.py
index f9d6405c9605854322d35f8c0d69ffccc26ba584..3cd677e0163ee9b809f0a0d4bb57248eda2b7a40 100644
--- a/releaseAutomation.py
+++ b/releaseAutomation.py
@@ -7,11 +7,17 @@ from __future__ import print_function
 import os
 import re
 import codecs
+import logging
 import subprocess
+import sys
 import tarfile
 import json
 
-from packager import readMetadata, getDefaultFileName
+from packager import readMetadata, getDefaultFileName, get_extension
+from localeTools import read_locale_config
+
+WEBEXT_PLATFORMS = {'chrome', 'gecko'}
+WEBEXT_SOURCE_ARCHIVE = 'adblockpluswebext-{}-source.tgz'
 
 
 def get_dependencies(prefix, repos):
@@ -113,68 +119,129 @@ def can_safely_release(*repo_paths):
     return True
 
 
-def run(baseDir, type, version, keyFile, downloadsRepo):
-    if not can_safely_release(baseDir, downloadsRepo):
-        print('Aborting release.')
-        return 1
+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.

 
-    if type == 'edge':
-        import buildtools.packagerEdge as packager
-    elif type == 'chrome':
-        import buildtools.packagerChrome as packager
 
-    # Replace version number in metadata file "manually", ConfigParser will mess
-    # up the order of lines.
-    metadata = readMetadata(baseDir, type)
-    with open(metadata.option_source('general', 'version'), 'r+b') as file:
-        rawMetadata = file.read()
+def update_metadata(metadata, version):
+    # Replace version number in metadata file "manually", ConfigParser will
+    # 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
         )
 
-        file.seek(0)
-        file.write(rawMetadata)
-        file.truncate()
+        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:
+        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
+
 
-    # Read extension name from locale data
-    default_locale_path = os.path.join('_locales', packager.defaultLocale,
+def release_commit(base_dir, extension_name, version, platforms):
+    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.

+
+
+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)

+        return 2
+
+    downloads = []
+    # Read extension name from first provided platform
+    locale_config = read_locale_config(baseDir, platforms[0],
+                                       readMetadata(baseDir, 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:
-        extensionName = json.load(fp)['name']['message']
+        extension_name = json.load(fp)['name']['message']
+
+    webext_builds = WEBEXT_PLATFORMS.intersection(platforms)
+
+    for platform in webext_builds:
+        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)
+        )
 
-    # Now commit the change and tag it
-    subprocess.check_call(['hg', 'commit', '-R', baseDir, '-m', 'Releasing %s %s' % (extensionName, version)])
-    tag_name = version
-    if type == 'edge':
-        tag_name = '{}-{}'.format(tag_name, type)
-    subprocess.check_call(['hg', 'tag', '-R', baseDir, '-f', tag_name])
+    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)
+        )
 
-    # Create a release build
-    downloads = []
-    if type == 'chrome':
-        # Create both signed and unsigned Chrome builds (the latter for Chrome Web Store).
-        buildPath = os.path.join(downloadsRepo, getDefaultFileName(metadata, version, 'crx'))
-        packager.createBuild(baseDir, type=type, outFile=buildPath, releaseBuild=True, keyFile=keyFile)
-        downloads.append(buildPath)
-
-        buildPathUnsigned = os.path.join(baseDir, getDefaultFileName(metadata, version, 'zip'))
-        packager.createBuild(baseDir, type=type, outFile=buildPathUnsigned, releaseBuild=True, keyFile=None)
-    elif type == 'edge':
-        # We only offer the Edge extension for use through the Windows Store
-        buildPath = os.path.join(downloadsRepo, getDefaultFileName(metadata, version, 'appx'))
-        packager.createBuild(baseDir, type=type, outFile=buildPath, releaseBuild=True)
-        downloads.append(buildPath)
-
-    # Create source archive
-    archivePath = os.path.splitext(buildPath)[0] + '-source.tgz'
-    create_sourcearchive(baseDir, archivePath)
-    downloads.append(archivePath)
+        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)

 
     # Now add the downloads and commit
-    subprocess.check_call(['hg', 'add', '-R', downloadsRepo] + downloads)
-    subprocess.check_call(['hg', 'commit', '-R', downloadsRepo, '-m', 'Releasing %s %s' % (extensionName, version)])
+    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))])
 
     # Push all changes
     subprocess.check_call(['hg', 'push', '-R', baseDir])
-    subprocess.check_call(['hg', 'push', '-R', downloadsRepo])
+    subprocess.check_call(['hg', 'push', '-R', downloads_repo])