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

packagerEdge.py

Index: packagerEdge.py
diff --git a/packagerEdge.py b/packagerEdge.py
index 23c32403ed13e6f0da0d7909a97c4ffe5f08c1f1..39bde3674c931ed66d628eb4a546b3995d6e5cbc 100644
--- a/packagerEdge.py
+++ b/packagerEdge.py
@@ -84,6 +84,7 @@ def create_appx_manifest(params, files, release_build=False):
     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'