Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4653 closed defect (fixed)

Safari Extension Update Manifest Generation Broken

Reported by: matze Assignee:
Priority: P1 Milestone:
Module: Sitescripts Keywords:
Cc: jsonesen, kvas, sebastian, kzar, philll, scheer Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29364443

Description (last modified by philll)

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.

Change History (14)

comment:1 Changed 3 years ago by kzar

  • Cc kzar philll scheer added

comment:2 Changed 3 years ago by kzar

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

comment:3 Changed 3 years ago by philll

  • Description modified (diff)

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

comment:4 Changed 3 years ago by kzar

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

comment:5 Changed 3 years ago by 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>

comment:6 Changed 3 years ago by 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:
Last edited 3 years ago by kzar (previous) (diff)

comment:7 Changed 3 years ago by 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.

comment:8 Changed 3 years ago by matze

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

adblockplussafari_revision=safari
Last edited 3 years ago by matze (previous) (diff)

comment:9 Changed 3 years ago by kvas

  • Priority changed from Unknown to P2
  • Ready set

comment:10 Changed 3 years ago by sebastian

  • Priority changed from P2 to P1

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.

Last edited 3 years ago by sebastian (previous) (diff)

comment:11 Changed 3 years ago by jsonesen

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:12 Changed 3 years ago by abpbot

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

comment:13 Changed 3 years ago by jsonesen

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.