Fail to build with unbound Info.plist substitutions instead of silently dropping them.

Fail to build with unbound Info.plist substitutions instead of silently dropping them.

- In plist_util.py, fail if any plist substitutions aren't bound to values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

BUG=649854, 650345
Review-Url: https://codereview.chromium.org/2367923002
Cr-Commit-Position: refs/heads/master@{#484348}
Committed: https://chromium.googlesource.com/chromium/src/+/faba7ddfc2b78b1741e578817d7cbbaed0030a93

[Sidney San Martín, 2016-09-23 19:04:12]

sdy@chromium.org changed reviewers:
+ rsesek@chromium.org

[Sidney San Martín, 2016-09-23 19:04:13]

When we talked about this, you wanted to let it fail with the natural KeyError.
I tried, and it didn't provide enough troubleshooting information: you didn't
even know which file had the missing substitution, since gen_plist.py takes a
bunch of input plists and merges them while doing its substitutions.

I think having an explicit error message was worth it — if you still don't think
so, let's chat more.

[Sidney San Martín, 2016-09-23 19:05:56]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- This also let gen_plist.py get shorter, since it doesn't have to bubble up
  None.

- The updated script caught a few dead keys in BuildInfo.plist which are
  added later by tweak_info_plist.py.

BUG=
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- gen_plist.py got shorter, since it no longer has to bubble up None.

- Removed a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

BUG=
==========

[Sidney San Martín, 2016-09-23 19:07:10]

On 2016/09/23 19:04:13, Sidney San Martín wrote:
> When we talked about this, you wanted to let it fail with the natural
KeyError.
> I tried, and it didn't provide enough troubleshooting information: you didn't
> even know which file had the missing substitution, since gen_plist.py takes a
> bunch of input plists and merges them while doing its substitutions.
> 
> I think having an explicit error message was worth it — if you still
don't think
> so, let's chat more.

Meant to include an example. Just deleting the check gave me an error like this:

    KeyError: 'VERSION'

The version in this CL prints an error like this:

    SubstitutionError: No substitution found for 'VERSION' in
../../build/config/mac/BuildInfo.plist

[Robert Sesek, 2016-09-23 20:55:55]

https://codereview.chromium.org/2367923002/diff/1/build/config/mac/gen_plist.py
File build/config/mac/gen_plist.py (left):

https://codereview.chromium.org/2367923002/diff/1/build/config/mac/gen_plist....
build/config/mac/gen_plist.py:168: if not isinstance(plist1, dict) or not
isinstance(plist2, dict):
Was this condition never hit?

https://codereview.chromium.org/2367923002/diff/1/build/config/mac/gen_plist.py
File build/config/mac/gen_plist.py (right):

https://codereview.chromium.org/2367923002/diff/1/build/config/mac/gen_plist....
build/config/mac/gen_plist.py:166: print("SubstitutionError: No substitution
found for '{0}' in {1}"
More idiomatic Chromium style is:

  print >>sys.stderr, 'SubstitutionError: …'

(which should allow you to remove the __future__ import).

[Robert Sesek, 2016-09-23 20:56:53]

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

[Robert Sesek, 2016-09-23 20:56:53]

Also, +sdefresne since he authored the script initially.

[Sidney San Martín, 2016-09-23 22:40:54]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- gen_plist.py got shorter, since it no longer has to bubble up None.

- Removed a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

BUG=
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- gen_plist.py got shorter, since it no longer has to bubble up None.

- Removed a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

BUG=649854
==========

[sdefresne, 2016-09-26 12:53:14]

On 2016/09/23 20:56:53, Robert Sesek wrote:
> Also, +sdefresne since he authored the script initially.

I think this may break iOS downstream as we have multiple Info.plist with
unbound substitutions (and the bound variable depends on some build settings).

[Sidney San Martín, 2016-09-26 18:35:58]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- gen_plist.py got shorter, since it no longer has to bubble up None.

- Removed a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

BUG=649854
==========

to

==========
By default, fail to build with unbound Info.plist substitutions instead of
silently dropping them.

- Removed a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In gen_plist.py, only throw away values with unbound substitutions if
  --skip-unbound-variables is passed. This flag should be temporary.

BUG=649854,650345
==========

[Sidney San Martín, 2016-09-26 18:36:31]

Description was changed from

==========
By default, fail to build with unbound Info.plist substitutions instead of
silently dropping them.

- Removed a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In gen_plist.py, only throw away values with unbound substitutions if
  --skip-unbound-variables is passed. This flag should be temporary.

BUG=649854,650345
==========

to

==========
By default, fail to build with unbound Info.plist substitutions instead of
silently dropping them.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In gen_plist.py, only throw away values with unbound substitutions if
  --skip-unbound-variables is passed. This flag should be temporary.

BUG=649854,650345
==========

[Sidney San Martín, 2016-09-26 18:38:25]

Let me know how this looks. It brings back support, behind a flag, for dropping
unbound values.

https://codereview.chromium.org/2367923002/diff/1/build/config/mac/gen_plist.py
File build/config/mac/gen_plist.py (left):

https://codereview.chromium.org/2367923002/diff/1/build/config/mac/gen_plist....
build/config/mac/gen_plist.py:168: if not isinstance(plist1, dict) or not
isinstance(plist2, dict):
On 2016/09/23 20:55:55, Robert Sesek wrote:
> Was this condition never hit?

It could get hit in the old code (:180 can call MergePList with None), but not
now, as far as I can tell.

https://codereview.chromium.org/2367923002/diff/1/build/config/mac/gen_plist.py
File build/config/mac/gen_plist.py (right):

https://codereview.chromium.org/2367923002/diff/1/build/config/mac/gen_plist....
build/config/mac/gen_plist.py:166: print("SubstitutionError: No substitution
found for '{0}' in {1}"
On 2016/09/23 20:55:55, Robert Sesek wrote:
> More idiomatic Chromium style is:
> 
>   print >>sys.stderr, 'SubstitutionError: …'
> 
> (which should allow you to remove the __future__ import).

Done.

[sdefresne, 2016-09-27 08:26:00]

Can you add --skip-unbound-variables to the invocation of gen_plist.py in
build/config/mac/base_rules.gni if is_ios is true? Otherwise we risk breaking
the tree when this change rolls downstream.

https://codereview.chromium.org/2367923002/diff/40001/build/config/mac/gen_pl...
File build/config/mac/gen_plist.py (right):

https://codereview.chromium.org/2367923002/diff/40001/build/config/mac/gen_pl...
build/config/mac/gen_plist.py:92: return InterpolateString(value, substitutions)
I think call may lead to uncaught SubstitutionError on iOS even if
--skip-unbound-variables is true. Should you use map_fn here too? Or add an
explicit catch (but then you'll have to pass the boolean flag here too.

[Sidney San Martín, 2016-09-27 20:15:21]

On 2016/09/27 08:26:00, sdefresne wrote:
> Can you add --skip-unbound-variables to the invocation of gen_plist.py in
> build/config/mac/base_rules.gni if is_ios is true? Otherwise we risk breaking
> the tree when this change rolls downstream.

Done.

https://codereview.chromium.org/2367923002/diff/40001/build/config/mac/gen_pl...
File build/config/mac/gen_plist.py (right):

https://codereview.chromium.org/2367923002/diff/40001/build/config/mac/gen_pl...
build/config/mac/gen_plist.py:92: return InterpolateString(value, substitutions)
On 2016/09/27 08:26:00, sdefresne wrote:
> I think call may lead to uncaught SubstitutionError on iOS even if
> --skip-unbound-variables is true. Should you use map_fn here too? Or add an
> explicit catch (but then you'll have to pass the boolean flag here too.

I think the current behavior might be correct, but let me know if my reasoning
doesn't make sense —

As it is, Interpolate never returns None. map_fn lets the caller take control of
collections (e.g. swallowing values when a SubstitutionError is raised). If
map_fn were wrapped around InterpolateString, what should Interpolate return
when it swallows the value? In practice, Interpolate is only ever called from
outside with lists/dicts. If someone *did* call it with a string with bad
substitutions, getting a SubstitutionError makes as much or more sense than it
just returning None, since they both have to be handled specially.

[Sidney San Martín, 2016-10-07 16:23:40]

On 2016/09/27 20:15:21, Sidney San Martín wrote:
> On 2016/09/27 08:26:00, sdefresne wrote:
> > Can you add --skip-unbound-variables to the invocation of gen_plist.py in
> > build/config/mac/base_rules.gni if is_ios is true? Otherwise we risk
breaking
> > the tree when this change rolls downstream.
> 
> Done.
> 
>
https://codereview.chromium.org/2367923002/diff/40001/build/config/mac/gen_pl...
> File build/config/mac/gen_plist.py (right):
> 
>
https://codereview.chromium.org/2367923002/diff/40001/build/config/mac/gen_pl...
> build/config/mac/gen_plist.py:92: return InterpolateString(value,
substitutions)
> On 2016/09/27 08:26:00, sdefresne wrote:
> > I think call may lead to uncaught SubstitutionError on iOS even if
> > --skip-unbound-variables is true. Should you use map_fn here too? Or add an
> > explicit catch (but then you'll have to pass the boolean flag here too.
> 
> I think the current behavior might be correct, but let me know if my reasoning
> doesn't make sense —
> 
> As it is, Interpolate never returns None. map_fn lets the caller take control
of
> collections (e.g. swallowing values when a SubstitutionError is raised). If
> map_fn were wrapped around InterpolateString, what should Interpolate return
> when it swallows the value? In practice, Interpolate is only ever called from
> outside with lists/dicts. If someone *did* call it with a string with bad
> substitutions, getting a SubstitutionError makes as much or more sense than it
> just returning None, since they both have to be handled specially.

Hey sdefresne@, just a friendly ping. Any thoughts?

[sdefresne, 2016-12-21 13:09:07]

On 2016/10/07 16:23:40, Sidney San Martín wrote:
> On 2016/09/27 20:15:21, Sidney San Martín wrote:
> > On 2016/09/27 08:26:00, sdefresne wrote:
> > > Can you add --skip-unbound-variables to the invocation of gen_plist.py in
> > > build/config/mac/base_rules.gni if is_ios is true? Otherwise we risk
> breaking
> > > the tree when this change rolls downstream.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2367923002/diff/40001/build/config/mac/gen_pl...
> > File build/config/mac/gen_plist.py (right):
> > 
> >
>
https://codereview.chromium.org/2367923002/diff/40001/build/config/mac/gen_pl...
> > build/config/mac/gen_plist.py:92: return InterpolateString(value,
> substitutions)
> > On 2016/09/27 08:26:00, sdefresne wrote:
> > > I think call may lead to uncaught SubstitutionError on iOS even if
> > > --skip-unbound-variables is true. Should you use map_fn here too? Or add
an
> > > explicit catch (but then you'll have to pass the boolean flag here too.
> > 
> > I think the current behavior might be correct, but let me know if my
reasoning
> > doesn't make sense —
> > 
> > As it is, Interpolate never returns None. map_fn lets the caller take
control
> of
> > collections (e.g. swallowing values when a SubstitutionError is raised). If
> > map_fn were wrapped around InterpolateString, what should Interpolate return
> > when it swallows the value? In practice, Interpolate is only ever called
from
> > outside with lists/dicts. If someone *did* call it with a string with bad
> > substitutions, getting a SubstitutionError makes as much or more sense than
it
> > just returning None, since they both have to be handled specially.
> 
> Hey sdefresne@, just a friendly ping. Any thoughts?

Most of the downstream iOS code has been upstreamed. Can you try to see if
rebasing this CL and sending it to CQ works. I tried to fix the iOS .plist
files, so I expect it to pass. If not, then we know what need to be fixed first.

[Sidney San Martín, 2017-01-05 02:18:59]

On 2016/12/21 13:09:07, sdefresne wrote:
> rebasing this CL and sending it to CQ works. I tried to fix the iOS .plist
> files, so I expect it to pass. If not, then we know what need to be fixed
first.

On it, thanks for the reminder!

[Sidney San Martín, 2017-01-05 02:19:26]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 02:19:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 02:27:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 02:27:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Sidney San Martín, 2017-01-05 03:04:00]

On 2016/12/21 13:09:07, sdefresne wrote:
> Most of the downstream iOS code has been upstreamed. Can you try to see if
> rebasing this CL and sending it to CQ works. I tried to fix the iOS .plist
> files, so I expect it to pass. If not, then we know what need to be fixed
first.

Looks like we have our first failure: SSOAUTH_URL_SCHEME. Want to take a look at
it?

[Sidney San Martín, 2017-01-07 00:07:25]

Description was changed from

==========
By default, fail to build with unbound Info.plist substitutions instead of
silently dropping them.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In gen_plist.py, only throw away values with unbound substitutions if
  --skip-unbound-variables is passed. This flag should be temporary.

BUG=649854,650345
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In gen_plist.py, return nonzero if any plist substitutions don't have values.

BUG=649854,650345
==========

[Sidney San Martín, 2017-01-07 00:07:39]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In gen_plist.py, return nonzero if any plist substitutions don't have values.

BUG=649854,650345
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In gen_plist.py, return nonzero if any plist substitutions aren't bound to
values.

BUG=649854,650345
==========

[Sidney San Martín, 2017-01-07 00:07:50]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In gen_plist.py, return nonzero if any plist substitutions aren't bound to
values.

BUG=649854,650345
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In plist_util.py, return nonzero if any plist substitutions aren't bound to
values.

BUG=649854,650345
==========

[Sidney San Martín, 2017-01-07 00:08:03]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

- In plist_util.py, return nonzero if any plist substitutions aren't bound to
values.

BUG=649854,650345
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, return nonzero if any plist substitutions aren't bound to
values.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

BUG=649854,650345
==========

[Sidney San Martín, 2017-01-07 00:08:33]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, return nonzero if any plist substitutions aren't bound to
values.

- Remove a few keys in BuildInfo.plist which had no substitutions because
  they're added later by tweak_info_plist.py.

BUG=649854,650345
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, return nonzero if any plist substitutions aren't bound to
values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

BUG=649854,650345
==========

[Sidney San Martín, 2017-02-01 23:17:54]

On 2017/01/05 03:04:00, sdy wrote:
> On 2016/12/21 13:09:07, sdefresne wrote:
> > Most of the downstream iOS code has been upstreamed. Can you try to see if
> > rebasing this CL and sending it to CQ works. I tried to fix the iOS .plist
> > files, so I expect it to pass. If not, then we know what need to be fixed
> first.
> 
> Looks like we have our first failure: SSOAUTH_URL_SCHEME. Want to take a look
at
> it?

Friendly ping. I'm happy to look at the errors when I have time, but it might be
better for someone who already knows the iOS code to take a look.

[sdefresne, 2017-06-01 13:21:37]

On 2017/02/01 23:17:54, Sidney San Martín wrote:
> On 2017/01/05 03:04:00, sdy wrote:
> > On 2016/12/21 13:09:07, sdefresne wrote:
> > > Most of the downstream iOS code has been upstreamed. Can you try to see if
> > > rebasing this CL and sending it to CQ works. I tried to fix the iOS .plist
> > > files, so I expect it to pass. If not, then we know what need to be fixed
> > first.
> > 
> > Looks like we have our first failure: SSOAUTH_URL_SCHEME. Want to take a
look
> at
> > it?
> 
> Friendly ping. I'm happy to look at the errors when I have time, but it might
be
> better for someone who already knows the iOS code to take a look.

Sorry for the late reply. I finally got to fix the compilation of iOS with the
patch applied. This lgtm once https://chromium-review.googlesource.com/c/521106/
lands.

You'll need to rebase though.

[sdefresne, 2017-06-15 13:28:12]

sdy: ping?

[Sidney San Martín, 2017-06-20 22:28:00]

On 2017/06/15 13:28:12, sdefresne wrote:
> sdy: ping?

Thanks — rebasing now!

[Sidney San Martín, 2017-06-20 22:28:11]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 22:28:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 22:45:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 22:45:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Sidney San Martín, 2017-06-20 23:01:20]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 23:01:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 23:14:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 23:14:53]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sidney San Martín, 2017-06-21 16:09:18]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 16:09:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sidney San Martín, 2017-06-21 16:11:07]

sdy@chromium.org changed reviewers:
+ thakis@chromium.org
- rsesek@chromium.org

[Sidney San Martín, 2017-06-21 16:11:08]

PTAL? I'm doing another dry run now, I think the last one failed for reasons
unrelated to this change :).

[commit-bot: I haz the power, 2017-06-21 17:41:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 17:41:02]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-06-26 11:44:40]

