Safari Extension Update Manifest Generation Broken

[matze]

November 17th we received the following error-message in production:

Subject: Cron <sitescripts@***> /usr/local/bin/update_update_manifests
Date: Fri, 18 Nov 2016 07:20:13 +0000 (UTC)
From: Cron Daemon <root@***>

Traceback (most recent call last):
  File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/opt/sitescripts/sitescripts/extensions/bin/updateUpdateManifests.py", line 139, in <module>
    updateUpdateManifests()
  File "/opt/sitescripts/sitescripts/extensions/bin/updateUpdateManifests.py", line 135, in updateUpdateManifests
    getDownloadLinks(parser)
  File "/opt/sitescripts/sitescripts/extensions/utils.py", line 324, in getDownloadLinks
    result.set(repo.repositoryName, 'downloadURL', downloadURL)
  File "/usr/lib/python2.7/ConfigParser.py", line 743, in set
    raise TypeError("option values must be strings")
TypeError: option values must be strings

This lead us to discovering an issue introduced in the context if #1483, revision ​8c3d5a9c5328: The sitescripts/extensions/utils.py script does not cover the case when no downloadURL is found any more. The following hot-fix made it work in production again:

diff -r 92b7a4428709 sitescripts/extensions/utils.py
--- a/sitescripts/extensions/utils.py   Mon Nov 07 14:01:53 2016 +0000
+++ b/sitescripts/extensions/utils.py   Fri Nov 18 16:50:44 2016 +0000
@@ -291,8 +291,15 @@
     gets the download link to the most current version of an extension
     """
     if repo.galleryID and repo.type == 'gecko':
-        return _getMozillaDownloadLink(repo.galleryID)
-    return _getLocalLink(repo)
+        result = _getMozillaDownloadLink(repo.galleryID)
+    else:
+        result = _getLocalLink(repo)
+
+    if result[0] is None:
+        message = 'Failed to get download link for {}'.format(repo)
+        raise Exception(message)
+
+    return result


 def _getQRCode(text):

While the script does not abort on error with one repository any more, we now saw what resource caused the issue popping up in the first place:

Traceback (most recent call last):
  File "/opt/sitescripts/sitescripts/extensions/utils.py", line 325, in getDownloadLinks
    (downloadURL, version) = _getDownloadLink(repo)
  File "/opt/sitescripts/sitescripts/extensions/utils.py", line 300, in _getDownloadLink
    raise Exception(message)
Exception: Failed to get download link for adblockplussafari

So it seems like in addition to the minor bug above there is an issue with the Safari extension's update manifest generation. Probably this one is related to the circumstance that this one is part of the Chrome repository now. Here's an excerpt of the related sitescripts.ini settings:

$ grep safari /etc/sitescripts.ini
adblockplussafari_repository=%(sitescripts_var_root)s/adblockpluschrome
adblockplussafari_type=safari
adblockplussafari_key=%(sitescripts_var_root)s/adblockplussafari.pem
safariUpdateManifestPath=%(web_root)s/adblockplussafari/updates.plist
safariUpdateManifest=extensions/template/updates.plist

All other repositories' update manifest generations work flawlessly.

A major effect of this is that ​the link to the ABP for Safari extension on adblockplus.org is not 404.

ToDo

• Improve on the aforementioned patch-set to fix the cron-job
• Identify and fix the issue preventing Safari update manifest generation

Note

Currently the update mechanism for the sitescripts repository on the affected production host(s) has been paused. Hence no other code updates will come through here until we re-activate that one, which we will do after a patch-set similar to the hot-fix has landed.

[kzar]

(I believe the current URL for the latest Safari release build is ​https://downloads.adblockplus.org/adblockplussafari-1.12.4.safariextz .)

[philll]

I recommend providing the download for ABP for Safari from the URL kzar posted above until this issue gets fixed.

[kzar]

@matze what is the contents (if any) of the broken Safari update manifest? (File at the path given by safariUpdateManifestPath.)

[matze]

$ cat /var/www/update/adblockplussafari/updates.plist
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
   <key>Extension Updates</key>
   <array>
   </array>
</dict>
</plist>

[kzar]

I am not familiar with this code, but after having a quick look I've found the/a problem.

sitescripts.extensions.utils.getDownloads calls the readMetadata function with no arguments. That function defaults to the "tip" revision and so it will try to read the metadata.safari file for the "tip" revision. That no longer exists, which explains why the problem started happening since November 17th as that was the day that ​the commit to remove that file landed.

If I'm right - and I didn't test that I am - then I think this should fix it:

diff --git a/sitescripts/extensions/utils.py b/sitescripts/extensions/utils.py
index da53e91..1e55607 100644
--- a/sitescripts/extensions/utils.py
+++ b/sitescripts/extensions/utils.py
@@ -207,7 +207,7 @@ class Configuration(object):
         return parser
 
     def getDownloads(self):
-        metadata = self.readMetadata()
+        metadata = self.readMetadata(self.revision)
         if metadata:
             prefix = metadata.get('general', 'basename')
         else:

[matze]

We have extended the hotfix already, pretty similar to what kzar proposed above:

diff -r 92b7a4428709 sitescripts/extensions/utils.py
--- a/sitescripts/extensions/utils.py	Mon Nov 07 14:01:53 2016 +0000
+++ b/sitescripts/extensions/utils.py	Wed Nov 23 17:29:21 2016 +0000
@@ -183,11 +183,12 @@
         """
         return self.repositoryName
 
-    def readMetadata(self, version='tip'):
+    def readMetadata(self, version=None):
+        revision = version or self.revision
         genericFilename = 'metadata'
         filename = '%s.%s' % (genericFilename, self.type)
         files = subprocess.check_output(['hg', '-R', self.repository,
-                                         'locate', '-r', version]).splitlines()
+                                         'locate', '-r', revision]).splitlines()
 
         if filename not in files:
             # some repositories like those for Android and
@@ -198,7 +199,7 @@
             # Fall back to platform-independent metadata file
             filename = genericFilename
 
-        command = ['hg', '-R', self.repository, 'cat', '-r', version, os.path.join(self.repository, filename)]
+        command = ['hg', '-R', self.repository, 'cat', '-r', revision, os.path.join(self.repository, filename)]
         result = subprocess.check_output(command)
 
         parser = SafeConfigParser()
@@ -291,8 +292,15 @@
     gets the download link to the most current version of an extension
     """
     if repo.galleryID and repo.type == 'gecko':
-        return _getMozillaDownloadLink(repo.galleryID)
-    return _getLocalLink(repo)
+        result = _getMozillaDownloadLink(repo.galleryID)
+    else:
+        result = _getLocalLink(repo)
+
+    if None is result[0]:
+        message = 'Failed to get download link for {}'.format(repo)
+        raise Exception(message)
+
+    return result
 
 
 def _getQRCode(text):

Smoke testing suggests that this should work now. Sidenote: The HG repositories on the update server did not feature *any* bookmarks. I guess they where initially cloned when the repos did not have bookmarks yet.

[matze]

Also, we've had to update sitescripts.ini to include the following:

adblockplussafari_revision=safari

[sebastian]

While there is a temporary workaround manually deployed on the server now, thus Adblock Plus for Safari can be downloaded again, we should still fix this properly as soon as possible. We cannot update the sitescripts code on that server anymore until this bug has been addressed upstream, since doing so would revert the manually applied changes.

[abpbot]

A commit referencing this issue has landed:
​Issue 4653 - Add handling for bookmarks to readMetadata

[abpbot]

A commit referencing this issue has landed:
​Issue 4653 - Set adblockplussafari_revision in sitescripts for updateserver role