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
@@ skipping 101 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 version_is_op_to(value, orig, op):

[Wladimir Palant, 2017/11/28 11:01:16]

For reference, sitescripts.extensions.utils has another version comparison
function. It's probably going to be an overkill here, but the function you
created here could definitely be simpler.

Typical comparison functions return a number - negative, zero or positive. This
makes comparisons simpler:

  def safe_get(list, index):
    return int(list[index]) if index < len(list) else 0

  for i in range(len(value_digits)):
    result = safe_get(value_digits, i) - safe_get(orig_digits, i)
    if result != 0:
      return result
  return 0

Nit: I consider value and orig bad parameter names for a comparison function,
these imply a special meaning of the passed in values. version1 and version2 or
a and b would be better.

[tlucas, 2017/11/28 13:28:43]

Done.

-    """Compare two version numbers (left to right) with a given operator.
-
-    If a passed version contains non-numeric values, the comparison is always
-    False.
-    """
-    def prefix_0(a, b):
-        target_len = max((len(a), len(b)))
-        a = '0' * (target_len - len(a)) + a
-        b = '0' * (target_len - len(b)) + b
-        return a, b
-
-    try:
-        value_digits = [int(v) for v in value.split('.')]
-        orig_digits = [int(v) for v in orig.split('.')]
-
-        value_dec = ''
-        orig_dec = ''
-
-        if len(value_digits) < len(orig_digits):
-            value_digits += [0] * (len(orig_digits) - len(value_digits))
-        elif len(orig_digits) < len(value_digits):
-            orig_digits += [0] * (len(value_digits) - len(orig_digits))
-
-        for i in range(len(value_digits)):
-            val, orig = prefix_0(str(value_digits[i]), str(orig_digits[i]))
-            value_dec += val
-            orig_dec += orig
-
-        return op(int(value_dec), int(orig_dec))
-
-    except ValueError:
-        pass

[Wladimir Palant, 2017/11/28 11:01:16]

How about only passing in tags that match r'^\d+(\.\d+)$' regular expression
instead of catching errors?

[tlucas, 2017/11/28 13:28:43]

Done, respectively only passed tags of the form "1.2.3[-chrome|-gecko|-edge]+"

-
-    return False
+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):

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

This docstring reflects what was discussed in IRC.

     """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.
     """
-    considered_tags = [
+    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
-            subprocess.check_output(
-                ['hg', 'tags', '-R', base_dir, '-q']).split()
-        ] if version_is_op_to(c[0], version, operator.ge)
+            platform_tags.findall(subprocess.check_output(
+                ['hg', 'tags', '-R', base_dir, '-q']))
+        ] if compare_versions(c[0], version) >= 0
     ]
 
-    for tag in considered_tags:
-        if version_is_op_to(tag[0], version, operator.eq):
-            if set(tag[1:]) != set(platforms):
-                return False, ('You have to re-release version {} for exactly '
-                               'all of {}').format(version, ', '.join(tag[1:]))
-        if version_is_op_to(tag[0], version, operator.gt):
-            intersect = set(tag[1:]).intersection(platforms)
-            if intersect:
-                return False, ('The higher version {} has already been '
-                               'released for {}').format(
-                                   tag[0], ', '.join(intersect))
-
-    return True, None
+    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".
 
     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(
@@ skipping 23 matching lines @@
     )
 
     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_call([
+    subprocess.check_output([
         'hg', 'commit', '-R', base_dir, '-m',
         'Noissue - Releasing {} {} for {}'.format(
             extension_name, version,
-            ', '.join([p.capitalize() for p in platforms]))])
+            ', '.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', '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
 
     target_platforms = sorted(platforms)
     release_identifier = '-'.join([version] + [p for p in target_platforms])
 
-    release_possible, reason = release_combination_is_possible(
+    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, target_platforms[0],
@@ skipping 19 matching lines @@
         )
 
     # 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)
-
-    release_commit(baseDir, extension_name, version, target_platforms)
+    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)
     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])