lgtm

thakis: ping for OWNERS

[Nico, 2017-06-26 14:38:33]

On 2017/06/26 11:44:40, sdefresne wrote:
> lgtm
> 
> thakis: ping for OWNERS

Here's how this thread looks like in my inbox: http://imgur.com/a/6FoxS

At no point before this ping was I aware that I was supposed to review anything
here.

What files am I supposed to look at?

[sdefresne, 2017-06-26 14:44:33]

On 2017/06/26 14:38:33, Nico (vacation Jun 30-Jul 11) wrote:
> On 2017/06/26 11:44:40, sdefresne wrote:
> > lgtm
> > 
> > thakis: ping for OWNERS
> 
> Here's how this thread looks like in my inbox: http://imgur.com/a/6FoxS
> 
> At no point before this ping was I aware that I was supposed to review
anything
> here.
> 
> What files am I supposed to look at?

It looks like OP switched from rsesek@ to thakis@ 4 days ago on this month old
CL, and I did not check you were not originally on the thread. Sorry about that.

I've already reviewed the whole CL but I'm not an OWNERS of ui/base, so I think
we need your approval for those files.

Thank you.

[Nico, 2017-06-26 14:47:18]

lgtm

https://codereview.chromium.org/2367923002/diff/140001/build/config/mac/Build...
File build/config/mac/BuildInfo.plist (left):

