Issue 29345751: Issue 4028 - Add support for Edge extensions to buildtools (Closed)
|
Created:
June 13, 2016, 12:31 p.m. by Vasily Kuznetsov Modified:
Oct. 17, 2016, 1:12 p.m. CC:
Oleksandr Visibility:
Public. |
DescriptionIssue 4028 - Add support for Edge extensions to buildtools
Repository: https://hg.adblockplus.org/buildtools/
Base revision: d865256754db
Patch Set 1 #
Total comments: 29
Patch Set 2 : Address comments on patch set 1 #
Total comments: 20
Patch Set 3 : Address review comments on patch set 2 #
Total comments: 17
Patch Set 4 : Address review comments 3 (except for localization of AppxManifest) #
Total comments: 2
Patch Set 5 : Address comments on patch set 4 #
Total comments: 17
Patch Set 6 : Address comments on patch set 5 #
Total comments: 4
Patch Set 7 : Address comments on patch set 6 #
Total comments: 15
Patch Set 8 : Address comments on patch set 7 #
Total comments: 14
Patch Set 9 : Address comments on patch set 8 #
Total comments: 2
Patch Set 10 : Remove app_id inconsistency warning and start padding the version to 4 groups of digits #Patch Set 11 : Address Windows Store issues with blockmap and devbuild display name #
MessagesTotal messages: 28
This is a draft implementation of the Edge packaging support. The work to address #4028 is not complete with this code and, actually, even the issue itself is likely not complete. I'm publishing the results of my R&D to ask for feedback on the general direction and on the details of Edge extension packaging. Let me know if anything looks obviously wrong in the resulting package. Also I'd be interested in your advice on the ways to test the output of the packager. Note: this patchset will not cleanly apply with `patch` or `hg import` but you can use this utility https://hg.adblockplus.org/codingtools/file/tip/patchconv to convert it to a proper git-format patch. https://codereview.adblockplus.org/29345751/diff/29345752/build.py File build.py (right): https://codereview.adblockplus.org/29345751/diff/29345752/build.py#newcode178 build.py:178: def runBuild(baseDir, scriptName, opts, args, type): Add support for `type = "edge"` and removing unnecessary code duplication. https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... docs/metadata.edge.example:1: [default] Rough example of how `metadata.edge` is supposed to look to make this code work. At the moment this is here just to make it possible to test the changes with `adblockpluschrome`. Ideally, I think the dependency on specific metadata content should be documented in buildtools for all types of builds. https://codereview.adblockplus.org/29345751/diff/29345752/packager.py File packager.py (right): https://codereview.adblockplus.org/29345751/diff/29345752/packager.py#newcode72 packager.py:72: templatePath = os.path.join(buildtools.__path__[0], 'templates') These changes are needed to move all templates into `templates` folder. Strictly speaking this was not necessary to add Edge support but I didn't feel like adding more stuff to an already overcrowded root directory of `buildtools`. https://codereview.adblockplus.org/29345751/diff/29345752/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29345751/diff/29345752/packagerChrome.py#n... packagerChrome.py:13: from packager import (readMetadata, getDefaultFileName, getBuildVersion, Removed unused imports. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:82: def zip(self, outFile, sortKey=None): This can eventually be replaced with a compressing version: https://issues.adblockplus.org/ticket/4149 https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:91: def createBuild(baseDir, type='edge', outFile=None, buildNum=None, This function has been copied from `packagerChrome` and modified to produce edge packages. There's in general a lot of code duplication between `createBuild` functions in all packagers and I chose to not do anything about it for now to keep the number of unrelated changes low. https://codereview.adblockplus.org/29345751/diff/29345752/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29345751/diff/29345752/tox.ini#newcode13 tox.ini:13: packagerEdge.py : A302,N802,N803,N806 Unfortunately it's either break the conventions of the module or have these errors. I thought that the latter is a lesser evil at this point.
The number of unrelated changes is questionable, it would have been nice to have those in separate commits/reviews. https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... docs/metadata.edge.example:1: [default] On 2016/06/13 12:57:31, Vasily Kuznetsov wrote: > Ideally, I think the dependency on specific metadata content should be > documented in buildtools for all types of builds. Documented - yes. But this content is very specific to adblockpluschrome and I don't think it belongs here in this form. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:16: BLOCKSIZE = 65536 Maybe more obvious if hexadecimal - 0x10000? https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:20: LFH_FIXED_SIZE = 30 zipfile.sizeFileHeader? https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:38: 'lfh_size': str(LFH_FIXED_SIZE + len(filename)), Assumption here: filename is ASCII. Maybe enforce that with an assert? https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:40: for block in blocks] No point stringifying all the numbers, the representation of these numbers is up to the template. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:59: params['logo_{}'.format(size)] = 'Assets\\logo_{}.png'.format(size) We need to verify that these files actually exist. Or maybe explicitly only package specific asset files so that a missing logo_50.png entry under [assets] will produce an error. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:65: names = sorted(files.keys(), key=len, reverse=True) Why sort here? If the point is creating a copy of files.keys() that is an iterator in Python 3 - that can be done easier. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:70: files[CONTENT_TYPES] = content_types_template.render().encode('utf-8') This function does two things - prefix file names and add a content types file (but not the other XML files). I don't see how they belong together, also "convertToAppx" doesn't really reflect the purpose here. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:82: def zip(self, outFile, sortKey=None): On 2016/06/13 12:57:31, Vasily Kuznetsov wrote: > This can eventually be replaced with a compressing version: > https://issues.adblockplus.org/ticket/4149 Maybe add a TODO comment on that, so it's obvious?
Thanks for the comments, Wladimir, https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... docs/metadata.edge.example:1: [default] On 2016/06/17 09:53:45, Wladimir Palant wrote: > On 2016/06/13 12:57:31, Vasily Kuznetsov wrote: > > Ideally, I think the dependency on specific metadata content should be > > documented in buildtools for all types of builds. > > Documented - yes. But this content is very specific to adblockpluschrome and I > don't think it belongs here in this form. Yeah, I agree. This example is pretty rough and has too much specific data. What we want is more like the list of the sections and keys that are significant for the packager, what values are expected and what will be done with them. Do you think it makes more sense to strip this file down and add comments with descriptions or to write an .md documentation file instead? https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:16: BLOCKSIZE = 65536 On 2016/06/17 09:53:46, Wladimir Palant wrote: > Maybe more obvious if hexadecimal - 0x10000? I'm not sure if it's actually more obvious. Because what we would like to say is 64k. So maybe just `64 * 1024` ? https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:20: LFH_FIXED_SIZE = 30 On 2016/06/17 09:53:46, Wladimir Palant wrote: > zipfile.sizeFileHeader? Oh, I didn't know about this constant. It's not mentioned in the documentation, but I guess if they didn't want me to use it, they would have put an underscore on it. Done. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:38: 'lfh_size': str(LFH_FIXED_SIZE + len(filename)), On 2016/06/17 09:53:46, Wladimir Palant wrote: > Assumption here: filename is ASCII. Maybe enforce that with an assert? If it's not ASCII, it gets encoded as utf-8. I implemented this option too, just in case -- it seems better than failing. So now we don't need an assert. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:40: for block in blocks] On 2016/06/17 09:53:46, Wladimir Palant wrote: > No point stringifying all the numbers, the representation of these numbers is up > to the template. Done. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:59: params['logo_{}'.format(size)] = 'Assets\\logo_{}.png'.format(size) On 2016/06/17 09:53:46, Wladimir Palant wrote: > We need to verify that these files actually exist. Or maybe explicitly only > package specific asset files so that a missing logo_50.png entry under [assets] > will produce an error. I started checking that they exist. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:65: names = sorted(files.keys(), key=len, reverse=True) On 2016/06/17 09:53:45, Wladimir Palant wrote: > Why sort here? If the point is creating a copy of files.keys() that is an > iterator in Python 3 - that can be done easier. Sorting is to make sure that moving 'foo' to 'Extension/foo' doesn't overwrite an already existing 'Extension/foo' (which should be moved to 'Extension/Extension/foo'). If we start with the files that have the longest paths, this cannot happen. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:70: files[CONTENT_TYPES] = content_types_template.render().encode('utf-8') On 2016/06/17 09:53:46, Wladimir Palant wrote: > This function does two things - prefix file names and add a content types file > (but not the other XML files). I don't see how they belong together, also > "convertToAppx" doesn't really reflect the purpose here. Yeah, makes sense. I broke it up. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:82: def zip(self, outFile, sortKey=None): On 2016/06/17 09:53:46, Wladimir Palant wrote: > On 2016/06/13 12:57:31, Vasily Kuznetsov wrote: > > This can eventually be replaced with a compressing version: > > https://issues.adblockplus.org/ticket/4149 > > Maybe add a TODO comment on that, so it's obvious? Done.
No real issues left but I think that Sebastian should take a look as well - and be it simply so that more people understand what this code is doing. https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... docs/metadata.edge.example:1: [default] On 2016/06/17 17:53:26, Vasily Kuznetsov wrote: > Do you think it makes more sense to strip this file down and add comments with > descriptions or to write an .md documentation file instead? IMHO, this isn't worth documenting "properly" - an example and a few comments where necessary will do. Maybe even split the example into a "common" part (not platform-specific) and an Edge-specific one, this will make adding examples for other platforms later easier. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:65: names = sorted(files.keys(), key=len, reverse=True) On 2016/06/17 17:53:26, Vasily Kuznetsov wrote: > Sorting is to make sure that moving 'foo' to 'Extension/foo' doesn't overwrite > an already existing 'Extension/foo' (which should be moved to > 'Extension/Extension/foo'). If we start with the files that have the longest > paths, this cannot happen. This makes sense but it's also non-obvious. You better add a comment on it, otherwise somebody else might remove the seemingly unnecessary sorting here later.
https://codereview.adblockplus.org/29345751/diff/29346645/build.py File build.py (right): https://codereview.adblockplus.org/29345751/diff/29346645/build.py#newcode185 build.py:185: if type != 'gecko' and not re.search(r'^\d+$', kwargs['buildNum']): I know it has been like that before, but while changing this line feel free to get rid of the unnecessary regexp: kwargs['buildNum'].isdigit() https://codereview.adblockplus.org/29345751/diff/29346645/packager.py File packager.py (left): https://codereview.adblockplus.org/29345751/diff/29346645/packager.py#oldcode11 packager.py:11: import codecs I suppose you can remove the respective putty-ignore for A301 from tox.ini now? https://codereview.adblockplus.org/29345751/diff/29346645/packager.py File packager.py (right): https://codereview.adblockplus.org/29345751/diff/29346645/packager.py#newcode72 packager.py:72: templatePath = os.path.join(buildtools.__path__[0], 'templates') While changing every line using this variable, mind using underscores instead camel case? https://codereview.adblockplus.org/29345751/diff/29346645/templates/edge/[Con... File templates/edge/[Content_Types].xml.tmpl (right): https://codereview.adblockplus.org/29345751/diff/29346645/templates/edge/[Con... templates/edge/[Content_Types].xml.tmpl:3: <Default Extension="json" ContentType="application/json"/> I wonder whether we should auto-generate the defaults based on the file extensions actually used in the bundle using the mimetypes module. https://codereview.adblockplus.org/29345751/diff/29346645/tests/AppManifest.x... File tests/AppManifest.xml.expect (right): https://codereview.adblockplus.org/29345751/diff/29346645/tests/AppManifest.x... tests/AppManifest.xml.expect:10: Name="EyeoGmbH.AdblockPlus" Please don't hard-code publisher, product name and description. The publisher is given by the "author" field in the "general" section of the metadata. Name and description can be read from the localization files. https://codereview.adblockplus.org/29345751/diff/29346645/tests/test_packager... File tests/test_packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29346645/tests/test_packager... tests/test_packagerEdge.py:19: from buildtools import packager, packagerEdge # noqa: must modify path before this. Perhaps it's better to just set the PYTHONPATH in tox.ini? https://codereview.adblockplus.org/29345751/diff/29346645/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29345751/diff/29346645/tox.ini#newcode13 tox.ini:13: packagerEdge.py : A302,N802,N803,N806 Well, the point of using putty-ignore is not to make this list even larger, but to make sure that new code complies and existing code doesn't get any worse. So please remove this line and fix any warning. https://codereview.adblockplus.org/29345751/diff/29346645/tox.ini#newcode20 tox.ini:20: basepython=python2 (Why) is this necessary?
I addressed the patch set 2 comments. https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29345752/docs/metadata.edge.... docs/metadata.edge.example:1: [default] On 2016/06/29 19:33:44, Wladimir Palant wrote: > On 2016/06/17 17:53:26, Vasily Kuznetsov wrote: > > Do you think it makes more sense to strip this file down and add comments with > > descriptions or to write an .md documentation file instead? > > IMHO, this isn't worth documenting "properly" - an example and a few comments > where necessary will do. Maybe even split the example into a "common" part (not > platform-specific) and an Edge-specific one, this will make adding examples for > other platforms later easier. For now I made the existing example more abstract and compact and added comments. I'd appreciate some review on the comments because I might have misunderstood the meaning of some keys. The common part is not (and wasn't) in this example file at the moment, We should probably add and example for it too, together with example metadata for other targets, but perhaps not in this issue. https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29345752/packagerEdge.py#new... packagerEdge.py:65: names = sorted(files.keys(), key=len, reverse=True) On 2016/06/29 19:33:44, Wladimir Palant wrote: > On 2016/06/17 17:53:26, Vasily Kuznetsov wrote: > > Sorting is to make sure that moving 'foo' to 'Extension/foo' doesn't overwrite > > an already existing 'Extension/foo' (which should be moved to > > 'Extension/Extension/foo'). If we start with the files that have the longest > > paths, this cannot happen. > > This makes sense but it's also non-obvious. You better add a comment on it, > otherwise somebody else might remove the seemingly unnecessary sorting here > later. Done https://codereview.adblockplus.org/29345751/diff/29346645/build.py File build.py (right): https://codereview.adblockplus.org/29345751/diff/29346645/build.py#newcode185 build.py:185: if type != 'gecko' and not re.search(r'^\d+$', kwargs['buildNum']): On 2016/07/01 15:56:03, Sebastian Noack wrote: > I know it has been like that before, but while changing this line feel free to > get rid of the unnecessary regexp: > > kwargs['buildNum'].isdigit() Done https://codereview.adblockplus.org/29345751/diff/29346645/packager.py File packager.py (left): https://codereview.adblockplus.org/29345751/diff/29346645/packager.py#oldcode11 packager.py:11: import codecs On 2016/07/01 15:56:03, Sebastian Noack wrote: > I suppose you can remove the respective putty-ignore for A301 from tox.ini now? It seems that there's no A301 ignore for this file (and I'm reluctant to also fix this in other unrelated places). https://codereview.adblockplus.org/29345751/diff/29346645/packager.py File packager.py (right): https://codereview.adblockplus.org/29345751/diff/29346645/packager.py#newcode72 packager.py:72: templatePath = os.path.join(buildtools.__path__[0], 'templates') On 2016/07/01 15:56:04, Sebastian Noack wrote: > While changing every line using this variable, mind using underscores instead > camel case? Done. https://codereview.adblockplus.org/29345751/diff/29346645/templates/edge/[Con... File templates/edge/[Content_Types].xml.tmpl (right): https://codereview.adblockplus.org/29345751/diff/29346645/templates/edge/[Con... templates/edge/[Content_Types].xml.tmpl:3: <Default Extension="json" ContentType="application/json"/> On 2016/07/01 15:56:04, Sebastian Noack wrote: > I wonder whether we should auto-generate the defaults based on the file > extensions actually used in the bundle using the mimetypes module. We could, but I thought it would be more work than just using a static file. https://codereview.adblockplus.org/29345751/diff/29346645/tests/AppManifest.x... File tests/AppManifest.xml.expect (right): https://codereview.adblockplus.org/29345751/diff/29346645/tests/AppManifest.x... tests/AppManifest.xml.expect:10: Name="EyeoGmbH.AdblockPlus" On 2016/07/01 15:56:04, Sebastian Noack wrote: > Please don't hard-code publisher, product name and description. The publisher is > given by the "author" field in the "general" section of the metadata. Name and > description can be read from the localization files. Well, this file is part of a test. In the template it's not hardcoded. I fully agree with you about not hard-coding. Ideally I'd like the packager code to be generic and able to package any extension. https://codereview.adblockplus.org/29345751/diff/29346645/tests/test_packager... File tests/test_packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29346645/tests/test_packager... tests/test_packagerEdge.py:19: from buildtools import packager, packagerEdge # noqa: must modify path before this. On 2016/07/01 15:56:04, Sebastian Noack wrote: > Perhaps it's better to just set the PYTHONPATH in tox.ini? You're right. Much less ugly. Done. https://codereview.adblockplus.org/29345751/diff/29346645/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29345751/diff/29346645/tox.ini#newcode13 tox.ini:13: packagerEdge.py : A302,N802,N803,N806 On 2016/07/01 15:56:04, Sebastian Noack wrote: > Well, the point of using putty-ignore is not to make this list even larger, but > to make sure that new code complies and existing code doesn't get any worse. So > please remove this line and fix any warning. Unfortunately to preserve the API of the packagers `createBuild` name and arguments had to remain matching past convention (marked with # noqa) but everything else is fixed. https://codereview.adblockplus.org/29345751/diff/29346645/tox.ini#newcode20 tox.ini:20: basepython=python2 On 2016/07/01 15:56:04, Sebastian Noack wrote: > (Why) is this necessary? Hm. Actually it's not. Perhaps I added this line when there was some other environment called something else than pyXX and then later removed that environment. Anyway, good catch, thanks.
https://codereview.adblockplus.org/29345751/diff/29346645/packager.py File packager.py (left): https://codereview.adblockplus.org/29345751/diff/29346645/packager.py#oldcode11 packager.py:11: import codecs On 2016/07/01 19:51:28, Vasily Kuznetsov wrote: > On 2016/07/01 15:56:03, Sebastian Noack wrote: > > I suppose you can remove the respective putty-ignore for A301 from tox.ini > now? > > It seems that there's no A301 ignore for this file (and I'm reluctant to also > fix this in other unrelated places). You are right. Importing codecs doesn't cause a A301, using codecs.open would. However, I suppose F401 (unused import) doesn't need to be ignored for this file anymore now. https://codereview.adblockplus.org/29345751/diff/29346645/templates/edge/[Con... File templates/edge/[Content_Types].xml.tmpl (right): https://codereview.adblockplus.org/29345751/diff/29346645/templates/edge/[Con... templates/edge/[Content_Types].xml.tmpl:3: <Default Extension="json" ContentType="application/json"/> On 2016/07/01 19:51:28, Vasily Kuznetsov wrote: > On 2016/07/01 15:56:04, Sebastian Noack wrote: > > I wonder whether we should auto-generate the defaults based on the file > > extensions actually used in the bundle using the mimetypes module. > > We could, but I thought it would be more work than just using a static file. I don't have a strong opinion, but in the sense of DRY adding a little more logic here would be better than maintaining a hardcoded/redundant list. Moreover, what if future builds add files with extensions not considered here? https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:24: h = hashlib.new('sha256') According to the Python documentation the named constructor (i.e. hashlib.sha256) is preferred over hashlib.new(). Perhaps this can even be written as a one liner, potentiality even inline. https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:44: 'blocks': [{'hash': _sha256(block), 'compressed_size': len(block)} From the documentation it seems we can simply omit the size if the package isn't compressed. https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:64: path = 'Assets/logo_{}.png'.format(size) Where does those files come from? Don't you move everything under Extension/? https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:67: params['logo_{}'.format(size)] = path.replace('/', '\\') As per our coding style please use the + operator when concatenating two strings. https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:78: files['Extension/' + filename] = files[filename] If you use files.pop() this would make the next line superfluous. https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:100: def zip(self, outFile, sortKey=None): # noqa: preserve API. The code duplication here isn't great. Perhaps just make File.zip() in packager.js configurable instead? https://codereview.adblockplus.org/29345751/diff/29347236/templates/edge/Appx... File templates/edge/AppxManifest.xml.tmpl (right): https://codereview.adblockplus.org/29345751/diff/29347236/templates/edge/Appx... templates/edge/AppxManifest.xml.tmpl:23: MinVersion="10.0.14332.0" The min and max version should be configured in the metadata file similar to how its currently done for Adblock Plus for Firefox. https://codereview.adblockplus.org/29345751/diff/29347236/templates/edge/Appx... templates/edge/AppxManifest.xml.tmpl:28: <Resource Language="en-us"/> We should localize the manifeat, using the respective name and description from the translation for each language, and listing all languages here. Refer to section 3 of the "Creating an Edge Extension package" document.
I addressed all the feedback except for localisation of AppxManifest.xml. The localisation will probably take a while -- the description in the new "Creating an Edge Extension AppX.PDF" is not entirely clear and the process seems to rely on proprietary Microsoft tools and formats that might be hard to reproduce. I'll keep you posted. https://codereview.adblockplus.org/29345751/diff/29346645/packager.py File packager.py (left): https://codereview.adblockplus.org/29345751/diff/29346645/packager.py#oldcode11 packager.py:11: import codecs On 2016/07/05 14:30:37, Sebastian Noack wrote: > On 2016/07/01 19:51:28, Vasily Kuznetsov wrote: > > On 2016/07/01 15:56:03, Sebastian Noack wrote: > > > I suppose you can remove the respective putty-ignore for A301 from tox.ini > > now? > > > > It seems that there's no A301 ignore for this file (and I'm reluctant to also > > fix this in other unrelated places). > > You are right. Importing codecs doesn't cause a A301, using codecs.open would. > However, I suppose F401 (unused import) doesn't need to be ignored for this file > anymore now. Done. https://codereview.adblockplus.org/29345751/diff/29346645/templates/edge/[Con... File templates/edge/[Content_Types].xml.tmpl (right): https://codereview.adblockplus.org/29345751/diff/29346645/templates/edge/[Con... templates/edge/[Content_Types].xml.tmpl:3: <Default Extension="json" ContentType="application/json"/> On 2016/07/05 14:30:37, Sebastian Noack wrote: > On 2016/07/01 19:51:28, Vasily Kuznetsov wrote: > > On 2016/07/01 15:56:04, Sebastian Noack wrote: > > > I wonder whether we should auto-generate the defaults based on the file > > > extensions actually used in the bundle using the mimetypes module. > > > > We could, but I thought it would be more work than just using a static file. > > I don't have a strong opinion, but in the sense of DRY adding a little more > logic here would be better than maintaining a hardcoded/redundant list. > Moreover, what if future builds add files with extensions not considered here? I implemented your suggestion and now the content type map is autogenerated. The template became a bit shorter but more complicated, and the function that uses it is now quite a bit longer. It might catch some extensions that were not hardcoded before though, so at least it's more robust this way. Done https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:24: h = hashlib.new('sha256') On 2016/07/05 14:30:37, Sebastian Noack wrote: > According to the Python documentation the named constructor (i.e. > hashlib.sha256) is preferred over hashlib.new(). > > Perhaps this can even be written as a one liner, potentiality even inline. Changed the constructor. About one-liner: it seems that you need at least 3: constructor, update, digest. And then I think the function makes sense so that _make_blockmap_entry can be nice and simple. https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:44: 'blocks': [{'hash': _sha256(block), 'compressed_size': len(block)} On 2016/07/05 14:30:38, Sebastian Noack wrote: > From the documentation it seems we can simply omit the size if the package isn't > compressed. Done. https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:64: path = 'Assets/logo_{}.png'.format(size) On 2016/07/05 14:30:38, Sebastian Noack wrote: > Where does those files come from? Don't you move everything under Extension/? These are separately added after everything is moved under `Extension/` (see lines 151-154 in this file). https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:67: params['logo_{}'.format(size)] = path.replace('/', '\\') On 2016/07/05 14:30:38, Sebastian Noack wrote: > As per our coding style please use the + operator when concatenating two > strings. Done. https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:78: files['Extension/' + filename] = files[filename] On 2016/07/05 14:30:38, Sebastian Noack wrote: > If you use files.pop() this would make the next line superfluous. Wow! I didn't realize you can `.pop()` from a dict. Neat. Done. https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:100: def zip(self, outFile, sortKey=None): # noqa: preserve API. On 2016/07/05 14:30:38, Sebastian Noack wrote: > The code duplication here isn't great. Perhaps just make File.zip() in > packager.js configurable instead? We'll have to redo this again when we implement our own compression for Appx files (#4149), but for now you're right, it doesn't make sense to duplicate this code. Done https://codereview.adblockplus.org/29345751/diff/29347236/templates/edge/Appx... File templates/edge/AppxManifest.xml.tmpl (right): https://codereview.adblockplus.org/29345751/diff/29347236/templates/edge/Appx... templates/edge/AppxManifest.xml.tmpl:23: MinVersion="10.0.14332.0" On 2016/07/05 14:30:39, Sebastian Noack wrote: > The min and max version should be configured in the metadata file similar to how > its currently done for Adblock Plus for Firefox. Done.
https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:24: h = hashlib.new('sha256') On 2016/07/07 16:23:49, Vasily Kuznetsov wrote: > On 2016/07/05 14:30:37, Sebastian Noack wrote: > > According to the Python documentation the named constructor (i.e. > > hashlib.sha256) is preferred over hashlib.new(). > > > > Perhaps this can even be written as a one liner, potentiality even inline. > > Changed the constructor. About one-liner: it seems that you need at least 3: > constructor, update, digest. And then I think the function makes sense so that > _make_blockmap_entry can be nice and simple. You can just pass the data to the constructor: base64.b64encode(hashlib.sha256(block).digest()) https://codereview.adblockplus.org/29345751/diff/29347322/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347322/packagerEdge.py#new... packagerEdge.py:154: files.read(path, 'Assets/{}'.format(name)) Use the + operator here?
https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347236/packagerEdge.py#new... packagerEdge.py:24: h = hashlib.new('sha256') On 2016/07/08 13:59:11, Sebastian Noack wrote: > On 2016/07/07 16:23:49, Vasily Kuznetsov wrote: > > On 2016/07/05 14:30:37, Sebastian Noack wrote: > > > According to the Python documentation the named constructor (i.e. > > > hashlib.sha256) is preferred over hashlib.new(). > > > > > > Perhaps this can even be written as a one liner, potentiality even inline. > > > > Changed the constructor. About one-liner: it seems that you need at least 3: > > constructor, update, digest. And then I think the function makes sense so that > > _make_blockmap_entry can be nice and simple. > > You can just pass the data to the constructor: > > base64.b64encode(hashlib.sha256(block).digest()) Somehow I missed the section in the documentation about this API. Thanks. Now it probably indeed makes sense for this to be inline. https://codereview.adblockplus.org/29345751/diff/29347322/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347322/packagerEdge.py#new... packagerEdge.py:154: files.read(path, 'Assets/{}'.format(name)) On 2016/07/08 13:59:12, Sebastian Noack wrote: > Use the + operator here? Done.
https://codereview.adblockplus.org/29345751/diff/29347396/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347396/docs/metadata.edge.... docs/metadata.edge.example:21: edge = 37.14332.1000.0 would this actually be used? It seems we only have min/max "windows" versions in the generated AppxMainfest.xml? (In test/metadata.edge you specify both) https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:40: {'hash': base64.b64encode(hashlib.sha256(block).digest())} Wrapping the value into a dict seems unnecessary. https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:65: raise KeyError('{} is not found in files'.format(path)) Use the + operator here? https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:85: 'xml': 'text/xml' Isn't that overridden below? https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:94: ext = filename.split('.')[-1] Use os.path.splitext? https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:96: # The return value can be a string or a tuple: Can it? I think it is always a tuple, or do I miss something in the docs?
https://codereview.adblockplus.org/29345751/diff/29347396/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347396/docs/metadata.edge.... docs/metadata.edge.example:21: edge = 37.14332.1000.0 On 2016/07/11 16:29:44, Sebastian Noack wrote: > would this actually be used? It seems we only have min/max "windows" versions in > the generated AppxMainfest.xml? (In test/metadata.edge you specify both) This end up in `minimum_edge_version` key in `manifest.json` inside of `Extension` folder. That's how it is in the example extension that you provided me with. However, min/max Windows versions should be documented here, so I added that. https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:40: {'hash': base64.b64encode(hashlib.sha256(block).digest())} On 2016/07/11 16:29:45, Sebastian Noack wrote: > Wrapping the value into a dict seems unnecessary. The dictionary might also contain the `compressed_size` key. Right now it's not used since compression is not implemented, but since we're planning to add the compression soon (#4149), removing the code related to compressed size from here, template and tests seems excessive. As we've agreed over IRC, I'm leaving this as is. https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:65: raise KeyError('{} is not found in files'.format(path)) On 2016/07/11 16:29:45, Sebastian Noack wrote: > Use the + operator here? Done. https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:85: 'xml': 'text/xml' On 2016/07/11 16:29:44, Sebastian Noack wrote: > Isn't that overridden below? Indeed it is, but since I'm not sure how the content map is interpreted, I was thinking that it would be safer to have a default here too. https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:94: ext = filename.split('.')[-1] On 2016/07/11 16:29:45, Sebastian Noack wrote: > Use os.path.splitext? Done. https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:96: # The return value can be a string or a tuple: On 2016/07/11 16:29:44, Sebastian Noack wrote: > Can it? I think it is always a tuple, or do I miss something in the docs? "The return value is a tuple (type, encoding) where type is None if the type can’t be guessed (missing or unknown suffix) or a string of the form 'type/subtype', usable for a MIME content-type header." Actually you're right, it makes much more sense to interpret the "or" as belonging to "where type is None...." clause rather than to "is a tuple ..." clause. I was really annoyed at this ridiculous API but actually it's my reading comprehension that's the problem. Fixed.
https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:85: 'xml': 'text/xml' On 2016/07/12 09:38:08, Vasily Kuznetsov wrote: > On 2016/07/11 16:29:44, Sebastian Noack wrote: > > Isn't that overridden below? > > Indeed it is, but since I'm not sure how the content map is interpreted, I was > thinking that it would be safer to have a default here too. I mean, isn't the value here overridden by the logic below, since there is always an *.xml file in the list of files? And in the theoretical scenario, there wouldn't be any *.xml file we wouldn't need to add that default anyway. https://codereview.adblockplus.org/29345751/diff/29347692/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347692/packagerEdge.py#new... packagerEdge.py:88: '/AppxBlockMap.xml': 'application/vnd.ms-appx.blockmap+xml', I just noticed that you define those filenames by some globals. So I guess we should use them here as well? https://codereview.adblockplus.org/29345751/diff/29347692/packagerEdge.py#new... packagerEdge.py:93: if '.' in filename[1:]: It seems, now where you use os.path.splitext(), this logic can be simplified a bit: ext = os.path.splitext(filename)[1] if ext: ...
https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:85: 'xml': 'text/xml' On 2016/07/12 21:15:25, Sebastian Noack wrote: > On 2016/07/12 09:38:08, Vasily Kuznetsov wrote: > > On 2016/07/11 16:29:44, Sebastian Noack wrote: > > > Isn't that overridden below? > > > > Indeed it is, but since I'm not sure how the content map is interpreted, I was > > thinking that it would be safer to have a default here too. > > I mean, isn't the value here overridden by the logic below, since there is > always an *.xml file in the list of files? And in the theoretical scenario, > there wouldn't be any *.xml file we wouldn't need to add that default anyway. My thinking was that we are normally adding the content-types file before the blockmap but it needs to have a record for blockmap. So if there are no other XML files, the default for XML will not be there and it might not be processed right. Anyway, this assumption that the blockmap will be added later is not a good thing. I rewrote this function to make things more explicit. See how you like it now. https://codereview.adblockplus.org/29345751/diff/29347692/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347692/packagerEdge.py#new... packagerEdge.py:88: '/AppxBlockMap.xml': 'application/vnd.ms-appx.blockmap+xml', On 2016/07/12 21:15:26, Sebastian Noack wrote: > I just noticed that you define those filenames by some globals. So I guess we > should use them here as well? Done. https://codereview.adblockplus.org/29345751/diff/29347692/packagerEdge.py#new... packagerEdge.py:93: if '.' in filename[1:]: On 2016/07/12 21:15:26, Sebastian Noack wrote: > It seems, now where you use os.path.splitext(), this logic can be simplified a > bit: > > ext = os.path.splitext(filename)[1] > if ext: > ... Done.
https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:85: 'xml': 'text/xml' On 2016/07/13 10:35:31, Vasily Kuznetsov wrote: > On 2016/07/12 21:15:25, Sebastian Noack wrote: > > On 2016/07/12 09:38:08, Vasily Kuznetsov wrote: > > > On 2016/07/11 16:29:44, Sebastian Noack wrote: > > > > Isn't that overridden below? > > > > > > Indeed it is, but since I'm not sure how the content map is interpreted, I > was > > > thinking that it would be safer to have a default here too. > > > > I mean, isn't the value here overridden by the logic below, since there is > > always an *.xml file in the list of files? And in the theoretical scenario, > > there wouldn't be any *.xml file we wouldn't need to add that default anyway. > > My thinking was that we are normally adding the content-types file before the > blockmap but it needs to have a record for blockmap. So if there are no other > XML files, the default for XML will not be there and it might not be processed > right. > > Anyway, this assumption that the blockmap will be added later is not a good > thing. I rewrote this function to make things more explicit. See how you like it > now. Do we even need to consider AppxBlockMap.xml and AppxManifest.xml for the defaults, since they have their mimetype explicitly specified below? I guess not. So the overrides could be hard-coded in the template and defaults can be initialized with an empty dict, significantly simplifying this implementation.
https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:85: 'xml': 'text/xml' On 2016/07/13 16:29:22, Sebastian Noack wrote: > On 2016/07/13 10:35:31, Vasily Kuznetsov wrote: > > On 2016/07/12 21:15:25, Sebastian Noack wrote: > > > On 2016/07/12 09:38:08, Vasily Kuznetsov wrote: > > > > On 2016/07/11 16:29:44, Sebastian Noack wrote: > > > > > Isn't that overridden below? > > > > > > > > Indeed it is, but since I'm not sure how the content map is interpreted, I > > was > > > > thinking that it would be safer to have a default here too. > > > > > > I mean, isn't the value here overridden by the logic below, since there is > > > always an *.xml file in the list of files? And in the theoretical scenario, > > > there wouldn't be any *.xml file we wouldn't need to add that default > anyway. > > > > My thinking was that we are normally adding the content-types file before the > > blockmap but it needs to have a record for blockmap. So if there are no other > > XML files, the default for XML will not be there and it might not be processed > > right. > > > > Anyway, this assumption that the blockmap will be added later is not a good > > thing. I rewrote this function to make things more explicit. See how you like > it > > now. > > Do we even need to consider AppxBlockMap.xml and AppxManifest.xml for the > defaults, since they have their mimetype explicitly specified below? I guess > not. So the overrides could be hard-coded in the template and defaults can be > initialized with an empty dict, significantly simplifying this implementation. Yeah, this is possible, but we'll later have other overrides, for example for the files related to signing the package. I guess we can always hardcode all overrides but I mildly dislike that in this case we'd have half of mime type information in the template and the other half computed in the code.
https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29347396/packagerEdge.py#new... packagerEdge.py:85: 'xml': 'text/xml' On 2016/07/13 17:36:45, Vasily Kuznetsov wrote: > On 2016/07/13 16:29:22, Sebastian Noack wrote: > > On 2016/07/13 10:35:31, Vasily Kuznetsov wrote: > > > On 2016/07/12 21:15:25, Sebastian Noack wrote: > > > > On 2016/07/12 09:38:08, Vasily Kuznetsov wrote: > > > > > On 2016/07/11 16:29:44, Sebastian Noack wrote: > > > > > > Isn't that overridden below? > > > > > > > > > > Indeed it is, but since I'm not sure how the content map is interpreted, > I > > > was > > > > > thinking that it would be safer to have a default here too. > > > > > > > > I mean, isn't the value here overridden by the logic below, since there is > > > > always an *.xml file in the list of files? And in the theoretical > scenario, > > > > there wouldn't be any *.xml file we wouldn't need to add that default > > anyway. > > > > > > My thinking was that we are normally adding the content-types file before > the > > > blockmap but it needs to have a record for blockmap. So if there are no > other > > > XML files, the default for XML will not be there and it might not be > processed > > > right. > > > > > > Anyway, this assumption that the blockmap will be added later is not a good > > > thing. I rewrote this function to make things more explicit. See how you > like > > it > > > now. > > > > Do we even need to consider AppxBlockMap.xml and AppxManifest.xml for the > > defaults, since they have their mimetype explicitly specified below? I guess > > not. So the overrides could be hard-coded in the template and defaults can be > > initialized with an empty dict, significantly simplifying this implementation. > > Yeah, this is possible, but we'll later have other overrides, for example for > the files related to signing the package. I guess we can always hardcode all > overrides but I mildly dislike that in this case we'd have half of mime type > information in the template and the other half computed in the code. The package is signed by the Windows Store. In fact, we have to upload an unsigned package. So I guess we can ignore this theoretical scenario. Having dynamic logic in code and static content in the template sounds reasonable to me.
https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:7: name = ACME.MyExt I just noticed, this setting is redundant. The part before the dot is redundant with "author" in the "general" section. And the part after the dot can be retrieved from "name" message in the source translation. Just remove any spaces. https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:31: description = MyExt is the greatest extension This is redundant as well. The description can be retrieved from the source translation. https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:37: extension_id = 1.0 This can be hard-coded to 1.0 in the template. Since there is only one entry point it doesn't have any effect anyway. https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:59: managedStorageSchema = schema.json Since Edge doesn't support managed storage (the key in the generated manifest.json is ignored) it's rather misleading to document it here. Same for the devtools option above. Or perhaps, just change the name of this file to be platform agnostic.
https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:37: extension_id = 1.0 On 2016/07/13 18:10:37, Sebastian Noack wrote: > This can be hard-coded to 1.0 in the template. Since there is only one entry > point it doesn't have any effect anyway. This attribute just changed, for Edge extensions it has to be set to "EdgeExtension", rather than being floating point number, now. Even less useful it is to have it configurable.
I think I addressed all the outstanding comments. Unfortunately after rebasing the changes on top of things that landed in master in the mean time, Rietveld is showing some of those changes are part of my commit. That's not actually the case. Please ignore changes to `build.py` and `packagerChrome.py`. I will test the produced package with the Windows Store this week to resolve the question about compression and see if anything else need to be changed. https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:7: name = ACME.MyExt On 2016/07/13 18:10:37, Sebastian Noack wrote: > I just noticed, this setting is redundant. The part before the dot is redundant > with "author" in the "general" section. And the part after the dot can be > retrieved from "name" message in the source translation. Just remove any spaces. If I understand correctly this thing is more like an id. I think it might be useful to have it here explicitly so it doesn't get messed up because of changes to UI text. https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:31: description = MyExt is the greatest extension On 2016/07/13 18:10:38, Sebastian Noack wrote: > This is redundant as well. The description can be retrieved from the source > translation. Done https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:37: extension_id = 1.0 On 2016/09/13 10:41:23, Sebastian Noack wrote: > On 2016/07/13 18:10:37, Sebastian Noack wrote: > > This can be hard-coded to 1.0 in the template. Since there is only one entry > > point it doesn't have any effect anyway. > > This attribute just changed, for Edge extensions it has to be set to > "EdgeExtension", rather than being floating point number, now. Even less useful > it is to have it configurable. Done. https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:59: managedStorageSchema = schema.json On 2016/07/13 18:10:37, Sebastian Noack wrote: > Since Edge doesn't support managed storage (the key in the generated > manifest.json is ignored) it's rather misleading to document it here. Same for > the devtools option above. Or perhaps, just change the name of this file to be > platform agnostic. Making this example platform agnostic is more complicated since different metadata files have different keys. I'll keep it simple and remove the redundant stuff. Done.
https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:7: name = ACME.MyExt On 2016/10/04 14:46:45, Vasily Kuznetsov wrote: > On 2016/07/13 18:10:37, Sebastian Noack wrote: > > I just noticed, this setting is redundant. The part before the dot is > redundant > > with "author" in the "general" section. And the part after the dot can be > > retrieved from "name" message in the source translation. Just remove any > spaces. > > If I understand correctly this thing is more like an id. I think it might be > useful to have it here explicitly so it doesn't get messed up because of changes > to UI text. Frankly, I'm not sure whether the Windows Store would except a build that uses a different name than has been reserved on the store and is the part of this generated ID. Same for the author. If it in fact doesn't have to be the same, I see your point. But even then one could argue, that it would be most likely an accident if it's inconsistent, and therefore good to require it to be consistent. Also I wonder whether it's worth to have a separate [package_identity] section. For reference, on Firefox we also have a unique ID and it is under [general]. And "publisher" is just the unique ID for the "author" which is already under [general]. What do you think? https://codereview.adblockplus.org/29345751/diff/29355791/build.py File build.py (right): https://codereview.adblockplus.org/29345751/diff/29355791/build.py#newcode181 build.py:181: if option in ('-l', '--locales') and type == 'gecko': While refactoring this code anyway, this should be a set rather than a tuple, same below. https://codereview.adblockplus.org/29345751/diff/29355791/build.py#newcode205 build.py:205: packager.createBuild(baseDir, type=type, **kwargs) Since you are apparently changing the calling code anyway, how about renaming createBuild() to create_build() in existing modules, rather than adding #noqa in the new code? https://codereview.adblockplus.org/29345751/diff/29355791/packager.py File packager.py (right): https://codereview.adblockplus.org/29345751/diff/29355791/packager.py#newcode137 packager.py:137: def zip(self, outFile, sortKey=None, compress=True): Perhaps it is a little simpler if you just pass the compression argument through? def zip(self, outFile, sortKey=None, compression=zipfile.ZIP_DEFLATED): ... https://codereview.adblockplus.org/29345751/diff/29355791/packager.py#newcode139 packager.py:139: zf = zipfile.ZipFile(outFile, 'w', compression) While changing this code anyway, use with-statement? https://codereview.adblockplus.org/29345751/diff/29355791/templates/edge/Appx... File templates/edge/AppxBlockMap.xml.tmpl (right): https://codereview.adblockplus.org/29345751/diff/29355791/templates/edge/Appx... templates/edge/AppxBlockMap.xml.tmpl:6: <Block Hash="{{block.hash}}" {% if block.compressed_size %}Size="{{block.compressed_size}}"{% endif %}/> If it turns out that we won't need to add compression later (because it gets repackaged by the Store anyway) this logic should be removed before this lands.
I addressed the comments on the last patch set and refactored the tests a bit (after adding the test for the warning about the possibly wrong app_id it started to make sense to remove duplication). https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:7: name = ACME.MyExt On 2016/10/04 19:58:03, Sebastian Noack wrote: > On 2016/10/04 14:46:45, Vasily Kuznetsov wrote: > > On 2016/07/13 18:10:37, Sebastian Noack wrote: > > > I just noticed, this setting is redundant. The part before the dot is > > redundant > > > with "author" in the "general" section. And the part after the dot can be > > > retrieved from "name" message in the source translation. Just remove any > > spaces. > > > > If I understand correctly this thing is more like an id. I think it might be > > useful to have it here explicitly so it doesn't get messed up because of > changes > > to UI text. > > Frankly, I'm not sure whether the Windows Store would except a build that uses a > different name than has been reserved on the store and is the part of this > generated ID. Same for the author. > > If it in fact doesn't have to be the same, I see your point. But even then one > could argue, that it would be most likely an accident if it's inconsistent, and > therefore good to require it to be consistent. > > Also I wonder whether it's worth to have a separate [package_identity] section. > For reference, on Firefox we also have a unique ID and it is under [general]. > And "publisher" is just the unique ID for the "author" which is already under > [general]. > > What do you think? Probably being consistent with Firefox is a good thing. I moved `name` and `publisher` into [general] renaming them to `app_id` and `publisher_id` (that's what they really are). I also added a warning in case the `app_id` seems mismatched with `author` (from the metadata) and `display_name` (from the default translation). https://codereview.adblockplus.org/29345751/diff/29355791/build.py File build.py (right): https://codereview.adblockplus.org/29345751/diff/29355791/build.py#newcode181 build.py:181: if option in ('-l', '--locales') and type == 'gecko': On 2016/10/04 19:58:04, Sebastian Noack wrote: > While refactoring this code anyway, this should be a set rather than a tuple, > same below. Done. https://codereview.adblockplus.org/29345751/diff/29355791/build.py#newcode205 build.py:205: packager.createBuild(baseDir, type=type, **kwargs) On 2016/10/04 19:58:05, Sebastian Noack wrote: > Since you are apparently changing the calling code anyway, how about renaming > createBuild() to create_build() in existing modules, rather than adding #noqa in > the new code? That's a lot of unrelated changes to pull in. Also `releaseAutomation.py` calls `createBuild` and god knows what else outside of buildtools. I would prefer to make a separate commit for these kinds of renames. https://codereview.adblockplus.org/29345751/diff/29355791/packager.py File packager.py (right): https://codereview.adblockplus.org/29345751/diff/29355791/packager.py#newcode137 packager.py:137: def zip(self, outFile, sortKey=None, compress=True): On 2016/10/04 19:58:05, Sebastian Noack wrote: > Perhaps it is a little simpler if you just pass the compression argument > through? > > def zip(self, outFile, sortKey=None, compression=zipfile.ZIP_DEFLATED): > ... Done. https://codereview.adblockplus.org/29345751/diff/29355791/packager.py#newcode139 packager.py:139: zf = zipfile.ZipFile(outFile, 'w', compression) On 2016/10/04 19:58:05, Sebastian Noack wrote: > While changing this code anyway, use with-statement? Done. https://codereview.adblockplus.org/29345751/diff/29355791/templates/edge/Appx... File templates/edge/AppxBlockMap.xml.tmpl (right): https://codereview.adblockplus.org/29345751/diff/29355791/templates/edge/Appx... templates/edge/AppxBlockMap.xml.tmpl:6: <Block Hash="{{block.hash}}" {% if block.compressed_size %}Size="{{block.compressed_size}}"{% endif %}/> On 2016/10/04 19:58:06, Sebastian Noack wrote: > If it turns out that we won't need to add compression later (because it gets > repackaged by the Store anyway) this logic should be removed before this lands. Done. I also removed all compression-related logic from everywhere else and closed https://issues.adblockplus.org/ticket/4149.
https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:7: name = ACME.MyExt On 2016/10/11 15:58:26, Vasily Kuznetsov wrote: > On 2016/10/04 19:58:03, Sebastian Noack wrote: > > On 2016/10/04 14:46:45, Vasily Kuznetsov wrote: > > > On 2016/07/13 18:10:37, Sebastian Noack wrote: > > > > I just noticed, this setting is redundant. The part before the dot is > > > redundant > > > > with "author" in the "general" section. And the part after the dot can be > > > > retrieved from "name" message in the source translation. Just remove any > > > spaces. > > > > > > If I understand correctly this thing is more like an id. I think it might be > > > useful to have it here explicitly so it doesn't get messed up because of > > changes > > > to UI text. > > > > Frankly, I'm not sure whether the Windows Store would except a build that uses > a > > different name than has been reserved on the store and is the part of this > > generated ID. Same for the author. > > > > If it in fact doesn't have to be the same, I see your point. But even then one > > could argue, that it would be most likely an accident if it's inconsistent, > and > > therefore good to require it to be consistent. > > > > Also I wonder whether it's worth to have a separate [package_identity] > section. > > For reference, on Firefox we also have a unique ID and it is under [general]. > > And "publisher" is just the unique ID for the "author" which is already under > > [general]. > > > > What do you think? > > Probably being consistent with Firefox is a good thing. I moved `name` and > `publisher` into [general] renaming them to `app_id` and `publisher_id` (that's > what they really are). > > I also added a warning in case the `app_id` seems mismatched with `author` (from > the metadata) and `display_name` (from the default translation). Fair enough, for keeping the app ID, rather then auto generating it. Not sure whether the warning is worth the extra complexity though. https://codereview.adblockplus.org/29345751/diff/29355791/build.py File build.py (right): https://codereview.adblockplus.org/29345751/diff/29355791/build.py#newcode205 build.py:205: packager.createBuild(baseDir, type=type, **kwargs) On 2016/10/11 15:58:27, Vasily Kuznetsov wrote: > On 2016/10/04 19:58:05, Sebastian Noack wrote: > > Since you are apparently changing the calling code anyway, how about renaming > > createBuild() to create_build() in existing modules, rather than adding #noqa > in > > the new code? > > That's a lot of unrelated changes to pull in. Also `releaseAutomation.py` calls > `createBuild` and god knows what else outside of buildtools. I would prefer to > make a separate commit for these kinds of renames. I was only talking about packagerEdge.create_build() which isn't used outside of buildtools yet, since it doesn't exist before these changes. Sure, this would be inconsistent (for now) but might be better than introducing new names using camalcase in new code.
https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:7: name = ACME.MyExt On 2016/10/12 11:07:48, Sebastian Noack wrote: > On 2016/10/11 15:58:26, Vasily Kuznetsov wrote: > > On 2016/10/04 19:58:03, Sebastian Noack wrote: > > > On 2016/10/04 14:46:45, Vasily Kuznetsov wrote: > > > > On 2016/07/13 18:10:37, Sebastian Noack wrote: > > > > > I just noticed, this setting is redundant. The part before the dot is > > > > redundant > > > > > with "author" in the "general" section. And the part after the dot can > be > > > > > retrieved from "name" message in the source translation. Just remove any > > > > spaces. > > > > > > > > If I understand correctly this thing is more like an id. I think it might > be > > > > useful to have it here explicitly so it doesn't get messed up because of > > > changes > > > > to UI text. > > > > > > Frankly, I'm not sure whether the Windows Store would except a build that > uses > > a > > > different name than has been reserved on the store and is the part of this > > > generated ID. Same for the author. > > > > > > If it in fact doesn't have to be the same, I see your point. But even then > one > > > could argue, that it would be most likely an accident if it's inconsistent, > > and > > > therefore good to require it to be consistent. > > > > > > Also I wonder whether it's worth to have a separate [package_identity] > > section. > > > For reference, on Firefox we also have a unique ID and it is under > [general]. > > > And "publisher" is just the unique ID for the "author" which is already > under > > > [general]. > > > > > > What do you think? > > > > Probably being consistent with Firefox is a good thing. I moved `name` and > > `publisher` into [general] renaming them to `app_id` and `publisher_id` > (that's > > what they really are). > > > > I also added a warning in case the `app_id` seems mismatched with `author` > (from > > the metadata) and `display_name` (from the default translation). > > Fair enough, for keeping the app ID, rather then auto generating it. Not sure > whether the warning is worth the extra complexity though. Yeah, me neither, actually. I coded it just so we could see how it would look. Happy to remove it if you think it's not worth it. https://codereview.adblockplus.org/29345751/diff/29355791/build.py File build.py (right): https://codereview.adblockplus.org/29345751/diff/29355791/build.py#newcode205 build.py:205: packager.createBuild(baseDir, type=type, **kwargs) On 2016/10/12 11:07:48, Sebastian Noack wrote: > On 2016/10/11 15:58:27, Vasily Kuznetsov wrote: > > On 2016/10/04 19:58:05, Sebastian Noack wrote: > > > Since you are apparently changing the calling code anyway, how about > renaming > > > createBuild() to create_build() in existing modules, rather than adding > #noqa > > in > > > the new code? > > > > That's a lot of unrelated changes to pull in. Also `releaseAutomation.py` > calls > > `createBuild` and god knows what else outside of buildtools. I would prefer to > > make a separate commit for these kinds of renames. > > I was only talking about packagerEdge.create_build() which isn't used outside of > buildtools yet, since it doesn't exist before these changes. Sure, this would be > inconsistent (for now) but might be better than introducing new names using > camalcase in new code. I tried doing it, but unfortunately we still can't remove the #noqa. There are two other naming issues in the definition of `packagerEdge.createBuild`: one argument is called `type` (that redefines a builtin) and all other arguments are in camelcase. The first issue is easy to address by removing `type` from the signature and adding a `**kw` (ugly but works) but the second one would require either putting just `*args, **kw` for arguments and then reconstructing them manually or calling `packagerEdge.create_build` in a different way from all other `createBuild` functions. I don't like either of these options so this leaves us with the choice between renaming `packagerEdge.createBuild` to `create_build` and still having the #noqa or just leaving things as they are. It seems like renaming is not really worth it. What do you think?
https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:7: name = ACME.MyExt On 2016/10/12 14:13:42, Vasily Kuznetsov wrote: > On 2016/10/12 11:07:48, Sebastian Noack wrote: > > On 2016/10/11 15:58:26, Vasily Kuznetsov wrote: > > > On 2016/10/04 19:58:03, Sebastian Noack wrote: > > > > On 2016/10/04 14:46:45, Vasily Kuznetsov wrote: > > > > > On 2016/07/13 18:10:37, Sebastian Noack wrote: > > > > > > I just noticed, this setting is redundant. The part before the dot is > > > > > redundant > > > > > > with "author" in the "general" section. And the part after the dot can > > be > > > > > > retrieved from "name" message in the source translation. Just remove > any > > > > > spaces. > > > > > > > > > > If I understand correctly this thing is more like an id. I think it > might > > be > > > > > useful to have it here explicitly so it doesn't get messed up because of > > > > changes > > > > > to UI text. > > > > > > > > Frankly, I'm not sure whether the Windows Store would except a build that > > uses > > > a > > > > different name than has been reserved on the store and is the part of this > > > > generated ID. Same for the author. > > > > > > > > If it in fact doesn't have to be the same, I see your point. But even then > > one > > > > could argue, that it would be most likely an accident if it's > inconsistent, > > > and > > > > therefore good to require it to be consistent. > > > > > > > > Also I wonder whether it's worth to have a separate [package_identity] > > > section. > > > > For reference, on Firefox we also have a unique ID and it is under > > [general]. > > > > And "publisher" is just the unique ID for the "author" which is already > > under > > > > [general]. > > > > > > > > What do you think? > > > > > > Probably being consistent with Firefox is a good thing. I moved `name` and > > > `publisher` into [general] renaming them to `app_id` and `publisher_id` > > (that's > > > what they really are). > > > > > > I also added a warning in case the `app_id` seems mismatched with `author` > > (from > > > the metadata) and `display_name` (from the default translation). > > > > Fair enough, for keeping the app ID, rather then auto generating it. Not sure > > whether the warning is worth the extra complexity though. > > Yeah, me neither, actually. I coded it just so we could see how it would look. > Happy to remove it if you think it's not worth it. I leave it up to you, but FWIW, no I don't think it's worth it. https://codereview.adblockplus.org/29345751/diff/29355791/build.py File build.py (right): https://codereview.adblockplus.org/29345751/diff/29355791/build.py#newcode205 build.py:205: packager.createBuild(baseDir, type=type, **kwargs) On 2016/10/12 14:13:42, Vasily Kuznetsov wrote: > On 2016/10/12 11:07:48, Sebastian Noack wrote: > > On 2016/10/11 15:58:27, Vasily Kuznetsov wrote: > > > On 2016/10/04 19:58:05, Sebastian Noack wrote: > > > > Since you are apparently changing the calling code anyway, how about > > renaming > > > > createBuild() to create_build() in existing modules, rather than adding > > #noqa > > > in > > > > the new code? > > > > > > That's a lot of unrelated changes to pull in. Also `releaseAutomation.py` > > calls > > > `createBuild` and god knows what else outside of buildtools. I would prefer > to > > > make a separate commit for these kinds of renames. > > > > I was only talking about packagerEdge.create_build() which isn't used outside > of > > buildtools yet, since it doesn't exist before these changes. Sure, this would > be > > inconsistent (for now) but might be better than introducing new names using > > camalcase in new code. > > I tried doing it, but unfortunately we still can't remove the #noqa. There are > two other naming issues in the definition of `packagerEdge.createBuild`: one > argument is called `type` (that redefines a builtin) and all other arguments are > in camelcase. The first issue is easy to address by removing `type` from the > signature and adding a `**kw` (ugly but works) but the second one would require > either putting just `*args, **kw` for arguments and then reconstructing them > manually or calling `packagerEdge.create_build` in a different way from all > other `createBuild` functions. I don't like either of these options so this > leaves us with the choice between renaming `packagerEdge.createBuild` to > `create_build` and still having the #noqa or just leaving things as they are. It > seems like renaming is not really worth it. What do you think? I guess, let's leave it as is for now. We still should probably change the calling convention for all packagers in one go with a different issue. https://codereview.adblockplus.org/29345751/diff/29356522/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29356522/packagerEdge.py#new... packagerEdge.py:141: 'version': version, As figured out during testing, and discussed on IRC, the version must have 4 parts. So we should bad it with zeros here.
https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... File docs/metadata.edge.example (right): https://codereview.adblockplus.org/29345751/diff/29347731/docs/metadata.edge.... docs/metadata.edge.example:7: name = ACME.MyExt On 2016/10/12 14:43:25, Sebastian Noack wrote: > On 2016/10/12 14:13:42, Vasily Kuznetsov wrote: > > On 2016/10/12 11:07:48, Sebastian Noack wrote: > > > On 2016/10/11 15:58:26, Vasily Kuznetsov wrote: > > > > On 2016/10/04 19:58:03, Sebastian Noack wrote: > > > > > On 2016/10/04 14:46:45, Vasily Kuznetsov wrote: > > > > > > On 2016/07/13 18:10:37, Sebastian Noack wrote: > > > > > > > I just noticed, this setting is redundant. The part before the dot > is > > > > > > redundant > > > > > > > with "author" in the "general" section. And the part after the dot > can > > > be > > > > > > > retrieved from "name" message in the source translation. Just remove > > any > > > > > > spaces. > > > > > > > > > > > > If I understand correctly this thing is more like an id. I think it > > might > > > be > > > > > > useful to have it here explicitly so it doesn't get messed up because > of > > > > > changes > > > > > > to UI text. > > > > > > > > > > Frankly, I'm not sure whether the Windows Store would except a build > that > > > uses > > > > a > > > > > different name than has been reserved on the store and is the part of > this > > > > > generated ID. Same for the author. > > > > > > > > > > If it in fact doesn't have to be the same, I see your point. But even > then > > > one > > > > > could argue, that it would be most likely an accident if it's > > inconsistent, > > > > and > > > > > therefore good to require it to be consistent. > > > > > > > > > > Also I wonder whether it's worth to have a separate [package_identity] > > > > section. > > > > > For reference, on Firefox we also have a unique ID and it is under > > > [general]. > > > > > And "publisher" is just the unique ID for the "author" which is already > > > under > > > > > [general]. > > > > > > > > > > What do you think? > > > > > > > > Probably being consistent with Firefox is a good thing. I moved `name` and > > > > `publisher` into [general] renaming them to `app_id` and `publisher_id` > > > (that's > > > > what they really are). > > > > > > > > I also added a warning in case the `app_id` seems mismatched with `author` > > > (from > > > > the metadata) and `display_name` (from the default translation). > > > > > > Fair enough, for keeping the app ID, rather then auto generating it. Not > sure > > > whether the warning is worth the extra complexity though. > > > > Yeah, me neither, actually. I coded it just so we could see how it would look. > > Happy to remove it if you think it's not worth it. > > I leave it up to you, but FWIW, no I don't think it's worth it. Me neither. Done https://codereview.adblockplus.org/29345751/diff/29355791/build.py File build.py (right): https://codereview.adblockplus.org/29345751/diff/29355791/build.py#newcode205 build.py:205: packager.createBuild(baseDir, type=type, **kwargs) On 2016/10/12 14:43:26, Sebastian Noack wrote: > On 2016/10/12 14:13:42, Vasily Kuznetsov wrote: > > On 2016/10/12 11:07:48, Sebastian Noack wrote: > > > On 2016/10/11 15:58:27, Vasily Kuznetsov wrote: > > > > On 2016/10/04 19:58:05, Sebastian Noack wrote: > > > > > Since you are apparently changing the calling code anyway, how about > > > renaming > > > > > createBuild() to create_build() in existing modules, rather than adding > > > #noqa > > > > in > > > > > the new code? > > > > > > > > That's a lot of unrelated changes to pull in. Also `releaseAutomation.py` > > > calls > > > > `createBuild` and god knows what else outside of buildtools. I would > prefer > > to > > > > make a separate commit for these kinds of renames. > > > > > > I was only talking about packagerEdge.create_build() which isn't used > outside > > of > > > buildtools yet, since it doesn't exist before these changes. Sure, this > would > > be > > > inconsistent (for now) but might be better than introducing new names using > > > camalcase in new code. > > > > I tried doing it, but unfortunately we still can't remove the #noqa. There are > > two other naming issues in the definition of `packagerEdge.createBuild`: one > > argument is called `type` (that redefines a builtin) and all other arguments > are > > in camelcase. The first issue is easy to address by removing `type` from the > > signature and adding a `**kw` (ugly but works) but the second one would > require > > either putting just `*args, **kw` for arguments and then reconstructing them > > manually or calling `packagerEdge.create_build` in a different way from all > > other `createBuild` functions. I don't like either of these options so this > > leaves us with the choice between renaming `packagerEdge.createBuild` to > > `create_build` and still having the #noqa or just leaving things as they are. > It > > seems like renaming is not really worth it. What do you think? > > I guess, let's leave it as is for now. We still should probably change the > calling convention for all packagers in one go with a different issue. Fully agree. https://codereview.adblockplus.org/29345751/diff/29356522/packagerEdge.py File packagerEdge.py (right): https://codereview.adblockplus.org/29345751/diff/29356522/packagerEdge.py#new... packagerEdge.py:141: 'version': version, On 2016/10/12 14:43:27, Sebastian Noack wrote: > As figured out during testing, and discussed on IRC, the version must have 4 > parts. So we should bad it with zeros here. I've done it inside of appx manifest generation instead. Should be fine too.
Patch set 10 fixes the issues with Windows Store validation of the package: blockmap and display name.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
