Merge lp:~abentley/bzr-builder/launchpad into lp:~launchpad-pqm/bzr-builder/trunk

[Aaron Bentley]

This updates to bzr-builder tip so that we can use its improved error messages.

[Māris Fogels (mars)]

Hi Aaron, this looks good. r=mars

You may want to self-review merges like this in future - I am not sure I contributed much :)

review: Approve

[Aaron Bentley (abentley)]

Well, in fact, I submitted against the wrong branch, and the diff didn't show the changes I described, so, there was room for review.

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'recipe.py'
--- recipe.py	2010-09-29 22:35:56 +0000
+++ recipe.py	2010-11-23 18:09:11 +0000
@@ -55,6 +55,13 @@
 NEST_PART_INSTRUCTION = "nest-part"
 NEST_INSTRUCTION = "nest"
 RUN_INSTRUCTION = "run"
+USAGE = {
+    MERGE_INSTRUCTION: 'merge NAME BRANCH [REVISION]',
+    NEST_INSTRUCTION: 'nest NAME BRANCH TARGET-DIR [REVISION]',
+    NEST_PART_INSTRUCTION:
+        'nest-part NAME BRANCH SUBDIR [TARGET-DIR [REVISION]]',
+    RUN_INSTRUCTION: 'run COMMAND',
+    }
 
 SAFE_INSTRUCTIONS = [
     MERGE_INSTRUCTION, NEST_PART_INSTRUCTION, NEST_INSTRUCTION]
@@ -520,11 +527,16 @@
 
     def as_text(self):
         revid_part = self._get_revid_part()
-        if self.target_subdir is not None:
+        if revid_part:
+            target_subdir = self.target_subdir
+            if target_subdir is None:
+                target_subdir = self.subpath
             target_revid_part = " %s%s" % (
-                self.target_subdir, revid_part)
+                target_subdir, revid_part)
+        elif self.target_subdir is not None:
+            target_revid_part = " %s" % self.target_subdir
         else:
-            target_revid_part = revid_part
+            target_revid_part = ""
         return "%s %s %s %s%s" % (
             NEST_PART_INSTRUCTION, self.recipe_branch.name,
             self.recipe_branch.url, self.subpath, target_revid_part)
@@ -762,6 +774,14 @@
                 problem=problem)
 
 
+class InstructionParseError(RecipeParseError):
+    _fmt = RecipeParseError._fmt + "\nUsage: %(usage)s"
+
+    def __init__(self, filename, line, char, problem, instruction):
+        RecipeParseError.__init__(self, filename, line, char, problem)
+        self.usage = USAGE[instruction]
+
+
 class ForbiddenInstructionError(RecipeParseError):
 
     def __init__(self, filename, line, char, problem, instruction_name=None):
@@ -848,17 +868,18 @@
                 instruction = self.parse_instruction(
                     permitted_instructions=permitted_instructions)
                 if instruction == RUN_INSTRUCTION:
-                    self.parse_whitespace("the command")
+                    self.parse_whitespace("the command",
+                        instruction=instruction)
                     command = self.take_to_newline().strip()
                     self.new_line()
                     active_branches[-1].run_command(command)
                 else:
-                    branch_id = self.parse_branch_id()
-                    url = self.parse_branch_url()
+                    branch_id = self.parse_branch_id(instruction)
+                    url = self.parse_branch_url(instruction)
                     if instruction == NEST_INSTRUCTION:
-                        location = self.parse_branch_location()
+                        location = self.parse_branch_location(instruction)
                     if instruction == NEST_PART_INSTRUCTION:
-                        path = self.parse_subpath()
+                        path = self.parse_subpath(instruction)
                         target_subdir = self.parse_optional_path()
                         if target_subdir == '':
                             target_subdir = None
@@ -867,7 +888,7 @@
                             revspec = self.parse_optional_revspec()
                     else:
                         revspec = self.parse_optional_revspec()
