Added script for copying external filterlists into existing repositories

sitescripts/extensions/bin/updateExternalFilterlists.py

 # coding: utf-8

[Wladimir Palant, 2013/06/24 12:35:10]

Probably better to have that script under management/bin

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2013 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
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import os, subprocess, codecs, urllib, re

[Felix Dahlke, 2013/06/25 10:10:02]

Seems like the re module is not being used here.

 from sitescripts.utils import get_config, setupStderr
 from tempfile import mkdtemp
 from shutil import rmtree
 
-def resolveRepositoryPath(repositories, path):
-  repoName = path
-  if path.find(':') >= 0:
-    repoName, target = path.split(':', 1)
-  if not repoName in repositories:
-    raise Exception('Cannot resolve path to repository "%s", this repository is unknown' % repoName)
-
-  repoPath = repositories[repoName]
-  relTargetDir = ''
-  if not repoName == path:
-    targetDir = os.path.join(repoPath, target)
-    relTargetDir = os.path.relpath(targetDir, repoPath)
-  
-  return repoPath, relTargetDir
-
-
-def updateExternalFilterlists(repositories):
+def updateExternalFiles():
   settings = readSettings()
   for name, setting in settings.iteritems():

[Wladimir Palant, 2013/06/25 11:21:03]

Nit: for setting in settings.itervalues() - the key isn't being used.

-    tempdir = mkdtemp(prefix='adblockplus')

[Wladimir Palant, 2013/06/24 12:35:10]

A more generic specific prefix might be better here, maybe 'external'. Sometimes
these temp directories will stay around and it is good to know which script
created them.

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

-    repoPath, targetDir = resolveRepositoryPath(repositories, setting['target'])
+    tempdir = mkdtemp(prefix='external')
     try:

[Wladimir Palant, 2013/06/24 12:35:10]

This try statement should immediately follow mkdtemp - if resolveRepositoryPath
throws then the temp directory won't be removed.

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

-      subprocess.Popen(['hg', 'clone', '-U', repoPath, tempdir], stdout=subprocess.PIPE).communicate()
-      subprocess.Popen(['hg', 'up', '-R', tempdir, '-r', 'default'], stdout=subprocess.PIPE).communicate()
+      repoPath = setting['targetrepository']
+      targetPath = os.path.dirname(setting['targetfile'])
+      filename = os.path.basename(setting['targetfile'])
       
-      path = os.path.join(tempdir, targetDir)
+      subprocess.Popen(['hg', 'clone', '-q', '-U', repoPath, tempdir], stdout=subprocess.PIPE).communicate()
+      subprocess.Popen(['hg', 'up', '-q', '-R', tempdir, '-r', 'default'], stdout=subprocess.PIPE).communicate()
+      
+      path = os.path.join(tempdir, targetPath)
       if not os.path.exists(path):
         os.makedirs(path)
       
-      filename = name + '.txt'

[Wladimir Palant, 2013/06/24 12:35:10]

The settings should really specify the full file name - the setting name is
merely a key to keep the related settings together.

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

       path = os.path.join(path, filename)
       exists = os.path.exists(path)
       file = codecs.open(path, 'wb', encoding='utf-8')
       data = urllib.urlopen(setting['source']).read()

[Wladimir Palant, 2013/06/25 11:21:03]

You need to add .decode('utf-8') at the end - as is this is being interpreted as
ASCII data and will cause issues if the file has non-ASCII content.

-      for line in str(data).splitlines():

[Wladimir Palant, 2013/06/24 12:35:10]

Why split lines and write/decode each line separately? Please use file.write()
to write out all the data at once.

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

-        if not line:
-          continue
-        print >>file, line.strip().decode('iso-8859-1')

[Wladimir Palant, 2013/06/24 12:35:10]

The source file encoding should always be UTF-8, definitely not ISO-8859-1.

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

-      file.close();

[Wladimir Palant, 2013/06/24 12:35:10]

No semicolon please.

[Thomas Greiner, 2013/06/24 14:24:45]

Done. :)

+      file.write(data)
+      file.close()
 
-      message = 'Updated copy of external filterlist %s'
+      message = 'Updated copy of external file %s'
       if not exists:
-        subprocess.Popen(['hg', 'add', '-R', tempdir], stdout=subprocess.PIPE).communicate()

[Wladimir Palant, 2013/06/24 12:35:10]

No need to add the file explicitly, use hg commit -A

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

-        message = 'Added copy of external filterlist %s'
-      subprocess.Popen(['hg', 'commit', '-R', tempdir, '-u', 'hgbot', '-m', message % filename], stdout=subprocess.PIPE).communicate()
-      subprocess.Popen(['hg', 'push', '-R', tempdir], stdout=subprocess.PIPE).communicate()

[Wladimir Palant, 2013/06/24 12:35:10]

Please use -q command line parameter for all Mercurial calls - we will run this
from cron, there should be no output unless something is wrong.

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

+        message = 'Added copy of external file %s'
+      subprocess.Popen(['hg', 'commit', '-q', '-A', '-R', tempdir, '-u', 'hgbot', '-m', message % filename], stdout=subprocess.PIPE).communicate()
+      subprocess.Popen(['hg', 'push', '-q', '-R', tempdir], stdout=subprocess.PIPE).communicate()
     finally:
       rmtree(tempdir)
 
 def readSettings():
   result = {}
-  for option in get_config().options('externalFilterlists'):

[Wladimir Palant, 2013/06/24 12:35:10]

Better:

for option, value in get_config().items('externalFiles'):

Then you won't need to get the value separately later.

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

+  for option, value in get_config().items('externalFiles'):
     if option.find('_') < 0:
       continue
     name, setting = option.rsplit('_', 2)
-    if not setting in ('source', 'target'):
+    if not setting in ('source', 'targetrepository', 'targetfile'):
       continue
 
     if not name in result:
       result[name] = {
         'source': None,
-        'target': None
+        'targetrepository': None,
+        'targetfile': None
       }
-    if isinstance(result[name][setting], list):

[Wladimir Palant, 2013/06/24 12:35:10]

What is that check for? I think we want exactly one value for each setting. And
this will never be true from what I can tell.

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

-      result[name][setting] = get_config().get('externalFilterlists', option).split(' ')
-    else:
-      result[name][setting] = get_config().get('externalFilterlists', option)
+    result[name][setting] = value
   return result
 
 if __name__ == '__main__':
   setupStderr()
-  
-  repositories = {}
-  for option, value in get_config().items('subscriptionDownloads'):
-    if option.endswith('_repository'):
-      repositories[re.sub(r'_repository$', '', option)] = value

[Wladimir Palant, 2013/06/24 12:35:10]

This script shouldn't really combine settings from two different sections - see
my proposal in .sitescripts.example.

[Thomas Greiner, 2013/06/24 14:24:45]

Done.

-  
-  updateExternalFilterlists(repositories)
+  updateExternalFiles()