Issue 5996 - Release consistent versions across WebExtensions

Issue 5996 - Release consistent versions across WebExtensions

[tlucas, 2017-11-17 15:25:31]

Path Set 1:
 * Enable releaseAutomation.py and build.py to handle gecko releases
 * Enable releaseAutomation.py and build.py to handle mutiple releases at a time
 * Disallowed the creation of separate builds for WebExtension platforms with
equal version numbers

[tlucas, 2017-11-20 11:44:28]

Patch Set 2:
 * Rebasing against https://hg.adblockplus.org/buildtools/rev/f92050874f05

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

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:127: return version in existing
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...

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:156: get_extension(platform, key_file is not None))
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.

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:168: 'Releasing {} {} for {}'.format(extension_name,
version,
Since you are touching this, how about prefixing the commit message with
"Noissue - "?

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:169: ', '.join(platforms))])
If we want to have it extra nice, maybe map(str.capitalize) as well?

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:173: subprocess.check_call(['hg', 'tag', '-R', base_dir,
'-f', tag_name])
Since you are touching this, how about passing '-m', 'Noissue - Adding release
tag for {} {}'

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:184: 'was already done. Aborting.').format(version))
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?

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:236: release_tag(baseDir, '{}-{}'.format(version,
platform))
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:53]

Patch Set 3
 * Beautified commit messages
 * Added docstrings to new functions
 * Introduced new release decision machinery

Sorry, i forgot to upload and publish this after a meeting yesterday. I also
adjusted the ticket to match what was discussed.

Best,
Tristan

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:127: return version in existing
On 2017/11/20 15:33:17, Wladimir Palant wrote:
> 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...

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

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:156: get_extension(platform, key_file is not None))
On 2017/11/20 15:33:16, Wladimir Palant wrote:
> 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.

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".

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:168: 'Releasing {} {} for {}'.format(extension_name,
version,
On 2017/11/20 15:33:16, Wladimir Palant wrote:
> Since you are touching this, how about prefixing the commit message with
> "Noissue - "?

Done.

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:169: ', '.join(platforms))])
On 2017/11/20 15:33:16, Wladimir Palant wrote:
> If we want to have it extra nice, maybe map(str.capitalize) as well?

Done.

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:173: subprocess.check_call(['hg', 'tag', '-R', base_dir,
'-f', tag_name])
On 2017/11/20 15:33:17, Wladimir Palant wrote:
> Since you are touching this, how about passing '-m', 'Noissue - Adding release
> tag for {} {}'

Done.

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:184: 'was already done. Aborting.').format(version))
On 2017/11/20 15:33:17, Wladimir Palant wrote:
> 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?

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

https://codereview.adblockplus.org/29611593/diff/29612649/releaseAutomation.p...
releaseAutomation.py:236: release_tag(baseDir, '{}-{}'.format(version,
platform))
On 2017/11/20 15:33:17, Wladimir Palant wrote:
> 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.

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

https://codereview.adblockplus.org/29611593/diff/29616562/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29611593/diff/29616562/releaseAutomation.p...
releaseAutomation.py:159: def release_combination_is_possible(version,
platforms, base_dir):
This docstring reflects what was discussed in IRC.

[Wladimir Palant, 2017-11-28 11:01:17]

https://codereview.adblockplus.org/29611593/diff/29616562/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29611593/diff/29616562/releaseAutomation.p...
releaseAutomation.py:122: def version_is_op_to(value, orig, op):
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.

https://codereview.adblockplus.org/29611593/diff/29616562/releaseAutomation.p...
releaseAutomation.py:154: pass
How about only passing in tags that match r'^\d+(\.\d+)$' regular expression
instead of catching errors?

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

Patch Set 4:
 * Addressing comments
 * Only consider new tag formats in comparison
 * Fixed false positives in combination-is-possible decision

https://codereview.adblockplus.org/29611593/diff/29616562/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29611593/diff/29616562/releaseAutomation.p...
releaseAutomation.py:122: def version_is_op_to(value, orig, op):
On 2017/11/28 11:01:16, Wladimir Palant wrote:
> 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.

Done.

https://codereview.adblockplus.org/29611593/diff/29616562/releaseAutomation.p...
releaseAutomation.py:154: pass
On 2017/11/28 11:01:16, Wladimir Palant wrote:
> How about only passing in tags that match r'^\d+(\.\d+)$' regular expression
> instead of catching errors?

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

[Wladimir Palant, 2017-11-28 14:04:41]

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.p...
releaseAutomation.py:128: return int(items[index]) if index < len(items) else 0
You don't need to call int() here, you converted to integer above already.

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.p...
releaseAutomation.py:130: for i in range(len(a_digits)):
This needs to be max(len(a_digits), len(b_digits)), otherwise 1.2 and 1.2.3 will
be considered equal.

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.p...
releaseAutomation.py:155: platform_tags =
re.compile(r'^(\d+(?:(?:\.\d+)+)(?:-\w+)+)$', re.MULTILINE)
Technically speaking, "1" is a valid version number, use * as quantifier?
\d+(?:(?:\.\d+)*)

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.p...
releaseAutomation.py:283: if not re_release:
This might not be a good measure. What if the release commit is backed out
before addressing an issue and re-releasing?

[tlucas, 2017-11-28 14:21:16]

Patch Set 5:
 * Removed double conversion
 * Accounted for versions of different length
 * loosened tag-regex
 * Ignore "nothing changed" only while re-releasing

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.py
File releaseAutomation.py (right):

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.p...
releaseAutomation.py:128: return int(items[index]) if index < len(items) else 0
On 2017/11/28 14:04:41, Wladimir Palant wrote:
> You don't need to call int() here, you converted to integer above already.

Done.

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.p...
releaseAutomation.py:130: for i in range(len(a_digits)):
On 2017/11/28 14:04:41, Wladimir Palant wrote:
> This needs to be max(len(a_digits), len(b_digits)), otherwise 1.2 and 1.2.3
will
> be considered equal.

Done.

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.p...
releaseAutomation.py:155: platform_tags =
re.compile(r'^(\d+(?:(?:\.\d+)+)(?:-\w+)+)$', re.MULTILINE)
On 2017/11/28 14:04:41, Wladimir Palant wrote:
> Technically speaking, "1" is a valid version number, use * as quantifier?
> \d+(?:(?:\.\d+)*)

Good point, Done.

https://codereview.adblockplus.org/29611593/diff/29622571/releaseAutomation.p...
releaseAutomation.py:283: if not re_release:
On 2017/11/28 14:04:41, Wladimir Palant wrote:
> This might not be a good measure. What if the release commit is backed out
> before addressing an issue and re-releasing?

Done. Replaced it with a try-except block, re-raising when we are not
re-releasing and hg returns something different then "nothing changed".

[Wladimir Palant, 2017-12-04 11:03:56]

LGTM