[iOS] Add support for iOS to build/mac/tweak_info_plist.py.

[iOS] Add support for iOS to build/mac/tweak_info_plist.py.

In order to allow Chrome on iOS to use build/mac/tweak_info_plist.py,
add options required to control the format of the generated plist file
(--format), the product reported to breakpad (--platform) and a way to
override some portion of the version string (--version-overrides).

This is required to allow Chrome on iOS to migrate from it old fork of
tweak_info_plist shell script and to incorporate tweak_info_plist.py
in the GN build.

BUG=502295
Committed: https://crrev.com/77bf6652e84cfb17925382598e9e50873065b3c2
Cr-Commit-Position: refs/heads/master@{#397745}

[sdefresne, 2016-06-03 12:49:59]

sdefresne@chromium.org changed reviewers:
+ thomasvl@chromium.org

[sdefresne, 2016-06-03 12:50:00]

Please take a look.

Note that Chrome on iOS needs to overrides only some part of the version number
for it's test binary, hence the --version-overrides flag.

[TVL, 2016-06-03 14:02:16]

thomasvl@chromium.org changed reviewers:
+ justincohen@chromium.org, mark@chromium.org
- thomasvl@chromium.org

[sdefresne, 2016-06-03 14:52:50]

sdefresne@chromium.org changed reviewers:
+ thomasvl@chromium.org

[sdefresne, 2016-06-03 14:52:51]

thomasvl: Mark is OOO for a week, if justincohen review the CL, could you give
the OWNERS approval?

[TVL, 2016-06-03 15:34:02]

On 2016/06/03 at 14:52:51, sdefresne wrote:
> thomasvl: Mark is OOO for a week, if justincohen review the CL, could you give
the OWNERS approval?

Sure.

[TVL, 2016-06-03 15:49:12]

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist.py
File build/mac/tweak_info_plist.py (right):

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:251: parser.add_option('--platform',
choices=('ios', 'mac'), default='mac',
Why not "iOS" and "Mac" so you don't have to do a mapping within the breakpad
helper?  The version override forces folks to get the capitalization right for
the overrides.

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:254: help='Key-value pair to override specific
component of version')
Like --version does, you probably want to give an example. Just saying there are
a pair could lead folks to thing "(key,value)" instead of "key=value".

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:285: overrides[key] = value
Do you want to validate the key to ensure it is one of the expected values?

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:332: # applications use xml1 for this, so convert
it to ensure that it's valid.
Comment isn't completely right, you are honoring the argument instead of always
doing xml1.

[sdefresne, 2016-06-03 16:11:45]

PTAL

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist.py
File build/mac/tweak_info_plist.py (right):

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:251: parser.add_option('--platform',
choices=('ios', 'mac'), default='mac',
On 2016/06/03 15:49:11, TVL wrote:
> Why not "iOS" and "Mac" so you don't have to do a mapping within the breakpad
> helper?  The version override forces folks to get the capitalization right for
> the overrides.

This value will be passed from BUILD.gn or *.gyp files, then I can just do
"--platform=${target_os}" or "--platform=<(OS)" instead of having a conditional
in GN/gyp. Better to do the conversion in a proper programming language.

Moreover, I've already got push-back in the past for using value different from
"mac"/"ios" to refer to the platform in BUILD.gn files.

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:254: help='Key-value pair to override specific
component of version')
On 2016/06/03 15:49:11, TVL wrote:
> Like --version does, you probably want to give an example. Just saying there
are
> a pair could lead folks to thing "(key,value)" instead of "key=value".

Done.

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:285: overrides[key] = value
On 2016/06/03 15:49:11, TVL wrote:
> Do you want to validate the key to ensure it is one of the expected values?

Done.

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:332: # applications use xml1 for this, so convert
it to ensure that it's valid.
On 2016/06/03 15:49:11, TVL wrote:
> Comment isn't completely right, you are honoring the argument instead of
always
> doing xml1.

Done.

[TVL, 2016-06-03 16:24:27]

lgtm

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist.py
File build/mac/tweak_info_plist.py (right):

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:251: parser.add_option('--platform',
choices=('ios', 'mac'), default='mac',
On 2016/06/03 at 16:11:45, sdefresne wrote:
> On 2016/06/03 15:49:11, TVL wrote:
> > Why not "iOS" and "Mac" so you don't have to do a mapping within the
breakpad
> > helper?  The version override forces folks to get the capitalization right
for
> > the overrides.
> 
> This value will be passed from BUILD.gn or *.gyp files, then I can just do
"--platform=${target_os}" or "--platform=<(OS)" instead of having a conditional
in GN/gyp. Better to do the conversion in a proper programming language.
> 
> Moreover, I've already got push-back in the past for using value different
from "mac"/"ios" to refer to the platform in BUILD.gn files.

That's fair, minor nit then: you might want to do the value remapping in Main()
where it knows about args, instead of making the breakpad function now about the
arg values and remap them.  Then that function can just honor what is passed to
it.

[sdefresne, 2016-06-03 16:29:04]

Thank you for the quick review.

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist.py
File build/mac/tweak_info_plist.py (right):

https://codereview.chromium.org/2037043002/diff/1/build/mac/tweak_info_plist....
build/mac/tweak_info_plist.py:251: parser.add_option('--platform',
choices=('ios', 'mac'), default='mac',
On 2016/06/03 16:24:27, TVL wrote:
> On 2016/06/03 at 16:11:45, sdefresne wrote:
> > On 2016/06/03 15:49:11, TVL wrote:
> > > Why not "iOS" and "Mac" so you don't have to do a mapping within the
> breakpad
> > > helper?  The version override forces folks to get the capitalization right
> for
> > > the overrides.
> > 
> > This value will be passed from BUILD.gn or *.gyp files, then I can just do
> "--platform=${target_os}" or "--platform=<(OS)" instead of having a
conditional
> in GN/gyp. Better to do the conversion in a proper programming language.
> > 
> > Moreover, I've already got push-back in the past for using value different
> from "mac"/"ios" to refer to the platform in BUILD.gn files.
> 
> That's fair, minor nit then: you might want to do the value remapping in
Main()
> where it knows about args, instead of making the breakpad function now about
the
> arg values and remap them.  Then that function can just honor what is passed
to
> it.

Done.

[sdefresne, 2016-06-03 16:30:01]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-06-03 16:30:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
thomasvl@chromium.org
Link to the patchset: https://codereview.chromium.org/2037043002/#ps40001
(title: "Move mapping from "ios/mac" to "iOS/Mac" to Main()")

[commit-bot: I haz the power, 2016-06-03 16:30:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2037043002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2037043002/40001

[commit-bot: I haz the power, 2016-06-03 17:37:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-03 17:40:12]

Description was changed from

==========
[iOS] Add support for iOS to build/mac/tweak_info_plist.py.

In order to allow Chrome on iOS to use build/mac/tweak_info_plist.py,
add options required to control the format of the generated plist file
(--format), the product reported to breakpad (--platform) and a way to
override some portion of the version string (--version-overrides).

This is required to allow Chrome on iOS to migrate from it old fork of
tweak_info_plist shell script and to incorporate tweak_info_plist.py
in the GN build.

BUG=502295
==========

to

==========
[iOS] Add support for iOS to build/mac/tweak_info_plist.py.

In order to allow Chrome on iOS to use build/mac/tweak_info_plist.py,
add options required to control the format of the generated plist file
(--format), the product reported to breakpad (--platform) and a way to
override some portion of the version string (--version-overrides).

This is required to allow Chrome on iOS to migrate from it old fork of
tweak_info_plist shell script and to incorporate tweak_info_plist.py
in the GN build.

BUG=502295

Committed: https://crrev.com/77bf6652e84cfb17925382598e9e50873065b3c2
Cr-Commit-Position: refs/heads/master@{#397745}
==========

[commit-bot: I haz the power, 2016-06-03 17:40:13]

Patchset 3 (id:??) landed as
https://crrev.com/77bf6652e84cfb17925382598e9e50873065b3c2
Cr-Commit-Position: refs/heads/master@{#397745}

[justincohen, 2016-06-05 22:29:54]

lgtm