Issue 520 - Generate PAD files for download portals when updating download links

Issue 520 - Generate PAD files for download portals when updating download links

[Wladimir Palant, 2014-05-26 10:54:57]

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/.sit...
File .sitescripts.example (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/.sit...
.sitescripts.example:49: abp_padTemplate=extensions/template/pad.xml
Why list the template for each extension individually if it is always the same
template?

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/bin/updateDownloadLinks.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/bin/updateDownloadLinks.py:73: response =
urlopen('https://clients2.google.com/service/update2/crx?x=' +
urlencode('id=%s&uc' % galleryID))
a) Why use an overly long line instead of a temporary param variable?

b) Why use string concatenation rather than substitution here? I seem to
remember that you were generally opposing string concatenation in Python.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/bin/updateDownloadLinks.py:126: if repo.type in
("chrome", "opera") and repo.galleryID:
Isn't the repo.type check redundant here? You check the type below anyway.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/pad/__init__.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:16: import subprocess, os, re, time
One import per line?

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:21: from sitescripts.extensions.android
import get_min_android_version
from ..android? Note that most code uses absolute imports but that's simply
because I didn't know that imports can be relative.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:22: from
sitescripts.extensions.pad.language import iso2pad
from .language?

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:25: OS_WINDOWS = ['Win7 x32', 'Win7 x64',
'WinVista', 'WinVista x64', 'WinXP']
What about Win 8 and Win 8.1?

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:34: self.download_url =
re.sub(r'(?<=^http)s(?=:)', '', download_url)
WTF? Are we downgrading download links to plain HTTP? Why?

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:37: self.first_release = (i == 0)
Isn't that supposed to be inside the if block below? But even then it won't
really be wrong - the downloads are listed in a random order, not in the release
order.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:61: return
os.stat(os.path.join(self.repo.downloadsRepo, self.download_filename)).st_size
This won't work, the downloads repo on the main server isn't updated to the
current revision. You probably want to use |hg cat| and measure the length of
the output.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:65: return
self.repo.readMetadata(self.version).items('compat')[0][1].split('/')[0].rstrip('.0')
This will do for Chrome extensions but not Firefox - Firefox extensions
typically list multiple items under compat and the order in which they will be
returned can be random. Not that this line is really readable/understandable as
it is, it needs to be split up in a piece of code that other people can follow.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/pad/validation.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/validation.py:97: return
I assume that libxml2 is being used due to its XPath support. Why not use
xml.etree package that is installed by default?

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/template/pad.xml (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/template/pad.xml:16:
<Company_WebSite_URL>http://eyeo.com</Company_WebSite_URL>
https please.

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

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/utils.py:18: import re, os, subprocess
One import per line?

[Sebastian Noack, 2014-05-26 12:04:53]

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/.sit...
File .sitescripts.example (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/.sit...
.sitescripts.example:49: abp_padTemplate=extensions/template/pad.xml
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> Why list the template for each extension individually if it is always the same
> template?

We are not generating a PAD file for every extension. So we need a way to mark
the extensions we are going to generate PAD file for. And if we are going to
generate a PAD file for other extensions (like URL fixer or EHH) in the future,
we might want to use a different template. So I found it most straight-forward
and flexible this way.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/bin/updateDownloadLinks.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/bin/updateDownloadLinks.py:73: response =
urlopen('https://clients2.google.com/service/update2/crx?x=' +
urlencode('id=%s&uc' % galleryID))
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> a) Why use an overly long line instead of a temporary param variable?
Agreed.

> b) Why use string concatenation rather than substitution here? I seem to
> remember that you were generally opposing string concatenation in Python.
When only concatenating two strings, string concatenation by plus operator feels
more intuitive and less complex. However when concatenating multiple strings,
string formatting is more efficient and IMO easier to read.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/bin/updateDownloadLinks.py:126: if repo.type in
("chrome", "opera") and repo.galleryID:
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> Isn't the repo.type check redundant here? You check the type below anyway.

