Improve 'duplicate dependency' error messages (#688)

Author: jkloetzke

pym/bob/errors.py

@@ -22,17 +22,14 @@ def __str__(self):
             ret = ret + "\n" + self.help
         return ret
 
-    def pushFrame(self, frame):
-        if not self.stack or (self.stack[0] != frame):
-            self.stack.insert(0, frame)
-
-    def setStack(self, stack):
-        if not self.stack: self.stack = stack[:]
-
 class ParseError(BobError):
     def __init__(self, slogan, *args, **kwargs):
         BobError.__init__(self, slogan, "Parse", "Processing stack", *args, **kwargs)
 
+    def pushFrame(self, frame):
+        if not self.stack or (self.stack[0] != frame):
+            self.stack.insert(0, frame)
+
     def setPath(self, path):
         self.stackSlogan = "Offending file"
         self.stack = [path]
@@ -41,6 +38,9 @@ class BuildError(BobError):
     def __init__(self, slogan, *args, **kwargs):
         BobError.__init__(self, slogan, "Build", "Failed package", *args, **kwargs)
 
+    def setStack(self, stack):
+        if not self.stack: self.stack = stack[:]
+
 
 class MultiBobError(BobError):
     def __init__(self, others):

pym/bob/input.py

@@ -1977,10 +1977,11 @@ def result(self):
 
 class DepTracker:
 
-    __slots__ = ('item', 'isNew', 'usedResult')
+    __slots__ = ('item', 'isNew', 'usedResult', 'depEntry')
 
-    def __init__(self, item):
+    def __init__(self, item, depEntry):
         self.item = item
+        self.depEntry = depEntry
         self.isNew = True
         self.usedResult = False
 
@@ -2084,9 +2085,10 @@ class Dependency(object):
         __slots__ = ('recipe', 'envOverride', 'provideGlobal', 'inherit',
                      'use', 'useEnv', 'useTools', 'useBuildResult', 'useDeps',
                      'useSandbox', 'condition', 'toolOverride', 'checkoutDep',
-                     'alias')
+                     'alias', 'origin')
 
-        def __init__(self, recipe, env, fwd, use, cond, tools, checkoutDep, inherit, alias):
+        def __init__(self, origin, recipe, env, fwd, use, cond, tools, checkoutDep, inherit, alias):
+            self.origin = origin
             self.recipe = recipe
             self.envOverride = env
             self.provideGlobal = fwd
