review comments

jwilk-forks · May 20, 2016 · 577872e · 577872e
1 parent b1b28a7
commit 577872e
Show file tree

Hide file tree

Showing 5 changed files with 223 additions and 57 deletions.
diff --git a/admin/check_topfile.py b/admin/check_topfile.py
diff --git a/bin/admin/check-topfile b/bin/admin/check-topfile
@@ -0,0 +1,22 @@
+#!/usr/bin/env python
+# Copyright (c) Twisted Matrix Laboratories.
+# See LICENSE for details.
+
+"""
+Check that the current commits since branching have a topfile.
+"""
+
+from __future__ import absolute_import, division, print_function
+
+import sys, os
+extra = os.path.dirname(os.path.dirname(sys.argv[0]))
+sys.path.insert(0, extra)
+try:
+    import _preamble
+except ImportError:
+    sys.exc_clear()
+sys.path.remove(extra)
+
+from twisted.python._release import CheckTopfileScript
+
+CheckTopfileScript(print).main(sys.argv[1:])
diff --git a/tox.ini b/tox.ini
@@ -54,7 +54,7 @@ commands =
     coverage: python {toxinidir}/admin/_copy.py "_trial_temp/.coverage*" {toxinidir}
     coverage: python {toxinidir}/admin/_copy.py ".coverage*" {toxinidir}
 
-    topfile: python {toxinidir}/admin/check_topfile.py "{toxinidir}"
+    topfile: python {toxinidir}/bin/admin/check-topfile "{toxinidir}"
 
 [testenv:twistedchecker]
 basepython=python2.7

diff --git a/twisted/python/_release.py b/twisted/python/_release.py
@@ -30,6 +30,9 @@
 # The offset between a year and the corresponding major version number.
 VERSION_OFFSET = 2000
 
+# Types of topfiles.
+TOPFILE_TYPES = ["doc", "bugfix", "misc", "feature", "removal"]
+
 
 def runCommand(args, cwd=None):
     """
@@ -1040,3 +1043,60 @@ def main(self, args):
             sys.exit("Must specify two arguments: "
                      "Twisted checkout and destination path")
         self.buildAPIDocs(FilePath(args[0]), FilePath(args[1]))
+
+
+
+class CheckTopfileScript(object):
+    """
+    A thing for checking whether a checkout has a topfile.
+    """
+    def __init__(self, _print):
+        self._print = _print
+
+
+    def main(self, args):
+        """
+        Run the script.
+
+        @type args: L{list} of L{str}
+        @param args: The command line arguments to process. This must contain
+            one string: the path to the root of the Twisted checkout.
+        """
+        if len(args) != 1:
+            sys.exit("Must specify one argument: the Twisted checkout")
+
+        location = os.path.abspath(args[0])
+
+        branch = runCommand([b"git", b"rev-parse", b"--abbrev-ref",  "HEAD"],
+                            cwd=location).strip()
+
+        if branch == "trunk":
+            self._print("On trunk, no need to look at this.")
+            sys.exit(0)
+
+        elif branch.startswith("release-"):
+            self._print("On a release branch, no need to look at this.")
+            sys.exit(0)
+
+        r = runCommand([b"git", b"diff", b"--name-only", b"origin/trunk..."],
+                       cwd=location)
+        files = r.strip().split(os.linesep)
+
+        self._print("Looking at these files:")
+        for change in files:
+            self._print(change)
+        self._print("----")
+
+        if len(files) == 1:
+            if files[0] == os.sep.join(["docs", "fun", "Twisted.Quotes"]):
+                self._print("Quotes change only; no topfile needed.")
+                sys.exit(0)
+
+        for change in files:
+            if os.sep + "topfiles" + os.sep in change:
+                if change.rsplit(".", 1)[1] in TOPFILE_TYPES:
+                    self._print("Found " + change)
+                    sys.exit(0)
+
+        self._print("No topfile found. Have you committed it?")
+        sys.exit(1)
diff --git a/twisted/python/test/test_release.py b/twisted/python/test/test_release.py
@@ -31,7 +31,7 @@
     _changeVersionInFile, getNextVersion, findTwistedProjects, replaceInFile,
     replaceProjectVersion, Project, generateVersionFileData,
     changeAllProjectVersions, VERSION_OFFSET, filePathDelta, CommandFailed,
-    APIBuilder, BuildAPIDocsScript,
+    APIBuilder, BuildAPIDocsScript, CheckTopfileScript,
     runCommand, NotWorkingDirectory,
     ChangeVersionsScript, NewsBuilder, SphinxBuilder,
     GitCommand, getRepositoryCommand, IVCSCommand)
@@ -1699,6 +1699,7 @@ def commitRepository(self, repository):
         runCommand(["git", "-C", repository.path, "commit", "-m", "hop"])
 
 
+
 class RepositoryCommandDetectionTest(ExternalTempdirTestCase):
     """
     Test the L{getRepositoryCommand} to access the right set of VCS commands
