getdeps: throw exceptions when found duplicate manifests and name mismatches

Summary:
Throws an exception when:

* The name specified in the manifest mismatches the filename
* Duplicated manifest -- with the sub-directory support it is now able to have multiple manifest files with the same name

Reviewed By: chadaustin

Differential Revision: D17438460

fbshipit-source-id: ac7ad0b701beb15f0e91bb05cd1ec8fe378ad5b6

build/fbcode_builder/getdeps/load.py

@@ -20,9 +20,9 @@ from .manifest import ManifestParser
 class Loader(object):
     """ The loader allows our tests to patch the load operation """
 
-    def _list_manifests(self, manifests_dir):
+    def _list_manifests(self, build_opts):
         """ Returns a generator that iterates all the available manifests """
-        for (path, _, files) in os.walk(manifests_dir):
+        for (path, _, files) in os.walk(build_opts.manifests_dir):
             for name in files:
                 # skip hidden files
                 if name.startswith("."):
@@ -30,12 +30,15 @@ class Loader(object):
 
                 yield os.path.join(path, name)
 
+    def _load_manifest(self, path):
+        return ManifestParser(path)
+
     def load_project(self, build_opts, project_name):
         if "/" in project_name or "\\" in project_name:
             # Assume this is a path already
             return ManifestParser(project_name)
 
-        for manifest in self._list_manifests(build_opts.manifests_dir):
+        for manifest in self._list_manifests(build_opts):
             if os.path.basename(manifest) == project_name:
                 return ManifestParser(manifest)
 
@@ -44,21 +47,26 @@ class Loader(object):
     def load_all(self, build_opts):
         manifests_by_name = {}
 
-        for manifest in self._list_manifests(build_opts.manifests_dir):
-            m = ManifestParser(manifest)
+        for manifest in self._list_manifests(build_opts):
+            m = self._load_manifest(manifest)
+
+            if m.name in manifests_by_name:
+                raise Exception("found duplicate manifest '%s'" % m.name)
+
             manifests_by_name[m.name] = m
 
         return manifests_by_name
 
 
 class ResourceLoader(Loader):
-    def __init__(self, namespace):
+    def __init__(self, namespace, manifests_dir):
         self.namespace = namespace
+        self.manifests_dir = manifests_dir
 
-    def _list_manifests(self):
+    def _list_manifests(self, _build_opts):
         import pkg_resources
 
-        dirs = ["manifests"]
+        dirs = [self.manifests_dir]
 
         while dirs:
             current = dirs.pop(0)
@@ -77,7 +85,7 @@ class ResourceLoader(Loader):
 
         raise ManifestNotFound(project_name)
 
-    def _load_resource_manifest(self, path):
+    def _load_manifest(self, path):
         import pkg_resources
 
         contents = pkg_resources.resource_string(self.namespace, path).decode("utf8")
@@ -87,21 +95,13 @@ class ResourceLoader(Loader):
         project_name = self._find_manifest(project_name)
         return self._load_resource_manifest(project_name)
 
-    def load_all(self, build_opts):
-        manifests_by_name = {}
-        for path in self._list_manifests():
-            m = self._load_resource_manifest(path)
-            manifests_by_name[m.name] = m
-
-        return manifests_by_name
-
 
 LOADER = Loader()
 
 
-def patch_loader(namespace):
+def patch_loader(namespace, manifests_dir="manifests"):
     global LOADER
-    LOADER = ResourceLoader(namespace)
+    LOADER = ResourceLoader(namespace, manifests_dir)
 
 
 def load_project(build_opts, project_name):

build/fbcode_builder/getdeps/manifest.py

@@ -9,6 +9,7 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 
 import io
+import os
 
 from .builder import (
     AutoconfBuilder,
@@ -198,6 +199,12 @@ class ManifestParser(object):
         self.shipit_project = self.get("manifest", "shipit_project")
         self.shipit_fbcode_builder = self.get("manifest", "shipit_fbcode_builder")
 
+        if self.name != os.path.basename(file_name):
+            raise Exception(
+                "filename of the manifest '%s' does not match the manifest name '%s'"
+                % (file_name, self.name)
+            )
+
     def get(self, section, key, defval=None, ctx=None):
         ctx = ctx or {}
 

build/fbcode_builder/getdeps/test/fixtures/duplicate/foo

+[manifest]
+name = foo

build/fbcode_builder/getdeps/test/fixtures/duplicate/subdir/foo

+[manifest]
+name = foo

build/fbcode_builder/getdeps/test/manifest_test.py

@@ -11,8 +11,6 @@ from __future__ import absolute_import, division, print_function, unicode_litera
 import sys
 import unittest
 
-import pkg_resources
-
 from ..load import load_all_manifests, patch_loader
 from ..manifest import ManifestParser
 
@@ -41,10 +39,10 @@ class ManifestTest(unittest.TestCase):
             "test",
             """
 [manifest]
-name = foo
+name = test
 """,
         )
-        self.assertEqual(p.name, "foo")
+        self.assertEqual(p.name, "test")
         self.assertEqual(p.fbsource_path, None)
 
     def test_minimal_with_fbsource_path(self):
@@ -52,11 +50,11 @@ name = foo
             "test",
             """
 [manifest]
-name = foo
+name = test
 fbsource_path = fbcode/wat
 """,
         )
-        self.assertEqual(p.name, "foo")
+        self.assertEqual(p.name, "test")
         self.assertEqual(p.fbsource_path, "fbcode/wat")
 
     def test_unknown_field(self):
@@ -71,7 +69,7 @@ fbsource_path = fbcode/wat
                 "test",
                 """
 [manifest]
-name = foo
+name = test
 invalid.field = woot
 """,
             )
@@ -84,7 +82,7 @@ invalid.field = woot
                 "test",
                 """
 [manifest]
-name = foo
+name = test
 
 [invalid.section]
 foo = bar
@@ -104,7 +102,7 @@ foo = bar
                 "test",
                 """
 [manifest]
-name = foo
+name = test
 
 [dependencies]
 foo = bar
@@ -124,7 +122,7 @@ foo = bar
                 "test",
                 """
 [manifest]
-name = foo
+name = test
 
 [dependencies.=]
 """,
@@ -135,7 +133,7 @@ name = foo
             "test",
             """
 [manifest]
-name = foo
+name = test
 
 [dependencies]
 a
@@ -159,7 +157,7 @@ foo
             "test",
             """
 [manifest]
-name = foo
+name = test
 
 [autoconf.args]
 --prefix=/foo
@@ -175,7 +173,7 @@ name = foo
             "test",
             """
 [manifest]
-name = foo
+name = test
 
 [cmake.defines]
 foo = bar
@@ -193,7 +191,7 @@ foo = baz
             "test",
             """
 [manifest]
-name = foo
+name = test
 
 [cmake.defines.test=on]
 foo = baz
@@ -213,6 +211,25 @@ foo = bar
         manifests = load_all_manifests(None)
         self.assertNotEqual(0, len(manifests), msg="parsed some number of manifests")
 
+    def test_mismatch_name(self):
+        with self.assertRaisesRegex(
+            Exception,
+            "filename of the manifest 'foo' does not match the manifest name 'bar'",
+        ):
+            ManifestParser(
+                "foo",
+                """
+[manifest]
+name = bar
+""",
+            )
+
+    def test_duplicate_manifest(self):
+        patch_loader(__name__, "fixtures/duplicate")
+
+        with self.assertRaisesRegex(Exception, "found duplicate manifest 'foo'"):
+            load_all_manifests(None)
+
     if sys.version_info < (3, 2):
 
         def assertRaisesRegex(self, *args, **kwargs):