Merge remote-tracking branch 'tor-github/pr/1667'

1 files changed, 141 insertions, 59 deletions

diff --git a/scripts/maint/add_c_file.py b/scripts/maint/add_c_file.py
index 9ec182efcc..e1e224d8d5 100755
--- a/scripts/maint/add_c_file.py
+++ b/scripts/maint/add_c_file.py
@@ -4,6 +4,21 @@
    Add a C file with matching header to the Tor codebase.  Creates
    both files from templates, and adds them to the right include.am file.
 
+   This script takes paths relative to the top-level tor directory. It
+   expects to be run from that directory.
+
+   This script creates files, and inserts them into include.am, also
+   relative to the top-level tor directory.
+
+   But the template content in those files is relative to tor's src
+   directory. (This script strips "src" from the paths used to create
+   templated comments and macros.)
+
+   This script expects posix paths, so it should be run with a python
+   where os.path is posixpath. (Rather than ntpath.) This probably means
+   Linux, macOS, or BSD, although it might work on Windows if your python
+   was compiled with mingw, MSYS, or cygwin.
+
    Example usage:
 
    % add_c_file.py ./src/feature/dirauth/ocelot.c
@@ -18,42 +33,60 @@ import os
 import re
 import time
 
-def topdir_file(name):
-    """Strip opening "src" from a filename"""
-    return os.path.relpath(name, './src')
-
-def guard_macro(name):
-    """Return the guard macro that should be used for the header file 'name'.
+def tordir_file(fname):
+    """Make fname relative to the current directory, which should be the
+       top-level tor directory. Also performs basic path simplifications."""
+    return os.path.normpath(os.path.relpath(fname))
+
+def srcdir_file(tor_fname):
+    """Make tor_fname relative to tor's "src" directory.
+       Also performs basic path simplifications.
+       (This function takes paths relative to the top-level tor directory,
+       but outputs a path that is relative to tor's src directory.)"""
+    return os.path.normpath(os.path.relpath(tor_fname, 'src'))
+
+def guard_macro(src_fname):
+    """Return the guard macro that should be used for the header file
+       'src_fname'. This function takes paths relative to tor's src directory.
     """
-    td = topdir_file(name).replace(".", "_").replace("/", "_").upper()
+    td = src_fname.replace(".", "_").replace("/", "_").upper()
     return "TOR_{}".format(td)
 
-def makeext(name, new_extension):
-    """Replace the extension for the file called 'name' with 'new_extension'.
+def makeext(fname, new_extension):
+    """Replace the extension for the file called 'fname' with 'new_extension'.
+       This function takes and returns paths relative to either the top-level
+       tor directory, or tor's src directory, and returns the same kind
+       of path.
     """
-    base = os.path.splitext(name)[0]
+    base = os.path.splitext(fname)[0]
     return base + "." + new_extension
 
-def instantiate_template(template, output_fname):
+def instantiate_template(template, tor_fname):
     """
     Fill in a template with string using the fields that should be used
-    for 'output_fname'.
+    for 'tor_fname'.
+
+    This function takes paths relative to the top-level tor directory,
+    but the paths in the completed template are relative to tor's src
+    directory. (Except for one of the fields, which is just a basename).
     """
+    src_fname = srcdir_file(tor_fname)
     names = {
         # The relative location of the header file.
-        'header_path' : makeext(topdir_file(output_fname), "h"),
+        'header_path' : makeext(src_fname, "h"),
         # The relative location of the C file file.
-        'c_file_path' : makeext(topdir_file(output_fname), "c"),
+        'c_file_path' : makeext(src_fname, "c"),
         # The truncated name of the file.
-        'short_name' : os.path.basename(output_fname),
+        'short_name' : os.path.basename(src_fname),
         # The current year, for the copyright notice
         'this_year' : time.localtime().tm_year,
         # An appropriate guard macro, for the header.
-        'guard_macro' : guard_macro(output_fname),
+        'guard_macro' : guard_macro(src_fname),
     }
 
     return template.format(**names)
 
+# This template operates on paths relative to tor's src directory
 HEADER_TEMPLATE = """\
 /* Copyright (c) 2001 Matej Pfajfar.
  * Copyright (c) 2001-2004, Roger Dingledine.
@@ -72,6 +105,7 @@ HEADER_TEMPLATE = """\
 #endif /* !defined({guard_macro}) */
 """
 
+# This template operates on paths relative to the tor's src directory
 C_FILE_TEMPLATE = """\
 /* Copyright (c) 2001 Matej Pfajfar.
  * Copyright (c) 2001-2004, Roger Dingledine.
@@ -93,16 +127,22 @@ class AutomakeChunk:
     Represents part of an automake file.  If it is decorated with
     an ADD_C_FILE comment, it has a "kind" based on what to add to it.
     Otherwise, it only has a bunch of lines in it.
+
+    This class operates on paths relative to the top-level tor directory.
     """
     pat = re.compile(r'# ADD_C_FILE: INSERT (\S*) HERE', re.I)
 
     def __init__(self):
         self.lines = []
         self.kind = ""
+        self.hasBlank = False # true if we end with a blank line.
 
     def addLine(self, line):
         """
         Insert a line into this chunk while parsing the automake file.
+
+        Return True if we have just read the last line in the chunk, and
+        False otherwise.
         """
         m = self.pat.match(line)
         if m:
@@ -110,23 +150,28 @@ class AutomakeChunk:
                 raise ValueError("control line not preceded by a blank line")
             self.kind = m.group(1)
 
-        self.lines.append(line)
         if line.strip() == "":
+            self.hasBlank = True
             return True
 
+        self.lines.append(line)
+
         return False
 
-    def insertMember(self, member):
+    def insertMember(self, new_tor_fname):
         """
-        Add a new member to this chunk.  Try to insert it in alphabetical
-        order with matching indentation, but don't freak out too much if the
-        source isn't consistent.
+        Add a new file name new_tor_fname to this chunk.  Try to insert it in
+        alphabetical order with matching indentation, but don't freak out too
+        much if the source isn't consistent.
 
         Assumes that this chunk is of the form:
            FOOBAR = \
               X     \
               Y     \
               Z
+
+        This function operates on paths relative to the top-level tor
+        directory.
         """
         prespace = "\t"
         postspace = "\t\t"
@@ -134,20 +179,21 @@ class AutomakeChunk:
             m = re.match(r'(\s+)(\S+)(\s+)\\', line)
             if not m:
                 continue
-            prespace, fname, postspace = m.groups()
-            if fname > member:
-                self.insert_before(lineno, member, prespace, postspace)
+            prespace, cur_tor_fname, postspace = m.groups()
+            if cur_tor_fname > new_tor_fname:
+                self.insert_before(lineno, new_tor_fname, prespace, postspace)
                 return
-        self.insert_at_end(member, prespace, postspace)
+        self.insert_at_end(new_tor_fname, prespace, postspace)
 
-    def insert_before(self, lineno, member, prespace, postspace):
+    def insert_before(self, lineno, new_tor_fname, prespace, postspace):
         self.lines.insert(lineno,
-                          "{}{}{}\\\n".format(prespace, member, postspace))
+                          "{}{}{}\\\n".format(prespace, new_tor_fname,
+                                              postspace))
 
-    def insert_at_end(self, member, prespace, postspace):
-        lastline = self.lines[-1]
-        self.lines[-1] += '{}\\\n'.format(postspace)
-        self.lines.append("{}{}\n".format(prespace, member))
+    def insert_at_end(self, new_tor_fname, prespace, postspace):
+        lastline = self.lines[-1].strip()
+        self.lines[-1] = '{}{}{}\\\n'.format(prespace, lastline, postspace)
+        self.lines.append("{}{}\n".format(prespace, new_tor_fname))
 
     def dump(self, f):
         """Write all the lines in this chunk to the file 'f'."""
@@ -156,9 +202,14 @@ class AutomakeChunk:
             if not line.endswith("\n"):
                 f.write("\n")
 
+        if self.hasBlank:
+            f.write("\n")
+
 class ParsedAutomake:
     """A sort-of-parsed automake file, with identified chunks into which
        headers and c files can be inserted.
+
+       This class operates on paths relative to the top-level tor directory.
     """
     def __init__(self):
         self.chunks = []
@@ -169,12 +220,15 @@ class ParsedAutomake:
         self.chunks.append(chunk)
         self.by_type[chunk.kind.lower()] = chunk
 
-    def add_file(self, fname, kind):
-        """Insert a file of kind 'kind' to the appropriate section of this
-           file. Return True if we added it.
+    def add_file(self, tor_fname, kind):
+        """Insert a file tor_fname of kind 'kind' to the appropriate
+           section of this file. Return True if we added it.
+
+           This function operates on paths relative to the top-level tor
+           directory.
         """
         if kind.lower() in self.by_type:
-            self.by_type[kind.lower()].insertMember(fname)
+            self.by_type[kind.lower()].insertMember(tor_fname)
             return True
         else:
             return False
@@ -184,49 +238,77 @@ class ParsedAutomake:
         for chunk in self.chunks:
             chunk.dump(f)
 
-def get_include_am_location(fname):
-    """Find the right include.am file for introducing a new file.  Return None
-       if we can't guess one.
+def get_include_am_location(tor_fname):
+    """Find the right include.am file for introducing a new file
+       tor_fname.  Return None if we can't guess one.
 
        Note that this function is imperfect because our include.am layout is
        not (yet) consistent.
+
+       This function operates on paths relative to the top-level tor directory.
     """
-    td = topdir_file(fname)
-    m = re.match(r'^(lib|core|feature|app)/([a-z0-9_]*)/', td)
+    # Strip src for pattern matching, but add it back when returning the path
+    src_fname = srcdir_file(tor_fname)
+    m = re.match(r'^(lib|core|feature|app)/([a-z0-9_]*)/', src_fname)
     if m:
         return "src/{}/{}/include.am".format(m.group(1),m.group(2))
 
-    if re.match(r'^test/', td):
+    if re.match(r'^test/', src_fname):
         return "src/test/include.am"
 
     return None
 
-def run(fn):
-    """
-    Create a new C file and H file corresponding to the filename "fn", and
-    add them to include.am.
+def run(fname):
     """
+    Create a new C file and H file corresponding to the filename "fname",
+    and add them to the corresponding include.am.
 
-    cf = makeext(fn, "c")
-    hf = makeext(fn, "h")
+    This function operates on paths relative to the top-level tor directory.
+    """
 
-    if os.path.exists(cf):
-        print("{} already exists".format(cf))
+    # Make sure we're in the top-level tor directory,
+    # which contains the src directory
+    if not os.path.isdir("src"):
+        raise RuntimeError("Could not find './src/'. "
+                           "Run this script from the top-level tor source "
+                           "directory.")
+
+    # And it looks like a tor/src directory
+    if not os.path.isfile("src/include.am"):
+        raise RuntimeError("Could not find './src/include.am'. "
+                           "Run this script from the top-level tor source "
+                           "directory.")
+
+    # Make the file name relative to the top-level tor directory
+    tor_fname = tordir_file(fname)
+    # And check that we're adding files to the "src" directory,
+    # with canonical paths
+    if tor_fname[:4] != "src/":
+        raise ValueError("Requested file path '{}' canonicalized to '{}', "
+                         "but the canonical path did not start with 'src/'. "
+                         "Please add files to the src directory."
+                         .format(fname, tor_fname))
+
+    c_tor_fname = makeext(tor_fname, "c")
+    h_tor_fname = makeext(tor_fname, "h")
+
+    if os.path.exists(c_tor_fname):
+        print("{} already exists".format(c_tor_fname))
         return 1
-    if os.path.exists(hf):
-        print("{} already exists".format(hf))
+    if os.path.exists(h_tor_fname):
+        print("{} already exists".format(h_tor_fname))
         return 1
 
-    with open(cf, 'w') as f:
-        f.write(instantiate_template(C_FILE_TEMPLATE, cf))
+    with open(c_tor_fname, 'w') as f:
+        f.write(instantiate_template(C_FILE_TEMPLATE, c_tor_fname))
 
-    with open(hf, 'w') as f:
-        f.write(instantiate_template(HEADER_TEMPLATE, hf))
+    with open(h_tor_fname, 'w') as f:
+        f.write(instantiate_template(HEADER_TEMPLATE, h_tor_fname))
 
-    iam = get_include_am_location(cf)
+    iam = get_include_am_location(c_tor_fname)
     if iam is None or not os.path.exists(iam):
         print("Made files successfully but couldn't identify include.am for {}"
-              .format(cf))
+              .format(c_tor_fname))
         return 1
 
     amfile = ParsedAutomake()
@@ -238,8 +320,8 @@ def run(fn):
                 cur_chunk = AutomakeChunk()
         amfile.addChunk(cur_chunk)
 
-    amfile.add_file(cf, "sources")
-    amfile.add_file(hf, "headers")
+    amfile.add_file(c_tor_fname, "sources")
+    amfile.add_file(h_tor_fname, "headers")
 
     with open(iam+".tmp", 'w') as f:
         amfile.dump(f)