-                    self.new_line()
+                    self.new_line(instruction)
                     last_branch = RecipeBranch(branch_id, url, revspec=revspec)
                     if instruction == NEST_INSTRUCTION:
                         active_branches[-1].nest_branch(location, last_branch)
@@ -922,12 +943,13 @@
         self.throw_parse_error("Expecting %s, got '%s'"
                 % (options_str, instruction))
 
-    def parse_branch_id(self):
-        self.parse_whitespace("the branch id")
+    def parse_branch_id(self, instruction):
+        self.parse_whitespace("the branch id", instruction=instruction)
         branch_id = self.peek_to_whitespace()
         if branch_id is None:
             self.throw_parse_error("End of line while looking for the "
-                    "branch id")
+                    "branch id", cls=InstructionParseError,
+                    instruction=instruction)
         if branch_id in self.seen_nicks:
             self.throw_parse_error("'%s' was already used to identify "
                     "a branch." % branch_id)
@@ -935,36 +957,39 @@
         self.seen_nicks.add(branch_id)
         return branch_id
 
-    def parse_branch_url(self):
-        self.parse_whitespace("the branch url")
-        branch_url = self.take_to_whitespace("the branch url")
+    def parse_branch_url(self, instruction):
+        self.parse_whitespace("the branch url", instruction=instruction)
+        branch_url = self.take_to_whitespace("the branch url", instruction)
         return branch_url
 
-    def parse_branch_location(self):
+    def parse_branch_location(self, instruction):
         # FIXME: Needs a better term
         self.parse_whitespace("the location to nest")
         location = self.peek_to_whitespace()
         if location is None:
             self.throw_parse_error("End of line while looking for the "
-                    "location to nest")
+                    "location to nest", cls=InstructionParseError,
+                    instruction=instruction)
         norm_location = os.path.normpath(location)
         if norm_location in self.seen_paths:
             self.throw_parse_error("The path '%s' is a duplicate of "
                     "the one used on line %d." % (location,
-                        self.seen_paths[norm_location]))
+                        self.seen_paths[norm_location]),
+                    InstructionParseError, instruction=instruction)
         if os.path.isabs(norm_location):
             self.throw_parse_error("Absolute paths are not allowed: %s"
-                    % location)
+                    % location, InstructionParseError, instruction=instruction)
         if norm_location.startswith(".."):
             self.throw_parse_error("Paths outside the current directory "
-                    "are not allowed: %s" % location)
+                    "are not allowed: %s" % location,
+                    cls=InstructionParseError, instruction=instruction)
         self.take_chars(len(location))
         self.seen_paths[norm_location] = self.line_index + 1
         return location
 
-    def parse_subpath(self):
-        self.parse_whitespace("the subpath to merge")
-        location = self.take_to_whitespace("the subpath to merge")
+    def parse_subpath(self, instruction):
+        self.parse_whitespace("the subpath to merge", instruction=instruction)
+        location = self.take_to_whitespace("the subpath to merge", instruction)
         return location
 
     def parse_revspec(self):
@@ -999,13 +1024,18 @@
     def throw_eol(self, expected):
         self.throw_parse_error("End of line while looking for '%s'" % expected)
 
-    def new_line(self):
+    def new_line(self, instruction=None):
         # Jump over any whitespace
         self.parse_whitespace(None, require=False)
         remaining = self.peek_to_whitespace()
         if remaining != None:
+            kwargs = {}
+            if instruction is not None:
+                kwargs = {
+                    'cls': InstructionParseError,
+                    'instruction': instruction}
             self.throw_parse_error("Expecting the end of the line, got '%s'"
-                    % remaining)
+                    % remaining, **kwargs)
         self.index = 0
         self.line_index += 1
         if self.line_index >= len(self.lines):
@@ -1071,12 +1101,18 @@
            return old_indent_level
         return None
 
-    def parse_whitespace(self, looking_for, require=True):
+    def parse_whitespace(self, looking_for, require=True, instruction=None):
         if require:
             actual = self.peek_char()
             if actual is None:
