Use platform-dependent metadata files to allow multiple builds from the same repository

Use platform-dependent metadata files to allow multiple builds from the same repository

[Felix Dahlke, 2013-01-25 07:49:43]

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

http://codereview.adblockplus.org/9259023/diff/3001/build.py#newcode510
build.py:510: if args[i] == '-t' and i < len(args) - 1:
While there doesn't seem to be a problem with this code, I think it would be
better to use the same option parsing approach as for the toolkit option.

http://codereview.adblockplus.org/9259023/diff/3001/build.py#newcode519
build.py:519: else:
The Mozilla Coding Style says: "Don't put an else right after a return. Delete
the else, it's unnecessary and increases indentation level."

Also happens to be my preference... :)

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

http://codereview.adblockplus.org/9259023/diff/3001/packagerChrome.py#newcode26
packagerChrome.py:26: def getMetadataPath(baseDir):
This function wasn't previously defined or imported here, so it seems to be
unused. Or am I missing something?

[Wladimir Palant, 2013-01-25 08:22:53]

The -t command line flag is now properly documented in the help output, it`s
still a very special case however compared to all other command line flags.

[Wladimir Palant, 2013-01-25 08:23:09]

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

http://codereview.adblockplus.org/9259023/diff/3001/build.py#newcode510
build.py:510: if args[i] == '-t' and i < len(args) - 1:
On 2013/01/25 07:49:43, Felix H. Dahlke wrote:
> While there doesn't seem to be a problem with this code, I think it would be
> better to use the same option parsing approach as for the toolkit option.

Yes, that would be nice indeed. The problem is that getopt isn't too flexible as
far as option parsing goes. But maybe we should really specify that -t has to be
the very first option, even before the command.

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

http://codereview.adblockplus.org/9259023/diff/3001/packagerChrome.py#newcode26
packagerChrome.py:26: def getMetadataPath(baseDir):
On 2013/01/25 07:49:43, Felix H. Dahlke wrote:
> This function wasn't previously defined or imported here, so it seems to be
> unused. Or am I missing something?

You are right. I realized that geckoPackager.getMetadataPath() is actually being
required by the release automation (releaseAutomationGecko.py). So I added it
here as well, for consistency.

[Felix Dahlke, 2013-01-25 08:30:49]

LGTM, apart from the rogue white space.

http://codereview.adblockplus.org/9259023/diff/7001/build.py
File build.py (right):

http://codereview.adblockplus.org/9259023/diff/7001/build.py#newcode511
build.py:511: if len(args) >= 2 and args[0] == '-t':
Hard coding that -t needs to be the very first parameter isn't great, it would
be more flexible if we would parse options before the command as global options
and those after the command as command-specific options, using getopts for each.
But since we really have just one global option right now, this should be fine.

http://codereview.adblockplus.org/9259023/diff/7001/build.py#newcode548
build.py:548: type = getType(baseDir, scriptName,   args)
There's a rogue white space before "args". Utter nit pick, of course :)

[Wladimir Palant, 2013-01-25 10:23:49]

http://codereview.adblockplus.org/9259023/diff/7001/build.py
File build.py (right):

http://codereview.adblockplus.org/9259023/diff/7001/build.py#newcode548
build.py:548: type = getType(baseDir, scriptName,   args)
On 2013/01/25 08:30:49, Felix H. Dahlke wrote:
> There's a rogue white space before "args". Utter nit pick, of course :)

Fixed that before pushing.