https://codereview.chromium.org/2367923002/diff/140001/build/config/mac/Build...
build/config/mac/BuildInfo.plist:24: <string>${COMMIT_HASH}</string>
From the Cl description, this removal here is behavior-preserving, yes?

[Sidney San Martín, 2017-06-26 17:40:03]

Thanks — sorry for the confusing message when I added you, Nico. Seeing the
screenshot helps.

https://codereview.chromium.org/2367923002/diff/140001/build/config/mac/Build...
File build/config/mac/BuildInfo.plist (left):

https://codereview.chromium.org/2367923002/diff/140001/build/config/mac/Build...
build/config/mac/BuildInfo.plist:24: <string>${COMMIT_HASH}</string>
On 2017/06/26 14:47:18, Nico (vacation Jun 30-Jul 11) wrote:
> From the Cl description, this removal here is behavior-preserving, yes?

Yes, as far as I can tell all of these keys are either in the plists which pull
in this one, or added by tweak_info_plist.py.

[Sidney San Martín, 2017-06-26 17:40:13]

The CQ bit was checked by sdy@chromium.org

[commit-bot: I haz the power, 2017-06-26 17:40:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 17:58:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 17:58:42]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Sidney San Martín, 2017-06-27 02:40:57]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 02:41:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sdefresne, 2017-06-27 08:26:25]

