Mac PKG: Craft the distribution XML file by hand.

The automatic creation of the distribution XML file yielded
a file that when installed was missing user-visible strings.

Bug: 1022413
Change-Id: If2ad9bfb1305d2dcd58b6a04d25aa0130216b25b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1906949
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Auto-Submit: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714123}

chrome/installer/mac/signing/pipeline.py

@@ -118,31 +118,63 @@ def _staple_chrome(paths, dist_config):
         notarize.staple(os.path.join(paths.work, part_path))
 
 
-def _productbuild_requirements_path(paths, dist_config):
-    """Creates a requirements file for use by `productbuild`. This specifies
+def _productbuild_distribution_path(paths, dist_config, component_pkg_path):
+    """Creates a distribution XML file for use by `productbuild`. This specifies
     that an x64 machine is required, and copies the OS requirement from the copy
     of Chrome being packaged.
 
     Args:
         paths: A |model.Paths| object.
         dist_config: The |config.CodeSignConfig| object.
+        component_pkg_path: The path to the existing component .pkg file.
 
     Returns:
-        The path to the requirements file.
+        The path to the distribution file.
     """
-    requirements_path = os.path.join(paths.work,
-                                     '{}.req'.format(dist_config.app_product))
+    distribution_path = os.path.join(paths.work,
+                                     '{}.dist'.format(dist_config.app_product))
 
     app_plist_path = os.path.join(paths.work, dist_config.app_dir, 'Contents',
                                   'Info.plist')
     with commands.PlistContext(app_plist_path) as app_plist:
-        with commands.PlistContext(
-                requirements_path, rewrite=True,
-                create_new=True) as requirements:
-            requirements['os'] = [app_plist['LSMinimumSystemVersion']]
-            requirements['arch'] = ['x86_64']
-
-    return requirements_path
+        distribution_xml = """<?xml version="1.0" encoding="utf-8"?>
+<installer-gui-script minSpecVersion="2">
+
+    <!-- Top-level info about the distribution. -->
+    <title>{app_product}</title>
+    <options customize="never" require-scripts="false" hostArchitectures="x86_64"/>
+    <volume-check>
+        <allowed-os-versions>
+            <os-version min="{minimum_system}"/>
+        </allowed-os-versions>
+    </volume-check>
+
+    <!-- The hierarchy of installation choices. -->
+    <choices-outline>
+        <line choice="default">
+            <line choice="{bundle_id}"/>
+        </line>
+    </choices-outline>
+
+    <!-- The individual choices. -->
+    <choice id="default"/>
+    <choice id="{bundle_id}" visible="false" title="{app_product}">
+        <pkg-ref id="{bundle_id}"/>
+    </choice>
+
+    <!-- The lone component package. -->
+    <pkg-ref id="{bundle_id}" version="{version}" onConclusion="none">{component_pkg_filename}</pkg-ref>
+
+</installer-gui-script>""".format(
+            app_product=dist_config.app_product,
+            bundle_id=dist_config.base_bundle_id,
+            minimum_system=app_plist['LSMinimumSystemVersion'],
+            component_pkg_filename=os.path.basename(component_pkg_path),
+            version=dist_config.version)
+
+        commands.write_file(distribution_path, distribution_xml)
+
+    return distribution_path
 
 
 def _package_and_sign_pkg(paths, dist_config):
@@ -182,26 +214,8 @@ def _package_and_sign_pkg(paths, dist_config):
 
     ## The product archive.
 
-    # There are two steps here. The first is to create the "distribution file"
-    # which describes the product archive. `productbuild` has a mode to generate
-    # such a file, with the optional input of a "requirements file" that
-    # describes the desired installation requirements of the product archive.
-    # Use this mode to generate a distribution file. Note that if, in the
-    # future, it's desired that the product archive have UI customization, then
-    # this distribution file will need to be hand-crafted. With any luck, this
-    # auto-generated distribution file continue to suffice.
-
-    requirements_path = _productbuild_requirements_path(paths, dist_config)
-
-    distribution_path = os.path.join(paths.work,
-                                     '{}.dist'.format(dist_config.app_product))
-    commands.run_command([
-        'productbuild', '--synthesize', '--product', requirements_path,
-        '--package', component_pkg_path, distribution_path
-    ])
-
-    # The second step is to actually create the product archive using
-    # `productbuild`.
+    distribution_path = _productbuild_distribution_path(paths, dist_config,
+                                                        component_pkg_path)
 
     product_pkg_path = os.path.join(
         paths.output, '{}.pkg'.format(dist_config.packaging_basename))

chrome/installer/mac/signing/pipeline_test.py

@@ -4,6 +4,7 @@
 
 import os.path
 import unittest
+from xml.etree import ElementTree
 
 from . import model, pipeline, test_common, test_config
 
@@ -18,8 +19,12 @@ def _get_work_dir(*args, **kwargs):
 _get_work_dir.count = 0
 
 
-def _productbuild_requirements_path(p, d):
-    return '$W/App Product.req'
+def _productbuild_distribution_path(p, d, c):
+    return '$W/App Product.dist'
+
+
+def _read_plist(p):
+    return {'LSMinimumSystemVersion': '10.19.7'}
 
 
 def _get_adjacent_item(l, o):
@@ -32,16 +37,15 @@ def _get_adjacent_item(l, o):
 
 @mock.patch.multiple(
     'signing.commands', **{
-        m: mock.DEFAULT for m in ('move_file', 'copy_files',
-                                  'copy_dir_overwrite_and_count_changes',
-                                  'run_command', 'make_dir', 'shutil')
+        m: mock.DEFAULT
+        for m in ('move_file', 'copy_files',
+                  'copy_dir_overwrite_and_count_changes', 'run_command',
+                  'make_dir', 'shutil', 'write_file')
     })
 @mock.patch.multiple(
     'signing.signing',
     **{m: mock.DEFAULT for m in ('sign_part', 'sign_chrome', 'verify_part')})
 @mock.patch('signing.commands.tempfile.mkdtemp', _get_work_dir)
-@mock.patch('signing.pipeline._productbuild_requirements_path',
-            _productbuild_requirements_path)
 class TestPipelineHelpers(unittest.TestCase):
 
     def setUp(self):
@@ -246,6 +250,38 @@ class TestPipelineHelpers(unittest.TestCase):
             mock.call('$W/App Product Canary.app')
         ])
 
+    @mock.patch('signing.commands.plistlib.readPlist', _read_plist)
+    def test_productbuild_distribution_path(self, **kwargs):
+        manager = mock.Mock()
+        for attr in kwargs:
+            manager.attach_mock(kwargs[attr], attr)
+
+        dist = model.Distribution()
+        dist_config = dist.to_config(test_config.TestConfig())
+        paths = self.paths.replace_work('$W')
+
+        component_pkg_path = os.path.join(
+            paths.work, '{}.pkg'.format(dist_config.app_product))
+
+        self.assertEqual(
+            '$W/App Product.dist',
+            pipeline._productbuild_distribution_path(paths, dist_config,
+                                                     component_pkg_path))
+
+        manager.assert_has_calls([
+            mock.call.write_file('$W/App Product.dist', mock.ANY),
+        ])
+
+        xml_string = manager.mock_calls[0][1][1]
+
+        xml_root = ElementTree.fromstring(xml_string)
+        volume_check = xml_root.find('volume-check')
+        allowed_os_versions = volume_check.find('allowed-os-versions')
+        os_versions = allowed_os_versions.findall('os-version')
+        self.assertEqual(len(os_versions), 1)
+        os_version = os_versions[0].get('min')
+        self.assertEqual(os_version, '10.19.7')
+
     def test_package_and_sign_dmg_no_branding(self, **kwargs):
         manager = mock.Mock()
         for attr in kwargs:
@@ -278,6 +314,8 @@ class TestPipelineHelpers(unittest.TestCase):
         self.assertEqual('AppProduct-99.0.9999.99',
                          kwargs['sign_part'].mock_calls[0][1][2].identifier)
 
+    @mock.patch('signing.pipeline._productbuild_distribution_path',
+                _productbuild_distribution_path)
     def test_package_and_sign_pkg_no_branding(self, **kwargs):
         manager = mock.Mock()
         for attr in kwargs:
@@ -292,7 +330,6 @@ class TestPipelineHelpers(unittest.TestCase):
                          pipeline._package_and_sign_pkg(paths, dist_config))
 
         manager.assert_has_calls([
-            mock.call.run_command(mock.ANY),
             mock.call.run_command(mock.ANY),
             mock.call.run_command(mock.ANY)
         ])
@@ -301,22 +338,13 @@ class TestPipelineHelpers(unittest.TestCase):
             call for call in manager.mock_calls if call[0] == 'run_command'
         ]
         pkgbuild_args = run_commands[0][1][0]
-        productbuild_synthesize_args = run_commands[1][1][0]
-        productbuild_args = run_commands[2][1][0]
+        productbuild_args = run_commands[1][1][0]
 
         self.assertEqual('test.signing.bundle_id',
                          _get_adjacent_item(pkgbuild_args, '--identifier'))
         self.assertEqual('99.0.9999.99',
                          _get_adjacent_item(pkgbuild_args, '--version'))
 
-        self.assertTrue('--synthesize' in productbuild_synthesize_args)
-        self.assertEqual(
-            '$W/App Product.req',
-            _get_adjacent_item(productbuild_synthesize_args, '--product'))
-        self.assertEqual(
-            '$W/AppProduct.pkg',
-            _get_adjacent_item(productbuild_synthesize_args, '--package'))
-
         self.assertEqual(
             '$W/App Product.dist',
             _get_adjacent_item(productbuild_args, '--distribution'))
@@ -361,6 +389,8 @@ class TestPipelineHelpers(unittest.TestCase):
         self.assertEqual('AppProduct-99.0.9999.99-MOO',
                          kwargs['sign_part'].mock_calls[0][1][2].identifier)
 
+    @mock.patch('signing.pipeline._productbuild_distribution_path',
+                _productbuild_distribution_path)
     def test_package_and_sign_pkg_branding(self, **kwargs):
         manager = mock.Mock()
         for attr in kwargs:
@@ -379,7 +409,6 @@ class TestPipelineHelpers(unittest.TestCase):
                          pipeline._package_and_sign_pkg(paths, dist_config))
 
         manager.assert_has_calls([
-            mock.call.run_command(mock.ANY),
             mock.call.run_command(mock.ANY),
             mock.call.run_command(mock.ANY)
         ])
@@ -388,22 +417,13 @@ class TestPipelineHelpers(unittest.TestCase):
             call for call in manager.mock_calls if call[0] == 'run_command'
         ]
         pkgbuild_args = run_commands[0][1][0]
-        productbuild_synthesize_args = run_commands[1][1][0]
-        productbuild_args = run_commands[2][1][0]
+        productbuild_args = run_commands[1][1][0]
 
         self.assertEqual('test.signing.bundle_id',
                          _get_adjacent_item(pkgbuild_args, '--identifier'))
         self.assertEqual('99.0.9999.99',
                          _get_adjacent_item(pkgbuild_args, '--version'))
 
-        self.assertTrue('--synthesize' in productbuild_synthesize_args)
-        self.assertEqual(
-            '$W/App Product.req',
-            _get_adjacent_item(productbuild_synthesize_args, '--product'))
-        self.assertEqual(
-            '$W/AppProduct.pkg',
-            _get_adjacent_item(productbuild_synthesize_args, '--package'))
-
         self.assertEqual(
             '$W/App Product.dist',
             _get_adjacent_item(productbuild_args, '--distribution'))