Issue 4767 - Improve error reporting in update_update_manifests

sitescripts/extensions/utils.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2016 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
@@ skipping 233 matching lines @@
 
 
 def _urlopen(url, attempts=3):
     """
     Tries to open a particular URL, retries on failure.
     """
     for i in range(attempts):
         try:
             return urllib.urlopen(url)
         except IOError as e:
-            error = e
+            error = Exception('Error {0} while opening {1} url'
+                              .format(e, url))
             time.sleep(5)
     raise error
 
 
+def _parseXMLDocument(url, attempts=2):
+    for i in range(attempts):
+        page = _urlopen(url)
+        content = page.read()
+        page.close()

[Sebastian Noack, 2017/01/17 09:33:32]

The close() should have gone into a finally block, or even better using the with
statement:

  with contextlib.closing(_urlopen(url)) as page:
      content = page.read()

[Sebastian Noack, 2017/01/17 10:16:56]

Yes, in Python 2, the repsonse object returned by urlopen() doesn't implement
__enter__/__exit__. So you either have to use the contextlib.closing() decorator
before passing it to the with statement or use a traditionally try-finally
block. I suggest doing the former, and removing the decorator once we migrate to
Python 3.

Also, please reply inline to inline comment.

[Sebastian Noack, 2017/01/17 10:20:15]

Well, alternatively you could use try-finally. But IMO, the with-statement is
nicer. For reference, here is how we did it in jsHydra where we support both
Python 2+3: https://hg.adblockplus.org/jshydra/file/tip/abp_rewrite.py#l11

[Vasily Kuznetsov, 2017/01/17 10:30:34]

Yeah, with is nicer, especially having Python 3 in mind.

+        try:
+            return dom.parseString(content)
+        except ExpatError as err:
+            exception = Exception('Error {0} while parsing xml:\n{1}\nfrom {2}'
+                                  .format(err, content, url))
+    raise exception
+
+
 def _getMozillaDownloadLink(galleryID):
     """
     gets download link for a Gecko add-on from the Mozilla Addons site
     """
     url = 'https://services.addons.mozilla.org/en-US/firefox/api/1/addon/%s' % _urlencode(galleryID)
-    document = dom.parse(_urlopen(url))
+    document = _parseXMLDocument(url)
     linkTags = document.getElementsByTagName('install')
     linkTag = linkTags[0] if len(linkTags) > 0 else None
     versionTags = document.getElementsByTagName('version')
     versionTag = versionTags[0] if len(versionTags) > 0 else None
     if linkTag and versionTag and linkTag.firstChild and versionTag.firstChild:
         return (linkTag.firstChild.data, versionTag.firstChild.data)
     else:
         return (None, None)
 
 
@@ skipping 39 matching lines @@
     """
     gets the download links for all extensions and puts them into the config
     object
     """
     for repo in Configuration.getRepositoryConfigurations():
         try:
             (downloadURL, version) = _getDownloadLink(repo)
             if downloadURL is None:
                 raise Exception('No download link found for repo: ' +
                                 repo.repositoryName)
-        except ExpatError:
-            traceback.print_exc()
-            print 'Error found while parsing xml from {0} link'\

[Vasily Kuznetsov, 2017/01/09 21:59:54]

Breaking lines with a backslash (`\`) is generally not recommended. It's
preferrable to do something like this:

print ('bla bla {0}'
       .format(arg))

Also previous line prints to stderr and this print will print to stderr. Is this
intended or should we print to stderr here as well?

[Vasily Kuznetsov, 2017/01/09 22:01:12]

Sorry, I meant that this print statement will print to stdout (while we probably
want stderr).

-                .format(repo.repositoryName)
-            continue
         except:
             traceback.print_exc()
             continue
         if not result.has_section(repo.repositoryName):
             result.add_section(repo.repositoryName)
         result.set(repo.repositoryName, 'downloadURL', downloadURL)
         result.set(repo.repositoryName, 'version', version)
 
         qrcode = _getQRCode(downloadURL)
         if qrcode is not None:
@@ skipping 46 matching lines @@
     if not extensions:
         return
 
     updates = {}
     for extension in extensions:
         updates[extension['basename']] = {
             'url': extension['updateURL'],
             'version': extension['version']
         }
     writeLibabpUpdateManifest(path, updates)