https://codereview.chromium.org/2367923002/diff/180001/extensions/shell/app/a...
File extensions/shell/app/app-Info.plist (right):

https://codereview.chromium.org/2367923002/diff/180001/extensions/shell/app/a...
extensions/shell/app/app-Info.plist:28: <string>10.9.0</string>
I don't like hardcoding the value here. It is available in GN via the
"mac_deployment_target" variable from build/config/mac/mac_sdk.gni, can't we
instead add the following to the template that does variable substitution on
.plist files:

  MACOSX_DEPLOYMENT_TARGET=$mac_deployment_target

This will avoid requiring to grep all files for "10.9" when this variable is
updated.

[Sidney San Martín, 2017-06-27 18:54:11]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, return nonzero if any plist substitutions aren't bound to
values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

BUG=649854,650345
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, fail if any plist substitutions aren't bound to values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

- Use $mac_deployment_target in Info.plist files now that our deployment target
  is back in sync with our minimum supporter macOS version (crbug/580152).

BUG=649854,650345
==========

[Sidney San Martín, 2017-06-27 18:58:26]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, fail if any plist substitutions aren't bound to values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

- Use $mac_deployment_target in Info.plist files now that our deployment target
  is back in sync with our minimum supporter macOS version (crbug/580152).

BUG=649854,650345
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, fail if any plist substitutions aren't bound to values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

