[buildtools] Issue 4578 - Make uap3:AppExtension.Id configurable for Microsoft Edge builds

packagerEdge.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import base64
 import hashlib
 import json
 import mimetypes
 import os
 import zipfile
@@ skipping 66 matching lines @@
 
 
 def create_appx_manifest(params, files, release_build=False):
     """Create AppxManifest.xml."""
     params = dict(params)
     metadata = params['metadata']
     w = params['windows_version'] = {}
     w['min'], w['max'] = metadata.get('compat', 'windows').split('/')
     params.update(metadata.items('general'))
     params['version'] = pad_version(params['version'])
+    params['app_extension_id'] = '1.0' if release_build else 'EdgeExtension'

[Wladimir Palant, 2016/12/16 10:39:25]

Given that this logic isn't something that is inherent to Edge extensions but
rather a mess that we (and Microsoft) created, this should be a configurable
parameter IMHO - so we don't have this hardcoded in buildtools but rather
configured in the particular project. Maybe [general] app_extension_id for
release builds and [general] app_extension_id_devbuild for development builds.

[Sebastian Noack, 2016/12/16 11:20:11]

I already had the same thought. But on the other hand, this way the
implementation is much simpler, and it is unlikely that we will build another
Edge extension any time soon, and if we do this can be made configurable easily
later on. No strong opinion from my side.

[Sebastian Noack, 2016/12/19 17:19:52]

Yes, it is permanent for Adblock Plus for Microsoft Edge, which has already been
released, i.e. published on the Windows Store, though labelled as beta, for now.
But once we leave beta it will still be the same item. So there is no way to get
rid of this inconsistency anymore.

However, this issue is specific to the Adblock Plus release version (and the few
other Microsoft Edge extensions that have been on the Windows Store from the
beginning). And if we should ever release another extension for Microsoft Edge,
separate from Adblock Plus, we might use the same code for packaging it, but
then the Id must be set to "EdgeExtension" also for release builds. At that
point it would have to be configurable per extension.

But this would add quite a bit unnecessary complexity and configuration
boilerplate for the time being, if this code is even ever used to build any
other extension for Microsoft Edge. I don't have a strong opinion here.

[Wladimir Palant, 2017/01/05 12:20:49]

I don't really see what this discussion is about. Having this hardcoded might do
the job but this is still ugly and not really self-explaining. I had to read
through the issue myself to understand what this is supposed to be. And I don't
see much complexity being added by solving this properly:

    id_setting = 'id' if release_build else 'id_devbuild'
    if metadata.has_option('general', id_setting):
      params['app_extension_id'] = metadata.get('general', id_setting)
    else:
      params['app_extension_id'] = 'EdgeExtension'

And we can comment in metadate.edge that we keep that id=1.0 setting for legacy
reasons, because we can no longer change it for the already published extension.

[Sebastian Noack, 2017/01/05 12:27:57]

Thanks for getting back to us. As I said above, I don't have a strong opinion
either way. I just offered an alternative point of view. However, since you seem
to feel strong about it, lets make it configurable, I guess.

[wspee, 2017/01/06 16:18:10]

I have made it configure able similar to how you suggested it. The fallback if
id hasn't been configured isn't really necessary, so I omitted it, right?

[Sebastian Noack, 2017/01/07 10:18:46]

I think it makes sense to use 'EdgeExtension' as default, and only specify the
id in the metadata file if it differs. Note that new extensions must use the
value 'EdgeExtension' anyway. So if we should ever use this code to bundle any
other extension for Microsfot Edge in the future, we don't need to bother, if we
use the appropriate default here.

[wspee, 2017/01/09 13:32:27]

Done.

 
     translation = load_translation(files, defaultLocale)
     name_key = 'name' if release_build else 'name_devbuild'
     params['display_name'] = translation[name_key]['message']
     params['description'] = translation['description']['message']
 
     for size in ['44', '50', '150']:
         path = '{}/logo_{}.png'.format(ASSETS_DIR, size)
         if path not in files:
             raise KeyError(path + ' is not found in files')
@@ skipping 79 matching lines @@
     if metadata.has_section('appx_assets'):
         for name, path in metadata.items('appx_assets'):
             path = os.path.join(baseDir, path)
             files.read(path, '{}/{}'.format(ASSETS_DIR, name))
 
     files[MANIFEST] = create_appx_manifest(params, files, releaseBuild)
     files[BLOCKMAP] = create_appx_blockmap(files)
     files[CONTENT_TYPES] = create_content_types_map(files.keys() + [BLOCKMAP])
 
     files.zip(outfile, compression=zipfile.ZIP_STORED)