You are right.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/pad/__init__.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:16: import subprocess, os, re, time
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> One import per line?

Done.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:21: from sitescripts.extensions.android
import get_min_android_version
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> from ..android? Note that most code uses absolute imports but that's simply
> because I didn't know that imports can be relative.

I avoid relative imports for following reasons:

1) You might have to change the import when moving code to a different module.
2) It makes it harder to locate imports for a given module using tools like
grep.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:22: from
sitescripts.extensions.pad.language import iso2pad
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> from .language?

See above.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:25: OS_WINDOWS = ['Win7 x32', 'Win7 x64',
'WinVista', 'WinVista x64', 'WinXP']
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> What about Win 8 and Win 8.1?

Windows 8 is not in the specification:
http://pad.asp-software.org/spec/spec.php

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:34: self.download_url =
re.sub(r'(?<=^http)s(?=:)', '', download_url)
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> WTF? Are we downgrading download links to plain HTTP? Why?

The specification requires an http:// or ftp:// URL here:
http://pad.asp-software.org/spec/spec.php

Yes, this is ridiculous.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:37: self.first_release = (i == 0)
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> Isn't that supposed to be inside the if block below? But even then it won't
> really be wrong - the downloads are listed in a random order, not in the
release
> order.

first_release must be True if there is only one release build, otherwise it must
be False. So it must not be inside the if-block. Otherwise it would be wrongly
set to True if there are multiple builds, but the one matching the version is
listed first.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:61: return
os.stat(os.path.join(self.repo.downloadsRepo, self.download_filename)).st_size
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> This won't work, the downloads repo on the main server isn't updated to the
> current revision. You probably want to use |hg cat| and measure the length of
> the output.

Done.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:65: return
self.repo.readMetadata(self.version).items('compat')[0][1].split('/')[0].rstrip('.0')
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> This will do for Chrome extensions but not Firefox - Firefox extensions
> typically list multiple items under compat and the order in which they will be
> returned can be random. Not that this line is really readable/understandable
as
> it is, it needs to be split up in a piece of code that other people can
follow.

Done.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/pad/validation.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/validation.py:97: return
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> I assume that libxml2 is being used due to its XPath support. Why not use
> xml.etree package that is installed by default?

I just gave etree a try. However their xpath implementation is very limited,
most notable it doesn't support lookups relative to the document root (/).