@@ -1738,3 +1739,141 @@ def test_git(self):
         L{GitCommand} implements L{IVCSCommand}.
         """
         self.assertTrue(IVCSCommand.implementedBy(GitCommand))
+
+
+
+class CheckTopfileScriptTests(ExternalTempdirTestCase):
+    """
+    Tests for L{CheckTopfileScript}.
+    """
+    skip = gitSkip
+
+    def setUp(self):
+        self.origin = FilePath(self.mktemp())
+        _gitInit(self.origin)
+        runCommand(["git", "checkout", "-b", "trunk"],
+                   cwd=self.origin.path)
+        self.origin.child("test").setContent(b"test!")
+        runCommand(["git", "add", self.origin.child("test").path],
+                   cwd=self.origin.path)
+        runCommand(["git", "commit", "-m", "initial"],
+                   cwd=self.origin.path)
+
+        self.repo = FilePath(self.mktemp())
+
+        runCommand(["git", "clone", self.origin.path, self.repo.path])
+
+
+    def test_noArgs(self):
+        """
+        Too few arguments returns a failure.
+        """
+        logs = []
+
+        with self.assertRaises(SystemExit) as e:
+            r = CheckTopfileScript(logs.append).main([])
+
+        self.assertEqual(e.exception.args,
+                         ("Must specify one argument: the Twisted checkout",))
+
+
+    def test_nothing(self):
+        """
+        No topfiles means a failure.
+        """
+        runCommand(["git", "checkout", "-b", "mypatch"],
+                   cwd=self.repo.path)
+
+        logs = []
+
+        with self.assertRaises(SystemExit) as e:
+            r = CheckTopfileScript(logs.append).main([self.repo.path])
+
+        self.assertEqual(e.exception.args, (1,))
+        self.assertEqual(logs[-1], "No topfile found. Have you committed it?")
+
+
+    def test_trunk(self):
+        """
+        Running it on trunk always gives green.
+        """
+        logs = []
+
+        with self.assertRaises(SystemExit) as e:
+            r = CheckTopfileScript(logs.append).main([self.repo.path])
+
+        self.assertEqual(e.exception.args, (0,))
+        self.assertEqual(logs[-1], "On trunk, no need to look at this.")
+
+
+    def test_release(self):
+        """
+        Running it a release branch always gives green.
+        """
+        runCommand(["git", "checkout", "-b", "release-16.11111-9001"],
+                   cwd=self.repo.path)
+
+        logs = []
+
+        with self.assertRaises(SystemExit) as e:
+            r = CheckTopfileScript(logs.append).main([self.repo.path])
+
+        self.assertEqual(e.exception.args, (0,))
+        self.assertEqual(logs[-1],
+                         "On a release branch, no need to look at this.")
+
+
+    def test_onlyQuotes(self):
+        """
+        Running it on a branch with only a quotefile change gives green.
+        """
+        runCommand(["git", "checkout", "-b", "quotefile"],
+                   cwd=self.repo.path)
+
+        fun = self.repo.child("docs").child("fun")
+        fun.makedirs()
+        quotes = fun.child("Twisted.Quotes")
+        quotes.setContent(b"Beep boop")
+
+        runCommand(["git", "add", quotes.path],
+                   cwd=self.repo.path)
+        runCommand(["git", "commit", "-m", "quotes"],
+                   cwd=self.repo.path)
+
+        logs = []
+
+        with self.assertRaises(SystemExit) as e:
+            r = CheckTopfileScript(logs.append).main([self.repo.path])
+
+        self.assertEqual(e.exception.args, (0,))
+        self.assertEqual(logs[-1],
+                         "Quotes change only; no topfile needed.")
+
+
+    def test_topfileAdded(self):
+        """
+        Running it on a branch with a topfile added
+        """
+        runCommand(["git", "checkout", "-b", "quotefile"],
+                   cwd=self.repo.path)
+
+        topfiles = self.repo.child("twisted").child("topfiles")
+        topfiles.makedirs()
+        fragment = topfiles.child("1234.misc")
+        fragment.setContent(b"")
+
+        unrelated = self.repo.child("somefile")
+        unrelated.setContent(b"Boo")
+
+        runCommand(["git", "add", fragment.path, unrelated.path],
+                   cwd=self.repo.path)
+        runCommand(["git", "commit", "-m", "topgile"],
+                   cwd=self.repo.path)
+
+        logs = []
+
+        with self.assertRaises(SystemExit) as e:
+            r = CheckTopfileScript(logs.append).main([self.repo.path])
+
+        self.assertEqual(e.exception.args, (0,))
+        self.assertEqual(logs[-1], "Found twisted/topfiles/1234.misc")