+                kwargs = {}
+                if instruction is not None:
+                    kwargs = {
+                        'cls': InstructionParseError,
+                        'instruction': instruction,
+                        }
                 self.throw_parse_error("End of line while looking for "
-                        "%s" % looking_for)
+                        "%s" % looking_for, **kwargs)
             if actual not in self.whitespace_chars:
                 self.throw_parse_error("Expecting whitespace before %s, "
                         "got '%s'." % (looking_for, actual))
@@ -1123,11 +1159,16 @@
             char = self.peek_char(skip=count)
         return ret
 
-    def take_to_whitespace(self, looking_for):
+    def take_to_whitespace(self, looking_for, instruction=None):
         text = self.peek_to_whitespace()
         if text is None:
+            kwargs = {}
+            if instruction is not None:
+                kwargs = {
+                    'cls': InstructionParseError,
+                    'instruction': instruction}
             self.throw_parse_error("End of line while looking for %s"
-                    % looking_for)
+                    % looking_for, **kwargs)
         self.take_chars(len(text))
         return text
 

=== modified file 'tests/test_recipe.py'
--- tests/test_recipe.py	2010-09-29 22:35:56 +0000
+++ tests/test_recipe.py	2010-11-23 18:09:11 +0000
@@ -29,13 +29,19 @@
         BaseRecipeBranch,
         build_tree,
         ensure_basedir,
+        InstructionParseError,
         ForbiddenInstructionError,
+        MERGE_INSTRUCTION,
+        NEST_INSTRUCTION,
+        NEST_PART_INSTRUCTION,
         pull_or_branch,
         RecipeParser,
         RecipeBranch,
         RecipeParseError,
         resolve_revisions,
+        RUN_INSTRUCTION,
         SAFE_INSTRUCTIONS,
+        USAGE,
         )
 
 
@@ -59,6 +65,13 @@
         self.assertEqual("recipe", exc.filename)
         return exc
 
+    def assertInstructionParseError(self, line, char, problem, instruction,
+            callable, *args, **kwargs):
+        exc = self.assertParseError(line, char, problem, callable, *args,
+                **kwargs)
+        self.assertIsInstance(exc, InstructionParseError),
+        self.assertEqual(USAGE[instruction], exc.usage)
+
     def check_recipe_branch(self, branch, name, url, revspec=None,
             num_child_branches=0, revid=None):
         self.assertEqual(name, branch.name)
@@ -138,53 +151,69 @@
                 self.basic_header + "http://foo.org/\n" + "cat")
 
     def test_rejects_merge_no_name(self):
