Issue 3176 - Add metadata to content blocker lists

sitescripts/content_blocker_lists/bin/generate_lists.py

 #!/usr/bin/env python
 # coding: utf-8
 
 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # Copyright (C) 2006-2015 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/>.
 
 from collections import OrderedDict
 from contextlib import closing
-from datetime import datetime
 import json
 import os
 import subprocess
-import thread
+import threading
+import time
 import re
 import urllib2
 
 from sitescripts.utils import get_config
 
 config = dict(get_config().items("content_blocker_lists"))
 
 def update_abp2blocklist():

[Felix Dahlke, 2015/12/01 08:43:09]

These functions were prefixed with an underscore for a reason, see:
https://adblockplus.org/coding-style#python

[Sebastian Noack, 2015/12/01 10:18:39]

Well, one could argue that this is less a module (to be imported somewhere
else), but more like a stand-alone script providing a CLI as high-level
interface. Therefore these methods wouldn't necessarily have to be considered
private. I probably wouldn't have prefixed them myself in this case. However,
removing the prefixes is a completely unrelated change. So I agree, that it
should be removed from this patch.

[kzar, 2015/12/01 12:13:38]

I would prefer to leave them off, they don't really add anything in this context
and they are kind of ugly. (It's true that it's an unrelated change, but as I'm
changing / creating nearly all the functional definitions in this patch set I
thought it made sense to change this one too.)

[Felix Dahlke, 2015/12/01 14:04:19]

I would argue that this is still a module - this script can only be run with
`python -m`. It isn't a loadable library, that's true, but neither are our web
modules. Also, It's not unheard of to move code between modules under bin and
reusable modules. I really think we should be consistent here, but it's up to
Sebastian. Let's however please clarify this in the coding style, it's obviously
ambiguous right now.

[Sebastian Noack, 2015/12/07 12:38:24]

Well, technically every piece of code is part of a module in Python. But the
point is that all code that sees these functions is allowed/supposed to call
them. Note that since the actual interface here is the CLI, the only code this
applies to, is this script itself. So there is no context in which these
function would/should be considered private. But if for some reason somebody
imports this module rather than running it as a script, which they shouldn't do
however, they would expect a more low-level API than provided by the CLI.

As for consistency, it seems that it's only code you wrote that overuses
prefixed/private functions in scenarios like this. For example have a look at
ensure_dependencies.py or buildtools in general. 

[Felix Dahlke, 2015/12/08 06:51:24]

Wladimir's newer code in Sitescripts also uses those prefixes for stuff under
bin. Anyway, your reasoning makes sense, so I'm fine with never using those
private prefixes for executable scripts. But please do clarify it in the coding
style.

   with open(os.devnull, "w") as devnull:
     abp2blocklist_path = config["abp2blocklist_path"]
     if os.path.isdir(abp2blocklist_path):
         subprocess.check_call(("hg", "pull", "-u", "-R", abp2blocklist_path),
                               stdout=devnull)
     else:
       subprocess.check_call(("hg", "clone", config["abp2blocklist_url"],
                              abp2blocklist_path), stdout=devnull)
     subprocess.check_call(("npm", "install"), cwd=abp2blocklist_path,
                           stdout=devnull)
 
 def download_filter_list(url):
-  filter_list = {}
   with closing(urllib2.urlopen(url)) as response:
-    filter_list["body"] = response.read()
-  filter_list["header"] = parse_filter_list_header(filter_list["body"])
-  filter_list["header"]["url"] = url
-  return filter_list

[Sebastian Noack, 2015/12/01 11:01:48]

I think it would be simpler if you just return a tuple like (body, url, version)
here, unpacking it while iterating over the filter lists. In particular as these
really seem to be the only data we need.

[kzar, 2015/12/01 12:13:38]

Done.

-
-def parse_filter_list_header(filter_list):
-  body_start = re.search(r"^[^![]", filter_list, re.MULTILINE).start()
-  field_re = re.compile(r"^!\s*([^:\s]+):\s*(.+)$", re.MULTILINE)
-  return { match.group(1): match.group(2)
-           for match in field_re.finditer(filter_list, 0, body_start) }
+    body = response.read()
+  version = re.search(r"^(?:[^[!])|^!\s*Version:\s*(.+)$",
+                      body, re.MULTILINE).group(1)
+  return body, url, version
 
 def generate_metadata(filter_lists, expires):
   metadata = OrderedDict((
-    ("version", datetime.utcnow().strftime("%Y%m%d%H%M")),

[Felix Dahlke, 2015/12/01 08:43:08]

FWIW, we're using `time.strftime("%Y%m%d%H%M", time.gmtime())` for this in the
notification handler. But it seems to me those are equivalent.

[Sebastian Noack, 2015/12/01 10:18:39]

I tend to agree, creating a datetime object is unnecessary here. Moreover, I
always have to wrap my head around how datetime handles timezones. I wouldn't
insist though.

[kzar, 2015/12/01 12:13:39]

Done.

+    ("version", time.strftime("%Y%m%d%H%M", time.gmtime())),
     ("expires", expires),
     ("sources", [])
   ))
