Issue 29716693: Issue 6371 - Update buildtools dep. to c830dfa08e2f, use AMO-signing API (Closed)
|
Created:
March 7, 2018, 10:05 p.m. by tlucas Modified:
March 22, 2018, 8:01 a.m. CC:
mathias, kzar Base URL:
https://hg.adblockplus.org/abpssembly/file/1e38c3375fa3 Visibility:
Public. |
DescriptionIssue 6371 - Update buildtools dep. to c830dfa08e2f, use AMO-signing API
* Refactored some reused logic
* Implemented full usage of the AMO signing api
* Introduced a lockfile-mechanism for extensions that have to be downloaded
* Introduce a "--download" parameter, causing createNightlies.py to only download files (this is to be called by a new cronjob)
** IMPORTANT **
This and https://codereview.adblockplus.org/29716699 have to land simultaneously.
Patch Set 1 #
Total comments: 25
Patch Set 2 : #
Total comments: 11
Patch Set 3 : #
Total comments: 3
Patch Set 4 : #Patch Set 5 : NO CHANGE - Rebase against current master #
MessagesTotal messages: 12
Patch Set 1 https://codereview.adblockplus.org/29716693/diff/29716694/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/ensure_dependencies... ensure_dependencies.py:1: #!/usr/bin/env python NOTE: All changes in this file are a result of the dependency update. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:351: if self.config.type == 'safari': Fully adapting to #6021 here (since the 'safari'-target does not yet have it included)
https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:422: def open_downloads_loackfile(self): Typo: loackfile => lockfile (or lock_file) https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:422: def open_downloads_loackfile(self): Shouldn't this be "read" rather than "open"? https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:424: current = {} This can go into the except block. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:454: current.pop(platform) You can use the del statement. We don't need it's value (returned by pop). https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:545: Nit: This blank line seems redundant. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:556: config.get('extensions', 'nightliesDirectory'), basename) Nit: When wrapping right after opening parenthesis, please put closing parenthesis in the same column where the line with the opening parentheses start. This is easier to read. file_path = os.path.join( config.get('extensions', 'nightliesDirectory'), basename ) Same below. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:569: sha256.update(file_content) You can pass the data to the sha256 constructor. No need for a separate call to update(). https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:580: config.get('extensions', 'nightliesURL'), self.basename, Nit: This is easier to read if you put each argument on a separate line. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:886: Nit: This blank line seems redundant. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... File sitescripts/extensions/template/gecko.json (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/template/gecko.json:3: {%- for extension in extensions %} Nit: The indentation is inconsistent with the {% enfor %} statement. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/template/gecko.json:6: { "version": "{{ extension.version }}", Please wrap objects consistently: { "version": ... ... }
Patch Set 2 * Addressed Sebastian's comments * Made method-naming consistent https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:422: def open_downloads_loackfile(self): On 2018/03/08 02:39:21, Sebastian Noack wrote: > Shouldn't this be "read" rather than "open"? Yes, you're right - Done. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:424: current = {} On 2018/03/08 02:39:22, Sebastian Noack wrote: > This can go into the except block. Done. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:454: current.pop(platform) On 2018/03/08 02:39:22, Sebastian Noack wrote: > You can use the del statement. We don't need it's value (returned by pop). Done. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:545: On 2018/03/08 02:39:22, Sebastian Noack wrote: > Nit: This blank line seems redundant. Done. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:556: config.get('extensions', 'nightliesDirectory'), basename) On 2018/03/08 02:39:22, Sebastian Noack wrote: > Nit: When wrapping right after opening parenthesis, please put closing > parenthesis in the same column where the line with the opening parentheses > start. This is easier to read. > > file_path = os.path.join( > config.get('extensions', 'nightliesDirectory'), basename > ) > > Same below. Done. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:569: sha256.update(file_content) On 2018/03/08 02:39:22, Sebastian Noack wrote: > You can pass the data to the sha256 constructor. No need for a separate call to > update(). Done. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:580: config.get('extensions', 'nightliesURL'), self.basename, On 2018/03/08 02:39:22, Sebastian Noack wrote: > Nit: This is easier to read if you put each argument on a separate line. Done. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:886: On 2018/03/08 02:39:21, Sebastian Noack wrote: > Nit: This blank line seems redundant. Done. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... File sitescripts/extensions/template/gecko.json (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/template/gecko.json:3: {%- for extension in extensions %} On 2018/03/08 02:39:22, Sebastian Noack wrote: > Nit: The indentation is inconsistent with the {% enfor %} statement. Done. https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/template/gecko.json:6: { "version": "{{ extension.version }}", On 2018/03/08 02:39:22, Sebastian Noack wrote: > Please wrap objects consistently: > > { > "version": ... > ... > } Done.
https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:424: current = {} On 2018/03/08 09:23:05, tlucas wrote: > On 2018/03/08 02:39:22, Sebastian Noack wrote: > > This can go into the except block. > > Done. You put it in the try-block, not in the except-block. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:356: command += ['-b', self.buildNum] You could use list.extend() in order to modify the list in place: command = [os.path.join(self.tempdir, 'build.py')] if self.config.type == 'safari': command.extend(['-t', self.config.type, 'build']) else: command.extend(['build', '-t', self.config.type]) command.extend(['-b', self.buildNum]) https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:368: '00latest%s' % self.config.packageSuffix) Please use .format() instead of %. flake8-eyeo would normally warn about this, but the error is likely ignored for this file. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:556: filename, Nit: Redundant comma after last argument. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:581: filename, Nit: Redundant comma after last argument. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... File sitescripts/extensions/template/gecko.json (right): https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/template/gecko.json:1: { It seems we use 2 (not 4) space indentation for other JSON documents/templates. FWIW, our coding style guide also states to use 2 space indentation, for everything but Python. (Same convention Mozilla has.)
Patch Set 3 Addressed Sebastian's comments (except .format(), see below). https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29716694/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:424: current = {} On 2018/03/09 00:33:18, Sebastian Noack wrote: > On 2018/03/08 09:23:05, tlucas wrote: > > On 2018/03/08 02:39:22, Sebastian Noack wrote: > > > This can go into the except block. > > > > Done. > > You put it in the try-block, not in the except-block. Done. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:356: command += ['-b', self.buildNum] On 2018/03/09 00:33:18, Sebastian Noack wrote: > You could use list.extend() in order to modify the list in place: > > command = [os.path.join(self.tempdir, 'build.py')] > if self.config.type == 'safari': > command.extend(['-t', self.config.type, 'build']) > else: > command.extend(['build', '-t', self.config.type]) > command.extend(['-b', self.buildNum]) Done. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:368: '00latest%s' % self.config.packageSuffix) On 2018/03/09 00:33:19, Sebastian Noack wrote: > Please use .format() instead of %. flake8-eyeo would normally warn about this, > but the error is likely ignored for this file. This is flawed, yes - but .format() should be used for more complex situations. I'll simply use the + operator. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:556: filename, On 2018/03/09 00:33:19, Sebastian Noack wrote: > Nit: Redundant comma after last argument. Done. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:581: filename, On 2018/03/09 00:33:19, Sebastian Noack wrote: > Nit: Redundant comma after last argument. Done. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... File sitescripts/extensions/template/gecko.json (right): https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/template/gecko.json:1: { On 2018/03/09 00:33:19, Sebastian Noack wrote: > It seems we use 2 (not 4) space indentation for other JSON documents/templates. > FWIW, our coding style guide also states to use 2 space indentation, for > everything but Python. (Same convention Mozilla has.) Done.
Looks almost good to me. Just one nit, I missed before. https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29717572/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:368: '00latest%s' % self.config.packageSuffix) On 2018/03/09 08:12:58, tlucas wrote: > On 2018/03/09 00:33:19, Sebastian Noack wrote: > > Please use .format() instead of %. flake8-eyeo would normally warn about this, > > but the error is likely ignored for this file. > > This is flawed, yes - but .format() should be used for more complex situations. > I'll simply use the + operator. You are right, since we just append in the end, + should be used. https://codereview.adblockplus.org/29716693/diff/29718561/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29718561/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:515: (('Content-Type', content_type),), Nit: I missed this one before. Again, comma after last argument. Also the outer data structure should be a list, not at tuple. We only use tuples for data that have structure (e.g. objects that can be unpacked, like the inner structure here), and lists for data that have order (i.e. objects that have a variable number of elements that are processed in a uniform way). self.generate_jwt_request( config.get('extensions', 'amo_key'), config.get('extensions', 'amo_secret'), upload_url, 'PUT', data, [('Content-Type', content_type)] )
Patch Set 4 * Addressed Sebastian's nit https://codereview.adblockplus.org/29716693/diff/29718561/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29718561/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:515: (('Content-Type', content_type),), On 2018/03/09 16:16:27, Sebastian Noack wrote: > Nit: I missed this one before. Again, comma after last argument. > > Also the outer data structure should be a list, not at tuple. We only use tuples > for data that have structure (e.g. objects that can be unpacked, like the inner > structure here), and lists for data that have order (i.e. objects that have a > variable number of elements that are processed in a uniform way). > > self.generate_jwt_request( > config.get('extensions', 'amo_key'), > config.get('extensions', 'amo_secret'), > upload_url, > 'PUT', > data, > [('Content-Type', content_type)] > ) Done.
LGTM
I don't have very much of substance to add, but there's one formatting-related point (see below). It's particularly relevant because I'm thinking about adding some rules for how to indent and structure multiline statements (that we've discussed with Sebastian but never wrote down) to speed up reviews. Sebastian, what is your reason for preferring to have no comma after the last argument? How about list and dict literals that are similarly formatted? Cheers, Vasily https://codereview.adblockplus.org/29716693/diff/29718561/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29716693/diff/29718561/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:515: (('Content-Type', content_type),), On 2018/03/12 07:22:46, tlucas wrote: > On 2018/03/09 16:16:27, Sebastian Noack wrote: > > Nit: I missed this one before. Again, comma after last argument. > > > > Also the outer data structure should be a list, not at tuple. We only use > tuples > > for data that have structure (e.g. objects that can be unpacked, like the > inner > > structure here), and lists for data that have order (i.e. objects that have a > > variable number of elements that are processed in a uniform way). > > > > self.generate_jwt_request( > > config.get('extensions', 'amo_key'), > > config.get('extensions', 'amo_secret'), > > upload_url, > > 'PUT', > > data, > > [('Content-Type', content_type)] > > ) > > Done. I actually prefer the comma after the last argument in such constructions. It allows adding and removing arguments without touching irrelevant lines. This is especially important for something like dict literals, but seems also slightly preferable in cases like this one.
On 2018/03/13 17:50:00, Vasily Kuznetsov wrote: > Sebastian, what is your reason for preferring to have no comma after the last > argument? How about list and dict literals that are similarly formatted? Mostly for consistence. I don't recall haven't seen (m)any code in our code base (or in the wild), where a trailing comma was added after the last argument in a function call. Lists and dictionaries are different, however. Those are variable data structures of uniform items. Adding/Removing items in a list/dict is common, and the trailing comma keeps those changes minimal. The arguments passed to a function, however, are structured. Adding/Removing arguments requires changing the way in which they are processed.
On 2018/03/13 18:01:30, Sebastian Noack wrote: > On 2018/03/13 17:50:00, Vasily Kuznetsov wrote: > > Sebastian, what is your reason for preferring to have no comma after the last > > argument? How about list and dict literals that are similarly formatted? > > Mostly for consistence. I don't recall haven't seen (m)any code in our code base > (or in the wild), where a trailing comma was added after the last argument in a > function call. Lists and dictionaries are different, however. Those are variable > data structures of uniform items. Adding/Removing items in a list/dict is > common, and the trailing comma keeps those changes minimal. The arguments passed > to a function, however, are structured. Adding/Removing arguments requires > changing the way in which they are processed. Indeed arguments of functions change less often, but sometimes they do change. Functions that take variable number of arguments or keyword arguments are rather common in Python and in those cases adding or removing arguments is not all uncommon. It's also simpler to just have the same rule for wrapping and indentation of lists, dicts, sets, tuples and function calls. Syntactically they all look pretty similar and it's easier to just remember one simple rule. In any case, this review seems like a rather bad place to discuss this so don't let me hold you from landing it -- this one instance doesn't matter that much. We can continue in the review for the coding style change that I'm planning to create soon.
On 2018/03/13 21:55:44, Vasily Kuznetsov wrote: > On 2018/03/13 18:01:30, Sebastian Noack wrote: > > On 2018/03/13 17:50:00, Vasily Kuznetsov wrote: > > > Sebastian, what is your reason for preferring to have no comma after the > last > > > argument? How about list and dict literals that are similarly formatted? > > > > Mostly for consistence. I don't recall haven't seen (m)any code in our code > base > > (or in the wild), where a trailing comma was added after the last argument in > a > > function call. Lists and dictionaries are different, however. Those are > variable > > data structures of uniform items. Adding/Removing items in a list/dict is > > common, and the trailing comma keeps those changes minimal. The arguments > passed > > to a function, however, are structured. Adding/Removing arguments requires > > changing the way in which they are processed. > > Indeed arguments of functions change less often, but sometimes they do change. > Functions that take variable number of arguments or keyword arguments are rather > common in Python and in those cases adding or removing arguments is not all > uncommon. Functions that take variable number of arguments and process them in a uniform way (as opposed to passing them through to another function which expects the arguments to be in a particular structure) exist but are rather rare. Usually, it is a better practice to just take a sequence as a single argument (one reason being that it doesn't force the input being converted to a tuple, causing unnecessary overhead in particular when you have a lazily evaluated sequence). > It's also simpler to just have the same rule for wrapping and indentation of > lists, dicts, sets, tuples and function calls. Syntactically they all look > pretty similar and it's easier to just remember one simple rule. I mostly want to avoid forcing some esoteric notation on our coding style, just because it makes a linter rule slightly simpler to implement. Feel free to show me examples (either from our code base or other popular projects written in Python) where argument lists have trailing commas. > In any case, this review seems like a rather bad place to discuss this so don't > let me hold you from landing it -- this one instance doesn't matter that much. > We can continue in the review for the coding style change that I'm planning to > create soon. Yes, this is definitely the wrong place for this discussion. Let's finally land this change and create a separate issue if you want to keep discussing this. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
