Issue 1144 - Generate IE update manifests

Issue 1144 - Generate IE update manifests

[Wladimir Palant, 2014-07-29 11:25:18]

It seems that some changes were implemented to prevent a special code path for
IE in updateUpdateManifests.py. However, IE *is* special. So by removing
complexity there you made the code significantly worse elsewhere.

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
File sitescripts/extensions/bin/createNightlies.py (left):

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
sitescripts/extensions/bin/createNightlies.py:210: def
writeLibabpUpdateManifest(self, updates):
Why did you remove this function? Are you going to duplicate the logic for
Android updates?

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
File sitescripts/extensions/bin/createNightlies.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
sitescripts/extensions/bin/createNightlies.py:205: templateName =
'ieUpdateManifest'
libabpUpdateManifest please, this format isn't specific to Internet Explorer.
Also, new settings should be documented in .sitescripts.example.

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
sitescripts/extensions/bin/createNightlies.py:224: self.version = versions[0]
This function shouldn't set any instance variables - it is a write function, it
shouldn't have any side-effects.

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
File sitescripts/extensions/template/ie-update.json (right):

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
sitescripts/extensions/template/ie-update.json:3: "{{extension.name}}/msie32": {
At the very least this needs proper escaping (json template filter). However, it
is generally a very bad idea to use templates to produce JSON code, it's too
easy to break the syntax.

[Felix Dahlke, 2014-07-29 11:37:16]

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
File sitescripts/extensions/bin/createNightlies.py (left):

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
sitescripts/extensions/bin/createNightlies.py:210: def
writeLibabpUpdateManifest(self, updates):
On 2014/07/29 11:25:18, Wladimir Palant wrote:
> Why did you remove this function? Are you going to duplicate the logic for
> Android updates?

Do we need special logic for Android here? How I see it, we could simply replace
the current update.xml template by an Android-specific update.json template when
we switch to libadblockplus-style manifests.

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
File sitescripts/extensions/bin/createNightlies.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
sitescripts/extensions/bin/createNightlies.py:205: templateName =
'ieUpdateManifest'
On 2014/07/29 11:25:18, Wladimir Palant wrote:
> libabpUpdateManifest please, this format isn't specific to Internet Explorer.
> Also, new settings should be documented in .sitescripts.example.

As it is, the update manifest is IE specific, we won't have 32-bit/64-bit
variants for Android e.g. A more generic solution would be nice, but then again,
IE is the only special case we have here how I see it. Coming up with something
generic for a single use case doesn't seem wise.

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
sitescripts/extensions/bin/createNightlies.py:224: self.version = versions[0]
On 2014/07/29 11:25:18, Wladimir Palant wrote:
> This function shouldn't set any instance variables - it is a write function,
it
> shouldn't have any side-effects.

True, it's a hack. How about I move this part to where we set self.basename for
IE below?

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
File sitescripts/extensions/template/ie-update.json (right):

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
sitescripts/extensions/template/ie-update.json:3: "{{extension.name}}/msie32": {
On 2014/07/29 11:25:18, Wladimir Palant wrote:
> At the very least this needs proper escaping (json template filter). However,
it
> is generally a very bad idea to use templates to produce JSON code, it's too
> easy to break the syntax.

The same could be said for XML, does that mean you'd like to move away from
templates in general?

[Felix Dahlke, 2014-07-29 11:47:58]

As discussed on IRC, we want to move away from templates for manifests (RDF and
Safari's plist being the only long term exception). So I'll generate JSON for
both nightly builds and stable update manifests.

[Wladimir Palant, 2014-07-29 12:03:10]

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
File sitescripts/extensions/bin/createNightlies.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5649050225344512/site...
sitescripts/extensions/bin/createNightlies.py:224: self.version = versions[0]
On 2014/07/29 11:37:17, Felix H. Dahlke wrote:
> True, it's a hack. How about I move this part to where we set self.basename
for
> IE below?

Yes, that would make sense.

[Felix Dahlke, 2014-07-29 15:10:27]

Uploaded a new patch set that generates update.json without using a template, as
discussed. It'll need some work still, see my comments.

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
File sitescripts/extensions/bin/createNightlies.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
sitescripts/extensions/bin/createNightlies.py:223:
writeIEUpdateManifest(manifestPath, [{
Not brilliant that this function has the same name as the current method -
suggestions?

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
File sitescripts/extensions/bin/updateUpdateManifests.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
sitescripts/extensions/bin/updateUpdateManifests.py:73: 'basename':
os.path.basename(repo.repository)
That's a hack if there ever was one, don't think we want to keep this. The only
alternative I see is to add a setting for this - do you know a better way?

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
File sitescripts/extensions/utils.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
sitescripts/extensions/utils.py:408: baseDir = os.path.dirname(path)
We didn't do this for any of the configured update manifest paths so far, only
for devbuilds. But doing it in createNightlies.writeIEUpdateManifest seemed
wrong too.

[Wladimir Palant, 2014-07-30 14:02:22]

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/.sit...
File .sitescripts.example (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/.sit...
.sitescripts.example:103: ieUpdateManifestPath=%(root)s/www/ieupdate.json
Obsolete?

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
File sitescripts/extensions/bin/createNightlies.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
sitescripts/extensions/bin/createNightlies.py:223:
writeIEUpdateManifest(manifestPath, [{
On 2014/07/29 15:10:27, Felix H. Dahlke wrote:
> Not brilliant that this function has the same name as the current method -
> suggestions?

Don't import the function globally but only in this function, under an alias?

from sitescripts.extensions.utils import writeIEUpdateManifest as doWrite

doWrite(...)

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
File sitescripts/extensions/utils.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
sitescripts/extensions/utils.py:408: baseDir = os.path.dirname(path)
It definitely doesn't hurt, for the update server it will typically be a no-op
however.

[Felix Dahlke, 2014-07-30 14:22:35]

New patch set.

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/.sit...
File .sitescripts.example (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/.sit...
.sitescripts.example:103: ieUpdateManifestPath=%(root)s/www/ieupdate.json
On 2014/07/30 14:02:23, Wladimir Palant wrote:
> Obsolete?

This is not the template, it's the destination.

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
File sitescripts/extensions/bin/createNightlies.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
sitescripts/extensions/bin/createNightlies.py:223:
writeIEUpdateManifest(manifestPath, [{
On 2014/07/30 14:02:23, Wladimir Palant wrote:
> On 2014/07/29 15:10:27, Felix H. Dahlke wrote:
> > Not brilliant that this function has the same name as the current method -
> > suggestions?
> 
> Don't import the function globally but only in this function, under an alias?
> 
> from sitescripts.extensions.utils import writeIEUpdateManifest as doWrite
> 
> doWrite(...)

Yeah, I like that. Done.

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
File sitescripts/extensions/bin/updateUpdateManifests.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
sitescripts/extensions/bin/updateUpdateManifests.py:73: 'basename':
os.path.basename(repo.repository)
On 2014/07/29 15:10:27, Felix H. Dahlke wrote:
> That's a hack if there ever was one, don't think we want to keep this. The
only
> alternative I see is to add a setting for this - do you know a better way?

So, are you actually OK with this? Well, it does work.

[Wladimir Palant, 2014-07-30 14:37:50]

LGTM

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
File sitescripts/extensions/bin/updateUpdateManifests.py (right):

http://codereview.adblockplus.org/6270530592178176/diff/5271204772446208/site...
sitescripts/extensions/bin/updateUpdateManifests.py:73: 'basename':
os.path.basename(repo.repository)
On 2014/07/30 14:22:36, Felix H. Dahlke wrote:
> So, are you actually OK with this? Well, it does work.

Of course, it is a hack. However, there isn't really a way around it right now -
IE and Android don't have a metadata file that we could extract the basename
from. So at this point there isn't much we can do. Ideally, we would support IE
and Android in build tools properly.