- Use $mac_deployment_target in Info.plist files now that our deployment target
  is back in sync with our minimum supporter macOS version (crbug/580152). Also
  move the ".0" from each Info.plist to the deployment target variable itself:
  it isn't guaranteed to be zero.

BUG=649854,650345
==========

[Sidney San Martín, 2017-06-27 19:45:52]

sdefresne@, mind taking another quick look? I want to make sure it's still
reasonable for this to be one CL.

https://codereview.chromium.org/2367923002/diff/180001/extensions/shell/app/a...
File extensions/shell/app/app-Info.plist (right):

https://codereview.chromium.org/2367923002/diff/180001/extensions/shell/app/a...
extensions/shell/app/app-Info.plist:28: <string>10.9.0</string>
On 2017/06/27 08:26:25, sdefresne wrote:
> I don't like hardcoding the value here. It is available in GN via the
> "mac_deployment_target" variable from build/config/mac/mac_sdk.gni, can't we
> instead add the following to the template that does variable substitution on
> .plist files:
> 
>   MACOSX_DEPLOYMENT_TARGET=$mac_deployment_target
> 
> This will avoid requiring to grep all files for "10.9" when this variable is
> updated.

Done. I just remembered why it was like this in the original CL: at that point,
our minimum supported macOS version was 10.9, but our build deployment target
was 10.7, and I was discouraged from adding a separate build variable that would
just be used in Info.plists. Now the target is back in sync :) (crbug/580152).

