Issue 1299 - Generate docs outside the devbuild build process

sitescripts/docs/bin/generate_docs.py

 #!/usr/bin/env python
-# coding: utf-8

[Sebastian Noack, 2016/02/05 18:02:32]

Not that a coding declaration is necessary here. Moreover, according to PEP-8
source files (in Python 2) should use the ASCII anyway.

[Felix Dahlke, 2016/02/05 20:47:58]

Wow true, either that part changed or we misunderstood it all along. We have
this coding declaration in 95% of the source files in sitescripts. I'll leave it
out here then, but we should consider changing that across the bank.

 
 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # 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
 # 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
 import shutil
 import subprocess
 
 from sitescripts.utils import get_config
 
-config = dict(get_config().items("docs"))

[Sebastian Noack, 2016/02/05 18:02:31]

I'd prefer if you use the ConfigParser API consistently rather than turning it
into a dictionary for no obvious reason.

[Felix Dahlke, 2016/02/05 20:47:57]

Done.

-
-def read_projects():
+def read_projects(config):
   projects = {}
-  for key, value in config.iteritems():
+  for key, value in config.items("docs"):
     key_parts = key.split("_", 1)
     if len(key_parts) < 2:
       continue
     project_name, field_name = key_parts
-    if field_name not in ["repository", "target_directory", "command"]:

[Sebastian Noack, 2016/02/05 18:02:32]

Nit: A set seems more appropriate here. In case you didn't know yet there are
also set literals:

  {"repository", "target_directory", "command"}

[Felix Dahlke, 2016/02/05 20:47:58]

No I didn't, nice. Done.

+    if field_name not in {"repository", "target_directory", "command"}:
       continue
-    projects.setdefault(project_name, {})

[Sebastian Noack, 2016/02/05 18:02:31]

There is a reason that setdefault() returns the respective value:

  projects.setdefault(project_name, {})[field_name] = value

[Felix Dahlke, 2016/02/05 20:47:58]

Done.

-    projects[project_name][field_name] = value
+    projects.setdefault(project_name, {})[field_name] = value
   return projects
 
 def sync_sources(sources_dir, repository_url):
-  remote_id = subprocess.check_output(["hg", "id", "-i", repository_url])

[Sebastian Noack, 2016/02/05 18:02:32]

Nit: Please use long option names in scripts, that people not familiar with the
invoked program have a better idea what it does. (The shorthand syntax mostly
exist for convenience on the shell.)

[Felix Dahlke, 2016/02/05 20:47:58]

Done.

-  id_path = sources_dir.rstrip("/") + ".id"

[Sebastian Noack, 2016/02/05 18:02:31]

Please don't hard-code file separators, use.ospath.sep.

[Felix Dahlke, 2016/02/05 20:47:58]

Done.

-  if os.path.exists(id_path):

[Sebastian Noack, 2016/02/05 18:02:31]

It's better to handle the corresponding IOError, rather than performing checks
in advance. That not only keeps the kernel/IO-roundtrip small, but also avoids
race conditions.

[Felix Dahlke, 2016/02/05 20:47:57]

Fair enough, I've gotten rid of the os.path.exists() I had an idea for how to
get rid of. Ideas welcome for the remaining ones.

-    with open(id_path, "r") as id_file:

[Sebastian Noack, 2016/02/05 18:02:32]

Please always add "b" to the file mode, for consistent behavior on Windows.

[Felix Dahlke, 2016/02/05 20:47:58]

Done.

-      local_id = id_file.read()
-    if local_id == remote_id:
-      return
   if os.path.exists(sources_dir):
-    shutil.rmtree(sources_dir)
-  subprocess.check_call(["hg", "archive", "-R", repository_url, "-r", "master",
-                         sources_dir])
-
-  # In theory, it is possible that some changesets are pushed after we fetch
-  # the ID above, but before we run `hg archive`, which will lead to an
-  # unnecessary `hg archive` operation the next time this runs.
-  with open(id_path, "w") as id_file:
-    id_file.write(remote_id)
+    subprocess.check_call(["hg", "pull", "--quiet",
+                           "--rev", "master",
+                           "--repository", sources_dir])
+    subprocess.check_call(["hg", "update", "--quiet",
+                           "--rev", "master"])
+  else:
+    subprocess.check_call(["hg", "clone", "--quiet",
+                           "--updaterev", "master",
+                           repository_url, sources_dir])
 
 def replace_dir(source_dir, target_dir):
   if not os.path.exists(target_dir):
     parent_dir = os.path.dirname(target_dir)
-    if not os.path.exists(parent_dir):
+    try:
       os.makedirs(parent_dir)
+    except OSError:
+      pass
     os.rename(source_dir, target_dir)
   else:
-    old_target_dir = target_dir.rstrip("/") + ".old"
-    if os.path.exists(old_target_dir):
-      shutil.rmtree(old_target_dir)
+    old_target_dir = target_dir.rstrip(os.path.sep) + ".old"
+    shutil.rmtree(old_target_dir, ignore_errors=True)
     os.rename(target_dir, old_target_dir)
     os.rename(source_dir, target_dir)
     shutil.rmtree(old_target_dir)
 
 def run_generation_command(command, sources_dir, output_dir):
-  if os.path.exists(output_dir):
-    shutil.rmtree(output_dir)
+  shutil.rmtree(output_dir, ignore_errors=True)
   command = command.format(output_dir=output_dir)
   subprocess.check_call(command, shell=True, cwd=sources_dir)
 
-def generate_docs(projects):
-  temp_directory = config["temp_directory"]
-  if not os.path.exists(temp_directory):
+def generate_docs(projects, config):
+  temp_directory = config.get("docs", "temp_directory")
+  try:
     os.makedirs(temp_directory)
+  except OSError:
+    pass
 
   for name, data in projects.iteritems():
     sources_dir = os.path.join(temp_directory, name)
     sync_sources(sources_dir, data["repository"])
-    output_dir = sources_dir.rstrip("/") + ".docs"
+    output_dir = sources_dir.rstrip(os.path.sep) + ".docs"
     run_generation_command(data["command"], sources_dir, output_dir)
     replace_dir(output_dir, data["target_directory"])
 
 if __name__ == "__main__":
-  projects = read_projects()
-  generate_docs(projects)
+  config = get_config()
+  projects = read_projects(config)
+  generate_docs(projects, config)