Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2359953003:
Fix up the Mac installer’s Info.plist, build with branding info (Closed)
|
Created:
4 years, 3 months ago by Sidney San Martín Modified:
4 years, 3 months ago CC:
chromium-reviews, grt+watch_chromium.org, pennymac+watch_chromium.org, oshima+watch_chromium.org, wfh+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix up the Mac installer’s Info.plist, build with branding info
BUG=
Committed: https://crrev.com/a00c1a94b41da9c22220ebbbb6b5f903890179b2
Cr-Commit-Position: refs/heads/master@{#420450}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Derive the installer's bundle ID from the main one, drop the tweak_info_plist step, add missing substitutions #
Total comments: 3
Patch Set 3 : Remove an unused gn dependency, add .installer to the bundle ID in the Info.plist instead of BUILD.gn #
Messages
Total messages: 23 (8 generated)
Description was changed from ========== Use BRANDING to build the Mac installer, fix up its Info.plist BUG= ========== to ========== Fix up the Mac installer’s Info.plist, build with branding info BUG= ==========
sdy@chromium.org changed reviewers: + rsesek@chromium.org
How does this look?
https://codereview.chromium.org/2359953003/diff/1/chrome/app/theme/chromium/B... File chrome/app/theme/chromium/BRANDING (right): https://codereview.chromium.org/2359953003/diff/1/chrome/app/theme/chromium/B... chrome/app/theme/chromium/BRANDING:9: MAC_INSTALLER_BUNDLE_ID=org.chromium.Chromium.installer I could just compose the installer's bundle ID as "$mac_bundle_id" + ".installer", but not sure if that's a good strategy. https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... chrome/installer/mac/app/BUILD.gn:41: } Make sure these flags look OK — the only weird bit I noticed in the output is that the installer ends up with a Chrome version number like "55.0.2868.0", and I'm not sure if that's appropriate.
https://codereview.chromium.org/2359953003/diff/1/build/util/branding.gni File build/util/branding.gni (right): https://codereview.chromium.org/2359953003/diff/1/build/util/branding.gni#new... build/util/branding.gni:18: "full_name = \"@PRODUCT_FULLNAME@\" " + Did gn format not try and change this? https://codereview.chromium.org/2359953003/diff/1/chrome/app/theme/chromium/B... File chrome/app/theme/chromium/BRANDING (right): https://codereview.chromium.org/2359953003/diff/1/chrome/app/theme/chromium/B... chrome/app/theme/chromium/BRANDING:9: MAC_INSTALLER_BUNDLE_ID=org.chromium.Chromium.installer On 2016/09/21 21:18:39, Sidney San Martín wrote: > I could just compose the installer's bundle ID as "$mac_bundle_id" + > ".installer", but not sure if that's a good strategy. I think it's fine to use the base MAC_BUNDLE_ID (it's what all our Info.plists currently do). If you want to use a new BRANDING key, you'll need to do it to the internal branding repo as well and roll its deps forward. (E.g.: https://chromereviews.googleplex.com/415397013/ and https://chromereviews.googleplex.com/430687013/) https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... chrome/installer/mac/app/BUILD.gn:41: } On 2016/09/21 21:18:39, Sidney San Martín wrote: > Make sure these flags look OK — the only weird bit I noticed in the output is > that the installer ends up with a Chrome version number like "55.0.2868.0", and > I'm not sure if that's appropriate. Yeah, I'd maybe skip the tweak step for the time being. I'm not sure how we'll want to version this. https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... chrome/installer/mac/app/BUILD.gn:46: extra_substitutions = [ "MACOSX_DEPLOYMENT_TARGET=10.9" ] You still need to add PRODUCT_INSTALLER_FULLNAME and MAC_INSTALLER_BUNDLE_ID here.
https://codereview.chromium.org/2359953003/diff/1/build/util/branding.gni File build/util/branding.gni (right): https://codereview.chromium.org/2359953003/diff/1/build/util/branding.gni#new... build/util/branding.gni:18: "full_name = \"@PRODUCT_FULLNAME@\" " + On 2016/09/21 21:41:21, Robert Sesek wrote: > Did gn format not try and change this? gn format *did* this. https://codereview.chromium.org/2359953003/diff/1/chrome/app/theme/chromium/B... File chrome/app/theme/chromium/BRANDING (right): https://codereview.chromium.org/2359953003/diff/1/chrome/app/theme/chromium/B... chrome/app/theme/chromium/BRANDING:9: MAC_INSTALLER_BUNDLE_ID=org.chromium.Chromium.installer On 2016/09/21 21:41:21, Robert Sesek wrote: > On 2016/09/21 21:18:39, Sidney San Martín wrote: > > I could just compose the installer's bundle ID as "$mac_bundle_id" + > > ".installer", but not sure if that's a good strategy. > > I think it's fine to use the base MAC_BUNDLE_ID (it's what all our Info.plists > currently do). If you want to use a new BRANDING key, you'll need to do it to > the internal branding repo as well and roll its deps forward. > > (E.g.: https://chromereviews.googleplex.com/415397013/ and > https://chromereviews.googleplex.com/430687013/) Done. https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... chrome/installer/mac/app/BUILD.gn:41: } On 2016/09/21 21:41:21, Robert Sesek wrote: > On 2016/09/21 21:18:39, Sidney San Martín wrote: > > Make sure these flags look OK — the only weird bit I noticed in the > output is > > that the installer ends up with a Chrome version number like "55.0.2868.0", > and > > I'm not sure if that's appropriate. > > Yeah, I'd maybe skip the tweak step for the time being. I'm not sure how we'll > want to version this. Done. https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... chrome/installer/mac/app/BUILD.gn:46: extra_substitutions = [ "MACOSX_DEPLOYMENT_TARGET=10.9" ] On 2016/09/21 21:41:21, Robert Sesek wrote: > You still need to add PRODUCT_INSTALLER_FULLNAME and MAC_INSTALLER_BUNDLE_ID > here. Oof, thanks. I skimmed the output Info.plist for empty or un-substituted fields and didn't see any, but I wasn't expecting the fields without values to be omitted completely. Do you think it would make sense to change mac_info_plist to fail on unbound substitutions?
https://codereview.chromium.org/2359953003/diff/1/build/util/branding.gni File build/util/branding.gni (right): https://codereview.chromium.org/2359953003/diff/1/build/util/branding.gni#new... build/util/branding.gni:18: "full_name = \"@PRODUCT_FULLNAME@\" " + On 2016/09/21 23:19:20, Sidney San Martín wrote: > On 2016/09/21 21:41:21, Robert Sesek wrote: > > Did gn format not try and change this? > > gn format *did* this. Oh, cool. I was trying to convince it that this was a superior formatting, but it would not agree. https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... chrome/installer/mac/app/BUILD.gn:46: extra_substitutions = [ "MACOSX_DEPLOYMENT_TARGET=10.9" ] On 2016/09/21 23:19:20, Sidney San Martín wrote: > On 2016/09/21 21:41:21, Robert Sesek wrote: > > You still need to add PRODUCT_INSTALLER_FULLNAME and MAC_INSTALLER_BUNDLE_ID > > here. > > Oof, thanks. I skimmed the output Info.plist for empty or un-substituted fields > and didn't see any, but I wasn't expecting the fields without values to be > omitted completely. > > Do you think it would make sense to change mac_info_plist to fail on unbound > substitutions? When I looked at your binary, I considered the same thing. Yes, I think gen_plist.py should error on undefined variables, rather than silently removing the key/value pair. https://codereview.chromium.org/2359953003/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2359953003/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:6: import("//build/mac/tweak_info_plist.gni") Unused. https://codereview.chromium.org/2359953003/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:39: "MAC_INSTALLER_BUNDLE_ID=$chrome_mac_bundle_id.installer", For consistency with chrome/BULD.gn, use CHROMIUM_BUNDLE_ID=$chrome_mac_bundle_id and then put the ".installer" literal in the actual Info.plist file (see also framework-Info.plist and helper-Info.plist for examples).
https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2359953003/diff/1/chrome/installer/mac/app/BU... chrome/installer/mac/app/BUILD.gn:46: extra_substitutions = [ "MACOSX_DEPLOYMENT_TARGET=10.9" ] On 2016/09/22 14:46:56, Robert Sesek wrote: > On 2016/09/21 23:19:20, Sidney San Martín wrote: > > On 2016/09/21 21:41:21, Robert Sesek wrote: > > > You still need to add PRODUCT_INSTALLER_FULLNAME and MAC_INSTALLER_BUNDLE_ID > > > here. > > > > Oof, thanks. I skimmed the output Info.plist for empty or un-substituted > fields > > and didn't see any, but I wasn't expecting the fields without values to be > > omitted completely. > > > > Do you think it would make sense to change mac_info_plist to fail on unbound > > substitutions? > > When I looked at your binary, I considered the same thing. Yes, I think > gen_plist.py should error on undefined variables, rather than silently removing > the key/value pair. Will do. https://codereview.chromium.org/2359953003/diff/20001/chrome/installer/mac/ap... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2359953003/diff/20001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:39: "MAC_INSTALLER_BUNDLE_ID=$chrome_mac_bundle_id.installer", On 2016/09/22 14:46:56, Robert Sesek wrote: > For consistency with chrome/BULD.gn, use > CHROMIUM_BUNDLE_ID=$chrome_mac_bundle_id and then put the ".installer" literal > in the actual Info.plist file (see also framework-Info.plist and > helper-Info.plist for examples). Done.
LGTM
The CQ bit was checked by sdy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sdy@chromium.org changed reviewers: + dpranke@chromium.org, mark@chromium.org
+ mark@ and dpranke@ for OWNER coverage.
lgtm
LGTM
The CQ bit was checked by sdy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix up the Mac installer’s Info.plist, build with branding info BUG= ========== to ========== Fix up the Mac installer’s Info.plist, build with branding info BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix up the Mac installer’s Info.plist, build with branding info BUG= ========== to ========== Fix up the Mac installer’s Info.plist, build with branding info BUG= Committed: https://crrev.com/a00c1a94b41da9c22220ebbbb6b5f903890179b2 Cr-Commit-Position: refs/heads/master@{#420450} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a00c1a94b41da9c22220ebbbb6b5f903890179b2 Cr-Commit-Position: refs/heads/master@{#420450} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