[Sidney San Martín, 2017-06-27 19:47:42]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 19:47:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 19:57:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 19:57:19]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Sidney San Martín, 2017-06-27 20:19:07]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 20:19:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 22:01:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 22:01:44]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-06-28 08:49:49]

Still lgtm, but the CL is starting to get large.

Can you split the $mac_deployment_target/$ios_deployment_target substitutions to
a separate CL? That will probably be easier than chasing all the required OWNERS
approval on that CL.

[Sidney San Martín, 2017-07-05 18:34:40]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 18:35:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-05 18:59:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-05 18:59:59]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Sidney San Martín, 2017-07-05 19:33:56]

I'm going to land this once crbug/739469 un-breaks the CQ.

[Sidney San Martín, 2017-07-05 19:54:51]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, fail if any plist substitutions aren't bound to values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

- Use $mac_deployment_target in Info.plist files now that our deployment target
  is back in sync with our minimum supporter macOS version (crbug/580152). Also
  move the ".0" from each Info.plist to the deployment target variable itself:
  it isn't guaranteed to be zero.

BUG=649854,650345
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, fail if any plist substitutions aren't bound to values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

BUG=649854,650345
==========

[Sidney San Martín, 2017-07-05 19:55:20]

The CQ bit was checked by sdy@chromium.org

[Sidney San Martín, 2017-07-05 19:55:21]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2367923002/#ps240001
(title: "Rebase to pick up crrev/c/555912")

[commit-bot: I haz the power, 2017-07-05 19:55:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-05 21:04:25]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1499284520582240,
"parent_rev": "b1d2ec19fc647fbde62797e1dcdd4d43db68fcc0", "commit_rev":
"faba7ddfc2b78b1741e578817d7cbbaed0030a93"}

[commit-bot: I haz the power, 2017-07-05 21:04:41]

Description was changed from

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, fail if any plist substitutions aren't bound to values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

BUG=649854,650345
==========

to

==========
Fail to build with unbound Info.plist substitutions instead of silently dropping
them.

- In plist_util.py, fail if any plist substitutions aren't bound to values.

- Remove a few keys in BuildInfo.plist which had no substitutions. They're
  added later by tweak_info_plist.py.

BUG=649854,650345

Review-Url: https://codereview.chromium.org/2367923002
Cr-Commit-Position: refs/heads/master@{#484348}
Committed:
https://chromium.googlesource.com/chromium/src/+/faba7ddfc2b78b1741e578817d7c...
==========

[commit-bot: I haz the power, 2017-07-05 21:04:44]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/faba7ddfc2b78b1741e578817d7c...