Moved Chrome extension scripts to buildtools repository

Note: This is a very low-priority review, more of FYI actually. For historical reasons we had a set of scripts in the adblockpluschrome repository instead of using buildtools. This functionality was now moved to buildtools and is accessible as build.py build, build.py synclocales and build.py updatepsl commands. In addition, the Crowdin integration was made to work for Chrome extensions as well (mostly, build.py gettranslations command still needs adapting) and extended with a new build.py uploadtrans command to upload existing translations. Architectural change: Originally, only entire build.py commands could be limited to particular extension types - now the same can be done for command options as well (some build.py build command options are relevant only for Firefox while --experimentalAPI is Chrome-only).

[Felix Dahlke, 2013-01-10 16:36:20]

http://codereview.adblockplus.org/8627097/diff/3001/build.py
File build.py (right):

http://codereview.adblockplus.org/8627097/diff/3001/build.py#newcode192
build.py:192: elif type == 'kmeleon':
Shouldn't we get rid of this while we're at it?

http://codereview.adblockplus.org/8627097/diff/3001/build.py#newcode230
build.py:230: else:
Shouldn't we check for the type here as well and have an else clause with an
error for unknown types? Or did we already make sure that the type is either
"chrome" or "gecko"?

http://codereview.adblockplus.org/8627097/diff/3001/build.py#newcode252
build.py:252: else:
Same as above.

http://codereview.adblockplus.org/8627097/diff/3001/build.py#newcode277
build.py:277: else:
Same as above.

http://codereview.adblockplus.org/8627097/diff/3001/build.py#newcode298
build.py:298: if type == 'chrome':
Same as above.

http://codereview.adblockplus.org/8627097/diff/3001/localeTools.py
File localeTools.py (right):

http://codereview.adblockplus.org/8627097/diff/3001/localeTools.py#newcode245
localeTools.py:245: key = re.sub(r'\..*', '', file) + '_' + re.sub(r'\W', '_',
stringID)
We have pretty much the same line in localeSyncChrome.py:82, can we avoid that?
To me, it looks like this part would feel more at home in localeSyncChrome.py
anyway - I think it makes sense to have creation and removal close to each
other.

http://codereview.adblockplus.org/8627097/diff/3001/localeTools.py#newcode312
localeTools.py:312: boundary = '----------ThIs_Is_tHe_bouNdaRY_$'
Would it make sense to generate this randomly?

http://codereview.adblockplus.org/8627097/diff/3001/packagerChrome.py
File packagerChrome.py (right):

http://codereview.adblockplus.org/8627097/diff/3001/packagerChrome.py#newcode44
packagerChrome.py:44: return json.dumps(data, sort_keys=True, indent=2)
Pretty much the same line is repeated a few times below. Maybe it makes sense to
put this in a function?

http://codereview.adblockplus.org/8627097/diff/3001/packagerChrome.py#newcode89
packagerChrome.py:89: file == 'buildtools' or file == 'qunit' or file ==
'metadata' or
How about this instead?

file in ['buildtools', 'qunit', 'metadata']

http://codereview.adblockplus.org/8627097/diff/3001/packagerChrome.py#newcode90
packagerChrome.py:90: filelc.endswith('.py') or filelc.endswith('.pyc') or
How about using a regex here? Definitely more concise:

re.match(r".*\.(py|pyc|crx|zip|sh|bat|txt)$", filelc)

[Wladimir Palant, 2013-01-16 14:20:14]

http://codereview.adblockplus.org/8627097/diff/3001/build.py
File build.py (right):

http://codereview.adblockplus.org/8627097/diff/3001/build.py#newcode192
build.py:192: elif type == 'kmeleon':
On 2013/01/10 16:36:20, Felix H. Dahlke wrote:
> Shouldn't we get rid of this while we're at it?

This was done in a separate batch of changes.

http://codereview.adblockplus.org/8627097/diff/3001/build.py#newcode230
build.py:230: else:
On 2013/01/10 16:36:20, Felix H. Dahlke wrote:
> Shouldn't we check for the type here as well and have an else clause with an
> error for unknown types? Or did we already make sure that the type is either
> "chrome" or "gecko"?

Yes, that command is only available for the chrome and gecko build types - so we
can assume that it won't be called for anything else. My general approach was to
keep the Gecko packager as the default and only add the Chrome packager as the
special case wherever necessary. I can change it of course if you think it's
worth it.

http://codereview.adblockplus.org/8627097/diff/3001/localeTools.py
File localeTools.py (right):

http://codereview.adblockplus.org/8627097/diff/3001/localeTools.py#newcode245
localeTools.py:245: key = re.sub(r'\..*', '', file) + '_' + re.sub(r'\W', '_',
stringID)
On 2013/01/10 16:36:20, Felix H. Dahlke wrote:
> We have pretty much the same line in localeSyncChrome.py:82, can we avoid
that?

Yes, the idea is to make locale syncing part of the build process rather than a
separate manual step. Then we wouldn't have to keep duplicates of Firefox
translations in the repository and this entire pre-processing step would become
unnecessary.

http://codereview.adblockplus.org/8627097/diff/3001/localeTools.py#newcode312
localeTools.py:312: boundary = '----------ThIs_Is_tHe_bouNdaRY_$'
On 2013/01/10 16:36:20, Felix H. Dahlke wrote:
> Would it make sense to generate this randomly?

I'm not sure where this boundary string originates from but at the very least it
is the default boundary for Ruby's Net::HTTP class. Given that our payload is
known to be translations - it's a safe bet that we won't ever have issues using
that static boundary string.

If we want to implement this properly then we need to check the payload to
verify that it doesn't contain instances of our boundary string - and choose a
different boundary if it does. That's complicated enough that I didn't consider
it worth the effort in this case.

http://codereview.adblockplus.org/8627097/diff/3001/packagerChrome.py
File packagerChrome.py (right):

http://codereview.adblockplus.org/8627097/diff/3001/packagerChrome.py#newcode44
packagerChrome.py:44: return json.dumps(data, sort_keys=True, indent=2)
On 2013/01/10 16:36:20, Felix H. Dahlke wrote:
> Pretty much the same line is repeated a few times below. Maybe it makes sense
to
> put this in a function?

Actually, it's four occasions and only two are really identical (both change
locale files).

There is nothing special about these JSON parameters - I merely chose them
because they are readable. In fact, the parameters are slightly different in all
other cases (e.g. localeTools use ensure_ascii=False which isn't important here
and after downloading we remove the space after commas which also isn't
important in all the other scenarios). Frankly, I don't really see much value in
unifying this.

http://codereview.adblockplus.org/8627097/diff/3001/packagerChrome.py#newcode89
packagerChrome.py:89: file == 'buildtools' or file == 'qunit' or file ==
'metadata' or
On 2013/01/10 16:36:20, Felix H. Dahlke wrote:
> How about this instead?
> 
> file in ['buildtools', 'qunit', 'metadata']

This code has been replaced by a whitelist approach by now, it's much more
concise now. You can see it in function getPackageFiles().

http://codereview.adblockplus.org/8627097/diff/3001/packagerChrome.py#newcode90
packagerChrome.py:90: filelc.endswith('.py') or filelc.endswith('.pyc') or
On 2013/01/10 16:36:20, Felix H. Dahlke wrote:
> How about using a regex here? Definitely more concise:
> 
> re.match(r".*\.(py|pyc|crx|zip|sh|bat|txt)$", filelc)

See above.