@@ -2103,9 +2105,9 @@ def __init__(self, recipe, env, fwd, use, cond, tools, checkoutDep, inherit, ali
             self.alias = alias
 
         @staticmethod
-        def __parseEntry(dep, env, fwd, use, cond, tools, checkoutDep, inherit):
+        def __parseEntry(origin, dep, env, fwd, use, cond, tools, checkoutDep, inherit):
             if isinstance(dep, str):
-                return [ Recipe.Dependency(dep, env, fwd, use, cond, tools, checkoutDep,
+                return [ Recipe.Dependency(origin, dep, env, fwd, use, cond, tools, checkoutDep,
                                            inherit, None) ]
             else:
                 envOverride = dep.get("environment")
@@ -2126,23 +2128,24 @@ def __parseEntry(dep, env, fwd, use, cond, tools, checkoutDep, inherit):
                 name = dep.get("name")
                 if name:
                     if "depends" in dep:
-                        raise ParseError("A dependency must not use 'name' and 'depends' at the same time!")
-                    return [ Recipe.Dependency(name, env, fwd, use, cond, tools,
+                        raise ParseError("A dependency must not use 'name' and 'depends' at the same time!",
+                                         help=f"The offending entries 'name' attribute is '{name}'")
+                    return [ Recipe.Dependency(origin, name, env, fwd, use, cond, tools,
                                                checkoutDep, inherit, dep.get("alias")) ]
                 dependencies = dep.get("depends")
                 if dependencies is None:
                     raise ParseError("Either 'name' or 'depends' required for dependencies!")
-                return Recipe.Dependency.parseEntries(dependencies, env, fwd,
+                return Recipe.Dependency.parseEntries(origin, dependencies, env, fwd,
                                                       use, cond, tools,
                                                       checkoutDep, inherit)
 
         @staticmethod
-        def parseEntries(deps, env={}, fwd=False, use=["result", "deps"],
+        def parseEntries(origin, deps, env={}, fwd=False, use=["result", "deps"],
                          cond=None, tools={}, checkoutDep=False, inherit=True):
             """Returns an iterator yielding all dependencies as flat list"""
             # return flattened list of dependencies
             return chain.from_iterable(
-                Recipe.Dependency.__parseEntry(dep, env, fwd, use, cond, tools,
+                Recipe.Dependency.__parseEntry(origin, dep, env, fwd, use, cond, tools,
                                                checkoutDep, inherit)
                 for dep in deps )
 
@@ -2205,7 +2208,7 @@ def __init__(self, recipeSet, recipe, layer, sourceFile, baseDir, packageName, b
         self.__inherit = recipe.get("inherit", [])
         self.__anonBaseClass = anonBaseClass
         self.__defaultScriptLanguage = scriptLanguage
-        self.__deps = list(Recipe.Dependency.parseEntries(recipe.get("depends", [])))
+        self.__deps = list(Recipe.Dependency.parseEntries(self, recipe.get("depends", [])))
         self.__packageName = packageName
         self.__baseName = baseName
         self.__root = recipe.get("root")
@@ -2628,14 +2631,16 @@ def prepare(self, inputEnv, sandboxEnabled, inputStates, inputSandbox=None,
             # A dependency should be named only once. Hence we can
             # optimistically create the DepTracker object. If the dependency is
             # named more than one we make sure that it is the same variant.
-            depTrack = thisDeps.setdefault(p.getName(), DepTracker(depRef))
+            depTrack = thisDeps.setdefault(p.getName(), DepTracker(depRef, dep))
             if depTrack.prime():
                 directPackages.append(depRef)
             elif depCoreStep.variantId != depTrack.item.refGetDestination().variantId:
                 self.__raiseIncompatibleLocal(depCoreStep)
             else:
+                sources = " and ".join(set([dep.origin.getPrimarySource(), depTrack.depEntry.origin.getPrimarySource()]))
                 raise ParseError("Duplicate dependency '{}'. Each dependency must only be named once!"
-                                    .format(p.getName()))
+                                    .format(p.getName()),
+                                 help=f"The dependencies were declared in {sources}.")
 
             # Remember dependency diffs before changing them
             origDepDiffTools = thisDepDiffTools
@@ -2704,7 +2709,7 @@ def prepare(self, inputEnv, sandboxEnabled, inputStates, inputSandbox=None,
             name = depCoreStep.corePackage.getName()
             depTrack = thisDeps.get(name)
             if depTrack is None:
-                thisDeps[name] = depTrack = DepTracker(depRef)
+                thisDeps[name] = depTrack = DepTracker(depRef, None)
 
             if depTrack.prime():
                 indirectPackages.append(depRef)

test/unit/test_input_recipe.py

@@ -25,15 +25,15 @@ def cmpEntry(self, entry, name, env={}, fwd=False, use=["result", "deps"],
 
     def testSimpleList(self):
         deps = [ "a", "b" ]
-        res = list(Recipe.Dependency.parseEntries(deps))
+        res = list(Recipe.Dependency.parseEntries(MagicMock(), deps))
 
         self.assertEqual(len(res), 2)
         self.cmpEntry(res[0], "a")
         self.cmpEntry(res[1], "b")
 
     def testMixedList(self):
         deps = [ "a", { "name" : "b", "environment" : { "foo" : ("bar", None) }} ]
-        res = list(Recipe.Dependency.parseEntries(deps))
+        res = list(Recipe.Dependency.parseEntries(MagicMock(), deps))
 
         self.assertEqual(len(res), 2)
         self.cmpEntry(res[0], "a")
@@ -47,7 +47,7 @@ def testNestedList(self):
                 { "depends" : [ "c" ] }
             ]}
         ]
-        res = list(Recipe.Dependency.parseEntries(deps))
+        res = list(Recipe.Dependency.parseEntries(MagicMock(), deps))
 
         self.assertEqual(len(res), 3)
         self.cmpEntry(res[0], "a")
@@ -70,7 +70,7 @@ def testNestedEnv(self):
             },
             "e"
         ]
-        res = list(Recipe.Dependency.parseEntries(deps))
+        res = list(Recipe.Dependency.parseEntries(MagicMock(), deps))
 
         self.assertEqual(len(res), 5)
         self.cmpEntry(res[0], "a")
@@ -95,7 +95,7 @@ def testNestedIf(self):
             },
             "e"
         ]
-        res = list(Recipe.Dependency.parseEntries(deps))
+        res = list(Recipe.Dependency.parseEntries(MagicMock(), deps))
 
         self.assertEqual(len(res), 5)
         self.cmpEntry(res[0], "a")
@@ -120,7 +120,7 @@ def testNestedUse(self):
             },
             "e"
         ]
-        res = list(Recipe.Dependency.parseEntries(deps))
+        res = list(Recipe.Dependency.parseEntries(MagicMock(), deps))
 
         self.assertEqual(len(res), 5)
         self.cmpEntry(res[0], "a")
@@ -145,7 +145,7 @@ def testNestedFwd(self):
             },
             "e"
         ]
