Issue 29357701: Issue 4548 - Split out Safari and add Edge release automation (Closed)
|
Created:
Oct. 19, 2016, 11:41 a.m. by kzar Modified:
Oct. 28, 2016, 8:38 a.m. CC:
kvas Visibility:
Public. |
DescriptionIssue 4548 - Split out Safari and add Edge release automation
Patch Set 1 #Patch Set 2 : Use correct appx extension for Edge build #
Total comments: 16
Patch Set 3 : Addressed feedback #
Total comments: 6
Patch Set 4 : Removed redundant keyFile parameters #Patch Set 5 : Addressed nits #Patch Set 6 : Add '-type' to release tag names for Safari+Edge #
Total comments: 5
Patch Set 7 : Avoid linting errors #
Total comments: 4
Patch Set 8 : Import getDefaultFileName and readMetadata from packager directly #
Total comments: 2
Patch Set 9 : Addressed nits #
MessagesTotal messages: 26
Patch Set 1 These changes are completely untested, I have no idea how to test this stuff without risking pushing tags etc!
Patch Set 2 : Use correct appx extension for Edge build
https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.p... releaseAutomation.py:111: packager.createBuild(baseDir, type=type, outFile=buildPathUnsigned, releaseBuild=True, keyFile=None) I suppose the absence of downloads.append(...) means that we wouldn't add the build to the downloads repository? I wonder whether we should still add it, even though there are some issues when installing a build that isn't signed by Microsoft. But it's not that we don't have similar issues with off-store extensions for Chrome and Safari. And that way, QA can at least try compare different versions. Even better, if we could download the build with Microsoft's signature from the Windows Store, after unsigned build has been uploaded, and archive it in our downloads repository. But I guess that doesn't work the way the release automation currently works.
https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.p... releaseAutomation.py:111: packager.createBuild(baseDir, type=type, outFile=buildPathUnsigned, releaseBuild=True, keyFile=None) On 2016/10/19 14:26:14, Sebastian Noack wrote: > I suppose the absence of downloads.append(...) means that we wouldn't add the > build to the downloads repository? That's the idea. I followed the example of the unsigned Chrome builds above, but I don't have a strong opinion on this either way.
https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.p... releaseAutomation.py:111: packager.createBuild(baseDir, type=type, outFile=buildPathUnsigned, releaseBuild=True, keyFile=None) On 2016/10/19 14:31:03, kzar wrote: > On 2016/10/19 14:26:14, Sebastian Noack wrote: > > I suppose the absence of downloads.append(...) means that we wouldn't add the > > build to the downloads repository? > > That's the idea. I followed the example of the unsigned Chrome builds above, but > I don't have a strong opinion on this either way. Sure, I just thought that it might be useful to archive the unsigned builds at least if we don't have a signed build for Microsoft Edge. But I start to wonder how useful this would be after all.
Please don't commit anything untested here, I really don't want to discover issues during release. You can test your code by commenting out the two `hg push` commands in the script. Or you can change the push URL of the repositories to point to a local directory. If you need to remove the commits later you can use `hg strip` (probably running `hg phase -d` first if you pushed them to a local directory). https://codereview.adblockplus.org/29357701/diff/29357705/build.py File build.py (right): https://codereview.adblockplus.org/29357701/diff/29357705/build.py#newcode431 build.py:431: keyFiles.append(value) You should modify option parsing here - no point expecting multiple keys any more. So it should be a keyFile variable and this statement should be simply `keyFile = value` - same as in runBuild(). Consequently, below you should merely check whether the key file was set and the code in releaseAutomation.py can be simplified as well. https://codereview.adblockplus.org/29357701/diff/29357705/build.py#newcode445 build.py:445: if (type == 'chrome' or type == 'safari') and len(keyFiles) != 1: Probably better to use `type in ('chrome', 'safari')` here. https://codereview.adblockplus.org/29357701/diff/29357705/build.py#newcode525 build.py:525: command.addOption('File containing private key and certificates required to sign the release. Note that for Chrome releases this option needs to be specified twice: first a key to sign Chrome builds, then another to sign the Safari build.', short='k', long='key', value='file', types=('chrome',)) This description needs to be updated, the note is outdated. Also, you need to add 'safari' and 'edge' to types here. https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.p... releaseAutomation.py:89: metadata = packager.readMetadata(baseDir, type) This statement is unnecessary - here and in all the other branches. We already read metadata above. https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.p... releaseAutomation.py:91: packager.createBuild(baseDir, type=type, outFile=buildPath, releaseBuild=True, keyFile=keyFile) While at it, could you remove the keyFile parameter here? packagerGecko no longer supports signing, it seems that we missed this in https://issues.adblockplus.org/ticket/4336. https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.p... releaseAutomation.py:111: packager.createBuild(baseDir, type=type, outFile=buildPathUnsigned, releaseBuild=True, keyFile=None) This file should definitely be added to the downloads repository. We don't offer Chrome or Firefox builds for download from our site either, these wouldn't be usable. But we need some place to look up what we've published there. Neither Chrome Web Store nor Windows Store will let you download historical releases, in fact Windows Store makes it ridiculously complicated even for the current release.
https://codereview.adblockplus.org/29357701/diff/29357705/build.py File build.py (right): https://codereview.adblockplus.org/29357701/diff/29357705/build.py#newcode445 build.py:445: if (type == 'chrome' or type == 'safari') and len(keyFiles) != 1: On 2016/10/20 10:03:07, Wladimir Palant wrote: > Probably better to use `type in ('chrome', 'safari')` here. I agree, however, a set is even more appropriate here (than a tuple).
Patch Set 3 : Addressed feedback I tested this somewhat by commenting out the `hg push` lines at the end of releaseAutomation.py like Wladimir suggested. I created Chrome and Edge releases successfully and checked that they showed up in the commit log with tags etc. I didn't test doing a Safari not Gecko release and I also didn't check that the generated builds were correct. I needed to make a small change to the Edge packager for this to work and I also needed a metadata.edge file. (I made this dummy one https://gist.github.com/kzar/102b8b6d9342dad32c1e942d6532bea7 .) One question, do we need to adjust the tag names and commit messages? Maybe "Added tag N for changeset R" and "Releasing Adblock Plus N" are no longer specific enough and we should include the target platform name too? https://codereview.adblockplus.org/29357701/diff/29357705/build.py File build.py (right): https://codereview.adblockplus.org/29357701/diff/29357705/build.py#newcode431 build.py:431: keyFiles.append(value) On 2016/10/20 10:03:08, Wladimir Palant wrote: > You should modify option parsing here - no point expecting multiple keys any > more. So it should be a keyFile variable and this statement should be simply > `keyFile = value` - same as in runBuild(). Consequently, below you should merely > check whether the key file was set and the code in releaseAutomation.py can be > simplified as well. Done. https://codereview.adblockplus.org/29357701/diff/29357705/build.py#newcode445 build.py:445: if (type == 'chrome' or type == 'safari') and len(keyFiles) != 1: On 2016/10/20 10:25:26, Sebastian Noack wrote: > On 2016/10/20 10:03:07, Wladimir Palant wrote: > > Probably better to use `type in ('chrome', 'safari')` here. > > I agree, however, a set is even more appropriate here (than a tuple). Done. https://codereview.adblockplus.org/29357701/diff/29357705/build.py#newcode525 build.py:525: command.addOption('File containing private key and certificates required to sign the release. Note that for Chrome releases this option needs to be specified twice: first a key to sign Chrome builds, then another to sign the Safari build.', short='k', long='key', value='file', types=('chrome',)) On 2016/10/20 10:03:07, Wladimir Palant wrote: > This description needs to be updated, the note is outdated. Also, you need to > add 'safari' and 'edge' to types here. Done. https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.p... releaseAutomation.py:89: metadata = packager.readMetadata(baseDir, type) On 2016/10/20 10:03:08, Wladimir Palant wrote: > This statement is unnecessary - here and in all the other branches. We already > read metadata above. Done. https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.p... releaseAutomation.py:91: packager.createBuild(baseDir, type=type, outFile=buildPath, releaseBuild=True, keyFile=keyFile) On 2016/10/20 10:03:08, Wladimir Palant wrote: > While at it, could you remove the keyFile parameter here? packagerGecko no > longer supports signing, it seems that we missed this in > https://issues.adblockplus.org/ticket/4336. Done. https://codereview.adblockplus.org/29357701/diff/29357705/releaseAutomation.p... releaseAutomation.py:111: packager.createBuild(baseDir, type=type, outFile=buildPathUnsigned, releaseBuild=True, keyFile=None) On 2016/10/20 10:03:08, Wladimir Palant wrote: > This file should definitely be added to the downloads repository. We don't offer > Chrome or Firefox builds for download from our site either, these wouldn't be > usable. But we need some place to look up what we've published there. Neither > Chrome Web Store nor Windows Store will let you download historical releases, in > fact Windows Store makes it ridiculously complicated even for the current > release. Done.
On 2016/10/20 12:09:36, kzar wrote:
> One question, do we need to adjust the tag names and commit messages? Maybe
> "Added tag N for changeset R" and "Releasing Adblock Plus N" are no longer
> specific enough and we should include the target platform name too?
Now that you ask - will version numbers still be unique? If we have releases for
Chrome and Safari with the same version number we'll get a conflict with the tag
names. I guess we need to use {version}-{type} as tag name. But please don't
change anything for the Gecko branch, there it's unnecessary.
https://codereview.adblockplus.org/29357701/diff/29358373/packagerEdge.py
File packagerEdge.py (right):
https://codereview.adblockplus.org/29357701/diff/29358373/packagerEdge.py#new...
packagerEdge.py:14: from packager import (readMetadata, getDefaultFileName)
Why did you add this? I don't see these functions being called directly, only as
packager.foo (packager module already being imported above).
https://codereview.adblockplus.org/29357701/diff/29358373/releaseAutomation.py
File releaseAutomation.py (right):
https://codereview.adblockplus.org/29357701/diff/29358373/releaseAutomation.p...
releaseAutomation.py:89: packager.createBuild(baseDir, type=type,
outFile=buildPath, releaseBuild=True, keyFile=None)
Please don't just remove the value - packagerGecko doesn't support keyFile
parameter at all.
On 2016/10/22 20:12:51, Wladimir Palant wrote:
> On 2016/10/20 12:09:36, kzar wrote:
> > One question, do we need to adjust the tag names and commit messages? Maybe
> > "Added tag N for changeset R" and "Releasing Adblock Plus N" are no longer
> > specific enough and we should include the target platform name too?
>
> Now that you ask - will version numbers still be unique? If we have releases
for
> Chrome and Safari with the same version number we'll get a conflict with the
tag
> names. I guess we need to use {version}-{type} as tag name. But please don't
> change anything for the Gecko branch, there it's unnecessary.
The plan was to keep Safari on the 1.12.* track since we won't do any major
changes there anymore, and advance the version to 1.13 when we start releasing
for Chrome separately.
On 2016/10/22 20:20:00, Sebastian Noack wrote: > The plan was to keep Safari on the 1.12.* track since we won't do any major > changes there anymore, and advance the version to 1.13 when we start releasing > for Chrome separately. And Adblock Plus for Edge will remain on version 0.9.* until the bookmark is merged upstream.
Patch Set 4 : Removed redundant keyFile parameters So the consensus is we stick with just using the version number for tags and release commits, since they are unique per platform anyway? Also this makes me think, perhaps we should look in metadata.safari for the Safari extension's version instead of metadata.general? (But then arguably by that logic we could move everything over into metadata.safari for the Safari extension, so perhaps we shouldn't bother messing with it.) https://codereview.adblockplus.org/29357701/diff/29358373/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29357701/diff/29358373/packagerEdge.py#new... packagerEdge.py:14: from packager import (readMetadata, getDefaultFileName) On 2016/10/22 20:12:51, Wladimir Palant wrote: > Why did you add this? I don't see these functions being called directly, only as > packager.foo (packager module already being imported above). This change was required for `./build.py -t edge release ...` to work. See these lines approx 58 and 105 and in releaseAutomation.py: metadata = packager.readMetadata(baseDir, type) buildPath = os.path.join(downloadsRepo, packager.getDefaultFileName(metadata, version, 'appx')) https://codereview.adblockplus.org/29357701/diff/29358373/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29358373/releaseAutomation.p... releaseAutomation.py:89: packager.createBuild(baseDir, type=type, outFile=buildPath, releaseBuild=True, keyFile=None) On 2016/10/22 20:12:51, Wladimir Palant wrote: > Please don't just remove the value - packagerGecko doesn't support keyFile > parameter at all. Done.
https://codereview.adblockplus.org/29357701/diff/29358373/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29357701/diff/29358373/packagerEdge.py#new... packagerEdge.py:14: from packager import (readMetadata, getDefaultFileName) On 2016/10/25 07:50:51, kzar wrote: > This change was required for `./build.py -t edge release ...` to work. See these > lines approx 58 and 105 and in releaseAutomation.py: 1) Please add a comment explaining that releaseAutomation expects these functions even though this file doesn't need them. 2) Nit: The parentheses are unnecessary.
Patch Set 5 : Addressed nits https://codereview.adblockplus.org/29357701/diff/29358373/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29357701/diff/29358373/packagerEdge.py#new... packagerEdge.py:14: from packager import (readMetadata, getDefaultFileName) On 2016/10/25 10:21:37, Wladimir Palant wrote: > On 2016/10/25 07:50:51, kzar wrote: > > This change was required for `./build.py -t edge release ...` to work. See > these > > lines approx 58 and 105 and in releaseAutomation.py: > > 1) Please add a comment explaining that releaseAutomation expects these > functions even though this file doesn't need them. > > 2) Nit: The parentheses are unnecessary. Done.
On 2016/10/25 07:50:52, kzar wrote: > Patch Set 4 : Removed redundant keyFile parameters > > So the consensus is we stick with just using the version number for tags and > release commits, since they are unique per platform anyway? Giving it a second thought, while in practice the versions would not conflict, it might still make sense to add the type for releases created out of a bookmark (e.g. 0.9.10-edge, 1.12.5-safari). Otherwise, it might create some confusion when going through the list of previous releases/tags, in the future. I wouldn't add the type for releases build out of "tip" though.
Patch Set 6 : Add '-type' to release tag names for Safari+Edge
LGTM
Please check that the tests still pass before pushing the changes. Right now it seems that they are broken (in a non-material and annoying way but still): https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py#new... packagerEdge.py:15: from packager import readMetadata, getDefaultFileName Please note that currently this causes `tox` to fail because `flake8` complains about unused imports. This can be solved by adding `# flake8: noqa` comment to the end of the line or (perhaps a bit more elegant) by adding those symbols (and maybe everything else that is expected to be here) to `__all__`.
https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py#new... packagerEdge.py:15: from packager import readMetadata, getDefaultFileName On 2016/10/25 16:16:26, Vasily Kuznetsov wrote: > Please note that currently this causes `tox` to fail because `flake8` complains > about unused imports. This can be solved by adding `# flake8: noqa` comment to > the end of the line or (perhaps a bit more elegant) by adding those symbols (and > maybe everything else that is expected to be here) to `__all__`. I'd like to avoid #noqa as much as possible. But I'm not a fan of __all__ either, since "from import *" is generally discouraged. Any reason the releaseAutomation module isn't importing those functions directly from the packager module? It seems it isn't overridden by any platform-specific packager anyway.
Patch Set 7 : Avoid linting errors https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py#new... packagerEdge.py:15: from packager import readMetadata, getDefaultFileName On 2016/10/26 09:33:14, Sebastian Noack wrote: > On 2016/10/25 16:16:26, Vasily Kuznetsov wrote: > > Please note that currently this causes `tox` to fail because `flake8` > complains > > about unused imports. This can be solved by adding `# flake8: noqa` comment to > > the end of the line or (perhaps a bit more elegant) by adding those symbols > (and > > maybe everything else that is expected to be here) to `__all__`. > > I'd like to avoid #noqa as much as possible. But I'm not a fan of __all__ > either, since "from import *" is generally discouraged. > > Any reason the releaseAutomation module isn't importing those functions directly > from the packager module? It seems it isn't overridden by any platform-specific > packager anyway. Ah sorry, I didn't realise the repository had tests! Since it took me a minute to figure out how to run them I've opened a review to add a README for the next person: https://codereview.adblockplus.org/29359949/ Also I found the tests fail for me even on master, this seems to be since my system expects a different content type for OTF files. I've opened an issue for that: https://issues.adblockplus.org/ticket/4575#ticket Done.
https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py#new... packagerEdge.py:15: from packager import readMetadata, getDefaultFileName It seems you ignored my comment from above: On 2016/10/26 09:33:14, Sebastian Noack wrote: > Any reason the releaseAutomation module isn't importing those functions directly > from the packager module? It seems it isn't overridden by any platform-specific > packager anyway. https://codereview.adblockplus.org/29357701/diff/29359952/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29359952/releaseAutomation.p... releaseAutomation.py:85: tag_name += '-' + type Nit: As per our coding style, we use the format() method when concatenating more than two strings: tag_name = '{}-{}'.format(tag_name, type)
Patch Set 8 : Import getDefaultFileName and readMetadata from packager directly https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29357701/diff/29359872/packagerEdge.py#new... packagerEdge.py:15: from packager import readMetadata, getDefaultFileName On 2016/10/27 10:57:32, Sebastian Noack wrote: > It seems you ignored my comment from above: > > On 2016/10/26 09:33:14, Sebastian Noack wrote: > > Any reason the releaseAutomation module isn't importing those functions > directly > > from the packager module? It seems it isn't overridden by any > platform-specific > > packager anyway. Whoops, so I did. Done. https://codereview.adblockplus.org/29357701/diff/29359952/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29359952/releaseAutomation.p... releaseAutomation.py:85: tag_name += '-' + type On 2016/10/27 10:57:32, Sebastian Noack wrote: > Nit: As per our coding style, we use the format() method when concatenating more > than two strings: > > tag_name = '{}-{}'.format(tag_name, type) Do you insist? IMO `tag_name += '-' + type` is nicer than `tag_name = '{}-{}'.format(tag_name, type)` here.
https://codereview.adblockplus.org/29357701/diff/29359952/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29359952/releaseAutomation.p... releaseAutomation.py:85: tag_name += '-' + type On 2016/10/27 15:25:58, kzar wrote: > On 2016/10/27 10:57:32, Sebastian Noack wrote: > > Nit: As per our coding style, we use the format() method when concatenating > more > > than two strings: > > > > tag_name = '{}-{}'.format(tag_name, type) > > Do you insist? IMO `tag_name += '-' + type` is nicer than `tag_name = > '{}-{}'.format(tag_name, type)` here. This might be a matter of taste. I think the string formatting variant is more readable because when you see '{}-{}' you immediately recognize the structure of the resulting string. One could also argue that this is faster in Python, but that doesn't matter here. Anyway, we added this to the coding style guide, so that we don't have to discuss that anymore on a case-by-case basis every time. https://codereview.adblockplus.org/29357701/diff/29360011/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29360011/releaseAutomation.p... releaseAutomation.py:47: from packager import readMetadata, getDefaultFileName I think imports should go at the top of the module, unless there is a necessity for them to be performed at runtime (like with the conditional imports below).
Patch Set 9 : Addressed nits https://codereview.adblockplus.org/29357701/diff/29359952/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29359952/releaseAutomation.p... releaseAutomation.py:85: tag_name += '-' + type On 2016/10/27 15:41:59, Sebastian Noack wrote: > On 2016/10/27 15:25:58, kzar wrote: > > On 2016/10/27 10:57:32, Sebastian Noack wrote: > > > Nit: As per our coding style, we use the format() method when concatenating > > more > > > than two strings: > > > > > > tag_name = '{}-{}'.format(tag_name, type) > > > > Do you insist? IMO `tag_name += '-' + type` is nicer than `tag_name = > > '{}-{}'.format(tag_name, type)` here. > > This might be a matter of taste. I think the string formatting variant is more > readable because when you see '{}-{}' you immediately recognize the structure of > the resulting string. One could also argue that this is faster in Python, but > that doesn't matter here. Anyway, we added this to the coding style guide, so > that we don't have to discuss that anymore on a case-by-case basis every time. Done. https://codereview.adblockplus.org/29357701/diff/29360011/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29357701/diff/29360011/releaseAutomation.p... releaseAutomation.py:47: from packager import readMetadata, getDefaultFileName On 2016/10/27 15:41:59, Sebastian Noack wrote: > I think imports should go at the top of the module, unless there is a necessity > for them to be performed at runtime (like with the conditional imports below). Done.
LGTM
Message was sent while issue was closed.
Still LGTM here. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