-        self.assertParseError(3, 7, "End of line while looking for "
-                "the branch id", self.get_recipe,
+        self.assertInstructionParseError(3, 7, "End of line while looking for "
+                "the branch id", MERGE_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "merge ")
 
+    def test_rejects_merge_no_name_no_space(self):
+        self.assertInstructionParseError(3, 6, "End of line while looking for "
+                "the branch id", MERGE_INSTRUCTION, self.get_recipe,
+                self.basic_header_and_branch + "merge")
+
     def test_rejects_merge_no_url(self):
-        self.assertParseError(3, 11, "End of line while looking for "
-                "the branch url", self.get_recipe,
+        self.assertInstructionParseError(3, 11, "End of line while looking for"
+                " the branch url", MERGE_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "merge foo ")
 
     def test_rejects_text_at_end_of_merge_line(self):
-        self.assertParseError(3, 17, "Expecting the end of the line, "
-                "got 'bar'", self.get_recipe,
+        self.assertInstructionParseError(3, 17,
+                "Expecting the end of the line, got 'bar'",
+                MERGE_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "merge foo url 2 bar")
 
     def test_rejects_nest_part_no_name(self):
-        self.assertParseError(3, 11, "End of line while looking for "
-                "the branch id", self.get_recipe,
+        self.assertInstructionParseError(3, 11, "End of line while looking for"
+                " the branch id", NEST_PART_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest-part ")
 
     def test_rejects_nest_part_no_url(self):
-        self.assertParseError(3, 15, "End of line while looking for "
-                "the branch url", self.get_recipe,
+        self.assertInstructionParseError(3, 15, "End of line while looking for"
+                " the branch url", NEST_PART_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest-part foo ")
 
     def test_rejects_nest_part_no_subpath(self):
-        self.assertParseError(3, 22, "End of line while looking for "
-                "the subpath to merge", self.get_recipe,
+        self.assertInstructionParseError(3, 22, "End of line while looking for"
+                " the subpath to merge", NEST_PART_INSTRUCTION,
+                self.get_recipe,
                 self.basic_header_and_branch + "nest-part foo url:// ")
 
+    def test_rejects_nest_part_no_subpath_no_space(self):
+        self.assertInstructionParseError(3, 21, "End of line while looking for"
+                " the subpath to merge", NEST_PART_INSTRUCTION,
+                self.get_recipe,
+                self.basic_header_and_branch + "nest-part foo url://")
+
     def test_rejects_nest_no_name(self):
-        self.assertParseError(3, 6, "End of line while looking for "
-                "the branch id", self.get_recipe,
+        self.assertInstructionParseError(3, 6, "End of line while looking for"
+                " the branch id", NEST_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest ")
 
     def test_rejects_nest_no_url(self):
-        self.assertParseError(3, 10, "End of line while looking for "
-                "the branch url", self.get_recipe,
-                self.basic_header_and_branch + "nest foo ")
+        self.assertInstructionParseError(3, 10,
+            "End of line while looking for the branch url",
+            NEST_INSTRUCTION, self.get_recipe,
+            self.basic_header_and_branch + "nest foo ")
 
     def test_rejects_nest_no_location(self):
-        self.assertParseError(3, 14, "End of line while looking for "
-                "the location to nest", self.get_recipe,
+        self.assertInstructionParseError(3, 14,
+                "End of line while looking for the location to nest",
+                NEST_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest foo url ")
 
     def test_rejects_text_at_end_of_nest_line(self):
-        self.assertParseError(3, 20, "Expecting the end of the line, "
-                "got 'baz'", self.get_recipe,
+        self.assertInstructionParseError(3, 20,
+                "Expecting the end of the line, got 'baz'",
+                NEST_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest foo url bar 2 baz")
 
     def test_rejects_indent_after_first_branch(self):
@@ -226,6 +255,12 @@
                 self.basic_header_and_branch + "merge foo url\n"
                 + "merge foo other-url\n")
 
+    def test_rejects_run_with_no_instruction(self):
+        self.assertInstructionParseError(3, 4,
+                "End of line while looking for the command",
+                RUN_INSTRUCTION, self.get_recipe,
+                self.basic_header_and_branch + "run\n")
+
     def test_builds_simplest_recipe(self):
         base_branch = self.get_recipe(self.basic_header_and_branch)
         self.check_base_recipe_branch(base_branch, "http://foo.org/")
@@ -443,34 +478,40 @@
         self.assertEqual("run", exc.instruction_name)
 
     def test_error_on_duplicate_path(self):
-        exc = self.assertParseError(3, 15, "The path '.' is a duplicate "
-                "of the one used on line 1.", self.get_recipe,
+        exc = self.assertInstructionParseError(3, 15,
+                "The path '.' is a duplicate of the one used on line 1.",
+                NEST_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest nest url .\n")
 
     def test_error_on_duplicate_path_with_another_nest(self):
-        exc = self.assertParseError(4, 16, "The path 'foo' is a duplicate "
-                "of the one used on line 3.", self.get_recipe,
+        exc = self.assertInstructionParseError(4, 16,
+                "The path 'foo' is a duplicate of the one used on line 3.",
+                NEST_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest nest url foo\n"
                 + "nest nest2 url foo\n")
 
     def test_duplicate_path_check_uses_normpath(self):
-        exc = self.assertParseError(3, 15, "The path 'foo/..' is a duplicate "
-                "of the one used on line 1.", self.get_recipe,
+        exc = self.assertInstructionParseError(3, 15,
+                "The path 'foo/..' is a duplicate of the one used on line 1.",
+                NEST_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest nest url foo/..\n")
 
     def test_error_absolute_path(self):
-        exc = self.assertParseError(3, 15, "Absolute paths are not allowed: "
-                "/etc/passwd", self.get_recipe, self.basic_header_and_branch
-                + "nest nest url /etc/passwd\n")
+        exc = self.assertInstructionParseError(3, 15,
+                "Absolute paths are not allowed: /etc/passwd",
+                NEST_INSTRUCTION, self.get_recipe,
+                self.basic_header_and_branch + "nest nest url /etc/passwd\n")
 
     def test_error_simple_parent_dir(self):
-        exc = self.assertParseError(3, 15, "Paths outside the current "
-                "directory are not allowed: ../foo", self.get_recipe,
+        exc = self.assertInstructionParseError(3, 15,
+                "Paths outside the current directory are not allowed: ../foo",
+                NEST_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest nest url ../foo\n")
 
     def test_error_complex_parent_dir(self):
-        exc = self.assertParseError(3, 15, "Paths outside the current "
-                "directory are not allowed: ./foo/../..", self.get_recipe,
+        exc = self.assertInstructionParseError(3, 15,
+                "Paths outside the current directory are not allowed:"
+                " ./foo/../..", NEST_INSTRUCTION, self.get_recipe,
                 self.basic_header_and_branch + "nest nest url ./foo/../..\n")
 
 
@@ -1064,8 +1105,8 @@
         nested_branch1 = RecipeBranch("nested1", "nested1_url",
                 revspec="tag:foo")
         base_branch.nest_branch(".", nested_branch1)
-        self.assertRaises(RecipeParseError, base_branch.get_recipe_text,
-                validate=True)
+        self.assertRaises(InstructionParseError,
+            base_branch.get_recipe_text, validate=True)
 
     def test_str_validates(self):
         base_branch = BaseRecipeBranch("base_url", "1", 0.1)
@@ -1073,7 +1114,7 @@
         nested_branch1 = RecipeBranch("nested1", "nested1_url",
                 revspec="tag:foo")
         base_branch.nest_branch(".", nested_branch1)
-        self.assertRaises(RecipeParseError, str, base_branch)
+        self.assertRaises(InstructionParseError, str, base_branch)
 
     def test_with_nest_part(self):
         base_branch = BaseRecipeBranch("base_url", "1", 0.1)
@@ -1087,6 +1128,29 @@
                 "nest-part nested1 nested1_url foo bar tag:foo\n",
                 manifest)
 
+    def test_with_nest_part_with_no_target_dir(self):
+        base_branch = BaseRecipeBranch("base_url", "1", 0.1)
+        base_branch.revid = "base_revid"
+        nested_branch1 = RecipeBranch("nested1", "nested1_url",
+                revspec="tag:foo")
+        base_branch.nest_part_branch(nested_branch1, "foo", None)
+        manifest = base_branch.get_recipe_text()
+        self.assertEqual("# bzr-builder format 0.1 deb-version 1\n"
+                "base_url revid:base_revid\n"
+                "nest-part nested1 nested1_url foo foo tag:foo\n",
+                manifest)
+
+    def test_with_nest_part_with_no_target_dir_no_revspec(self):
+        base_branch = BaseRecipeBranch("base_url", "1", 0.1)
+        base_branch.revid = "base_revid"
+        nested_branch1 = RecipeBranch("nested1", "nested1_url")
+        base_branch.nest_part_branch(nested_branch1, "foo", None)
+        manifest = base_branch.get_recipe_text()
+        self.assertEqual("# bzr-builder format 0.1 deb-version 1\n"
+                "base_url revid:base_revid\n"
+                "nest-part nested1 nested1_url foo\n",
+                manifest)
+
 
 class RecipeBranchTests(TestCaseInTempDir):