-        res = list(Recipe.Dependency.parseEntries(deps))
+        res = list(Recipe.Dependency.parseEntries(MagicMock(), deps))
 
         self.assertEqual(len(res), 5)
         self.cmpEntry(res[0], "a")
@@ -174,7 +174,7 @@ def testNestedCheckoutDep(self):
                 "checkoutDep" : True,
             }
         ]
-        res = list(Recipe.Dependency.parseEntries(deps))
+        res = list(Recipe.Dependency.parseEntries(MagicMock(), deps))
 
         self.assertEqual(len(res), 6)
         self.cmpEntry(res[0], "a")
@@ -184,6 +184,27 @@ def testNestedCheckoutDep(self):
         self.cmpEntry(res[4], "e")
         self.cmpEntry(res[5], "f", checkoutDep=True)
 
+    def testNameAndDepends(self):
+        """A dependency must not use 'name' and 'depends' at the same time"""
+        deps = [
+            {
+                "name" : "a",
+                "depends" : [],
+            }
+        ]
+        with self.assertRaises(ParseError):
+            list(Recipe.Dependency.parseEntries(MagicMock(), deps))
+
+    def testNeitherNameNorDepends(self):
+        """A dependency must use 'name' or 'depends'"""
+        deps = [
+            {
+                "if" : "a",
+            }
+        ]
+        with self.assertRaises(ParseError):
+            list(Recipe.Dependency.parseEntries(MagicMock(), deps))
+
 
 class RecipeCommon:
 

test/unit/test_input_recipeset.py

@@ -877,6 +877,32 @@ def testPackageDepends(self):
         self.assertEqual(p.getPackageStep().getArguments()[2].getPackage().getName(),
                          "lib2")
 
+    def testDuplicateDep(self):
+        """Dependencies must only be named once"""
+        self.writeRecipe("root", """\
+            root: True
+            depends: [a, a]
+            """)
+        self.writeRecipe("a", "")
+
+        packages = self.generate()
+        self.assertRaises(ParseError, packages.getRootPackage)
+
+    def testDuplicateDepWithClass(self):
+        """Dependencies must only be named once"""
+        self.writeRecipe("root", """\
+            root: True
+            inherit: [cls]
+            depends: [a]
+            """)
+        self.writeClass("cls", """\
+            depends: [a]
+            """)
+        self.writeRecipe("a", "")
+
+        packages = self.generate()
+        self.assertRaises(ParseError, packages.getRootPackage)
+
 class TestDependencyEnv(RecipesTmp, TestCase):
     """Tests related to "environment" block in dependencies"""