-  for filter_list in filter_lists:
-    metadata["sources"].append({ k.lower(): filter_list["header"][k]
-                                 for k in ["url", "Version"]})

[Felix Dahlke, 2015/12/01 08:43:08]

Nit: Sebastian convinced me a while ago that tuples are better than lists for
immutable lists like this.

[Sebastian Noack, 2015/12/01 10:18:39]

Well, frankly, I don't think that it matters in the case here. However, when you
assign a sequence to a variable, but changing it isn't intended or supported by
your code, using an immutable type is more appropriate and prevents you from
certain bugs, later. But if the sequence isn't accessible after defining it
anyway, I don't really care.

[Felix Dahlke, 2015/12/01 10:26:29]

Wouldn't insist either.

[Sebastian Noack, 2015/12/01 10:38:09]

For reference, I found a quite interesting answer on SO, on the difference
between tuples and lists, from a more semantic point of view:
https://stackoverflow.com/questions/626759/whats-the-difference-between-list-...

I didn't consider that aspect yet, but I tend to agree. So in our scenario here,
as structure doesn't matter (i.e. adding/removing/changing an item in the
sequence of fields here wouldn't invalidate the whole data structure), a list
would actually be more appropriate than a tuple, in their conclusion.

[kzar, 2015/12/01 12:13:39]

Acknowledged.

+  for body, url, version in filter_lists:
+    metadata["sources"].append({"url": url, "version": version})
   return metadata
+
+def pipe_in(process, filter_lists):
+  try:
+    for body, _, _ in filter_lists:
+      print >>process.stdin, body
+  finally:
+    process.stdin.close()
 
 def write_block_list(filter_lists, path, expires):
   block_list = generate_metadata(filter_lists, expires)
   process = subprocess.Popen(("node", "abp2blocklist.js"),
                              cwd=config["abp2blocklist_path"],
                              stdin=subprocess.PIPE, stdout=subprocess.PIPE)
-  def pipe_in(process):

[Sebastian Noack, 2015/12/01 10:47:14]

Nit: This is inconsistent. You pass in the process but access the filter_lists
via the closure. If you keep it as an inner function I would access all
variables via closure. However, I'd tend to move that function to the top-level.

[kzar, 2015/12/01 12:13:38]

Done.

-    try:
-      for filter_list in filter_lists:
-        print >>process.stdin, filter_list["body"]
-    finally:
-      process.stdin.close()
-      process.wait()
-
-  thread.start_new_thread(pipe_in, (process,))

[Sebastian Noack, 2015/12/01 10:47:14]

Please use the high-level threading module instead. Using the thread module
directly can be considered deprecated as it got prefixed in Python 3.

[kzar, 2015/12/01 12:13:38]

Done.

+  threading.Thread(target=pipe_in, args=(process, filter_lists)).start()
   block_list["rules"] = json.load(process.stdout)
-
-  if process.returncode:
+  if process.wait():
     raise Exception("abp2blocklist returned %s" % process.returncode)
 
   with open(path, "wb") as destination_file:
     json.dump(block_list, destination_file, indent=2, separators=(",", ": "))
 
 if __name__ == "__main__":
   update_abp2blocklist()
 
   easylist = download_filter_list(config["easylist_url"])
   exceptionrules = download_filter_list(config["exceptionrules_url"])
 
   write_block_list([easylist],

[Felix Dahlke, 2015/12/01 08:43:08]

Nit: "block list" is highly ambiguous, we often use that term for ABP style
filter lists. "content blocker list" is the correct term, but I can see why
you'd want to keep it short here, fair enough.

[Sebastian Noack, 2015/12/01 10:18:39]

Well, after all, the program called here is also called "abp2blocklist".
However, if you want to have a more distinct term, I guess we have to invent
one, because there isn't a name for the format we are generating here (container
holding WebKit content blocker rules plus some metadata) yet.

[Felix Dahlke, 2015/12/01 10:26:29]

Good point, let's really leave it alone.

                    config["easylist_content_blocker_path"],
                    config["easylist_content_blocker_expires"])
   write_block_list([easylist, exceptionrules],
                    config["combined_content_blocker_path"],
                    config["combined_content_blocker_expires"])