Also the documentation states that the xml.* modules have known security
vulnerabilities:
https://docs.python.org/2/library/xml.html#xml-vulnerabilities

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/template/pad.xml (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/template/pad.xml:16:
<Company_WebSite_URL>http://eyeo.com</Company_WebSite_URL>
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> https please.

Again, the specification doesn't allow https:// URLs here:
http://pad.asp-software.org/spec/spec.php

Yes, ridiculous.

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

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/utils.py:18: import re, os, subprocess
On 2014/05/26 10:54:57, Wladimir Palant wrote:
> One import per line?

Done.

[Wladimir Palant, 2014-05-26 13:26:53]

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/.sit...
File .sitescripts.example (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/.sit...
.sitescripts.example:49: abp_padTemplate=extensions/template/pad.xml
On 2014/05/26 12:04:53, Sebastian Noack wrote:
> We are not generating a PAD file for every extension.

Why not? We don't have to publish the links to these files but it won't hurt to
have them around.

Note that I don't consider the error-prone per-extension configuration of
templates a straight-forward way.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/bin/updateDownloadLinks.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/bin/updateDownloadLinks.py:73: response =
urlopen('https://clients2.google.com/service/update2/crx?x=' +
urlencode('id=%s&uc' % galleryID))
On 2014/05/26 12:04:53, Sebastian Noack wrote:
> When only concatenating two strings, string concatenation by plus operator
feels
> more intuitive and less complex.

What we have here is a URL template, substitution seems to be a better choice
then - the string to be replaced doesn't necessarily need to be at the end of
the URL.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/pad/__init__.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:25: OS_WINDOWS = ['Win7 x32', 'Win7 x64',
'WinVista', 'WinVista x64', 'WinXP']
On 2014/05/26 12:04:53, Sebastian Noack wrote:
> Windows 8 is not in the specification:
> http://pad.asp-software.org/spec/spec.php

That HTML page links to
http://repository.appvisor.com/padspec/files/padspec40.xml and apparently
"Windows 8" is allowed (not "Windows 8.1" however).

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:34: self.download_url =
re.sub(r'(?<=^http)s(?=:)', '', download_url)
On 2014/05/26 12:04:53, Sebastian Noack wrote:
> The specification requires an http:// or ftp:// URL here:
> http://pad.asp-software.org/spec/spec.php

Same as above - the XML version of the spec allows HTTPS.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:37: self.first_release = (i == 0)
On 2014/05/26 12:04:53, Sebastian Noack wrote:
> first_release must be True if there is only one release build, otherwise it
must
> be False. So it must not be inside the if-block. Otherwise it would be wrongly
> set to True if there are multiple builds, but the one matching the version is
> listed first.

I guess I understand the idea behind this now, you are essentially doing
self.first_release = (len(repo.getDownloads()) > 1) in a very non-obvious way.
Can this be made more obvious? E.g.:

self.first_release = True
for ...
  if i > 0:
    self.first_release = False

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/pad/validation.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/validation.py:97: return
On 2014/05/26 12:04:53, Sebastian Noack wrote:
> I just gave etree a try. However their xpath implementation is very limited,
> most notable it doesn't support lookups relative to the document root (/).

Why does it have to? Relative lookups should do just fine if you look up
relative to the document root.

> Also the documentation states that the xml.* modules have known security
> vulnerabilities:
> https://docs.python.org/2/library/xml.html#xml-vulnerabilities

Yes, they have a warning about that at the top. But we are talking about the
data we generated ourselves, it isn't supposed to be malicious.

[Sebastian Noack, 2014-05-27 12:01:23]

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/.sit...
File .sitescripts.example (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/.sit...
.sitescripts.example:49: abp_padTemplate=extensions/template/pad.xml
On 2014/05/26 13:26:53, Wladimir Palant wrote:
> On 2014/05/26 12:04:53, Sebastian Noack wrote:
> > We are not generating a PAD file for every extension.
> 
> Why not? We don't have to publish the links to these files but it won't hurt
to
> have them around.
Some details in the template (like the descriptions) are currently specific to
Adblock Plus.

> Note that I don't consider the error-prone per-extension configuration of
> templates a straight-forward way.

Changed, as discussed.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/bin/updateDownloadLinks.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/bin/updateDownloadLinks.py:73: response =
urlopen('https://clients2.google.com/service/update2/crx?x=' +
urlencode('id=%s&uc' % galleryID))
On 2014/05/26 13:26:53, Wladimir Palant wrote:
> On 2014/05/26 12:04:53, Sebastian Noack wrote:
> > When only concatenating two strings, string concatenation by plus operator
> feels
> > more intuitive and less complex.
> 
> What we have here is a URL template, substitution seems to be a better choice
> then - the string to be replaced doesn't necessarily need to be at the end of
> the URL.

Done.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/pad/__init__.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:25: OS_WINDOWS = ['Win7 x32', 'Win7 x64',
'WinVista', 'WinVista x64', 'WinXP']
On 2014/05/26 13:26:53, Wladimir Palant wrote:
> On 2014/05/26 12:04:53, Sebastian Noack wrote:
> > Windows 8 is not in the specification:
> > http://pad.asp-software.org/spec/spec.php
> 
> That HTML page links to
> http://repository.appvisor.com/padspec/files/padspec40.xml and apparently
> "Windows 8" is allowed (not "Windows 8.1" however).

Done.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:34: self.download_url =
re.sub(r'(?<=^http)s(?=:)', '', download_url)
On 2014/05/26 13:26:53, Wladimir Palant wrote:
> On 2014/05/26 12:04:53, Sebastian Noack wrote:
> > The specification requires an http:// or ftp:// URL here:
> > http://pad.asp-software.org/spec/spec.php
> 
> Same as above - the XML version of the spec allows HTTPS.

Done.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/__init__.py:37: self.first_release = (i == 0)
On 2014/05/26 13:26:53, Wladimir Palant wrote:
> On 2014/05/26 12:04:53, Sebastian Noack wrote:
> > first_release must be True if there is only one release build, otherwise it
> must
> > be False. So it must not be inside the if-block. Otherwise it would be
wrongly
> > set to True if there are multiple builds, but the one matching the version
is
> > listed first.
> 
> I guess I understand the idea behind this now, you are essentially doing
> self.first_release = (len(repo.getDownloads()) > 1) in a very non-obvious way.
> Can this be made more obvious? E.g.:
> 
> self.first_release = True
> for ...
>   if i > 0:
>     self.first_release = False

Done.

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
File sitescripts/extensions/pad/validation.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/5738600293466112/site...
sitescripts/extensions/pad/validation.py:97: return
On 2014/05/26 13:26:53, Wladimir Palant wrote:
> On 2014/05/26 12:04:53, Sebastian Noack wrote:
> > I just gave etree a try. However their xpath implementation is very limited,
> > most notable it doesn't support lookups relative to the document root (/).
> 
> Why does it have to? Relative lookups should do just fine if you look up
> relative to the document root.
> 
> > Also the documentation states that the xml.* modules have known security
> > vulnerabilities:
> > https://docs.python.org/2/library/xml.html#xml-vulnerabilities
> 
> Yes, they have a warning about that at the top. But we are talking about the
> data we generated ourselves, it isn't supposed to be malicious.

I am using mindom (without xpath) now. Also I'm validating against a more recent
version of PAD, that allows HTTPS URLs and Windows 8.

[Wladimir Palant, 2014-06-04 14:32:04]

LGTM with the nits addressed.

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
File sitescripts/extensions/pad/__init__.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
sitescripts/extensions/pad/__init__.py:67: ['hg', 'cat',
self.download_filename],
Please add |-r tip| parameters here, otherwise you are still dependent on the
revision of the working directory.

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
File sitescripts/extensions/pad/validation.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
sitescripts/extensions/pad/validation.py:140: leaf = False
Nit: this variable is assigned but never used.

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
sitescripts/extensions/pad/validation.py:167: validate_fields(nested_fields,
node.childNodes, filename)
Nit: wrong indentation here, smart tabs?

[Sebastian Noack, 2014-06-04 16:38:58]

Please note that I also added a customized description for Adblock Plus for
Safari. That was requested by Simona, for fun, SEO and profit. ;P

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
File sitescripts/extensions/pad/__init__.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
sitescripts/extensions/pad/__init__.py:67: ['hg', 'cat',
self.download_filename],
On 2014/06/04 14:32:04, Wladimir Palant wrote:
> Please add |-r tip| parameters here, otherwise you are still dependent on the
> revision of the working directory.

Done.

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
File sitescripts/extensions/pad/validation.py (right):

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
sitescripts/extensions/pad/validation.py:140: leaf = False
On 2014/06/04 14:32:04, Wladimir Palant wrote:
> Nit: this variable is assigned but never used.

Done.

http://codereview.adblockplus.org/5723465818570752/diff/6107477854650368/site...
sitescripts/extensions/pad/validation.py:167: validate_fields(nested_fields,
node.childNodes, filename)
On 2014/06/04 14:32:04, Wladimir Palant wrote:
> Nit: wrong indentation here, smart tabs?

I'm still using tabs (instead spaces) for everything as default. And apparently
forgot to execute ":set et sw=2 ts=2" when I opened that file in vim.

[Wladimir Palant, 2014-06-04 17:17:16]

Even more LGTM