Add get_elements; preserve backwards compat for expand_dependencies

expand_dependencies() was a public interface so we should try and
preserve backwards compat. However, since the interface is really
broken, add a new exported function "get_elements" that instack can
switch to.  This returns the canonical list of elements without
duplicates, and gives the path to each element too.

This highlighted that the unit tests were really a bit wrong.  They're
testing inner functions when we have an "API" in the get_elements()
function.  Convert all unit-tests to use this function instead.  Since
this is a library call, convert the sys.exit() calls to raised
exceptions.

Refactor the variable output into a separate function so we can do a
sanity check on it.

The added flake8 ignores are for the "over-indented for ... indent"
which happens a lot with these new longer lines.  Most other projects
ignore them.

This is an alternative proposal to
I15609389c18adf3017220fc94552514d195b323a

Change-Id: If97bcd21e45de1b5ed91023fdc441a4617051a6b

diskimage_builder/element_dependencies.py

@@ -26,6 +26,22 @@ import diskimage_builder.logging_config
 logger = logging.getLogger(__name__)
 
 
+class MissingElementException(Exception):
+    pass
+
+
+class AlreadyProvidedException(Exception):
+    pass
+
+
+class MissingOSException(Exception):
+    pass
+
+
+class InvalidElementDir(Exception):
+    pass
+
+
 class Element(object):
     """An element"""
     def __init__(self, name, path):
@@ -63,19 +79,25 @@ class Element(object):
 
         logger.debug("New element : %s", str(self))
 
+    def __eq__(self, other):
+        return self.name == other.name
+
+    def __repr__(self):
+        return self.name
+
     def __str__(self):
         return '%s p:<%s> d:<%s>' % (self.name,
                                      ','.join(self.provides),
                                      ','.join(self.depends))
 
 
-def get_elements_dir():
+def _get_elements_dir():
     if not os.environ.get('ELEMENTS_PATH'):
         raise Exception("$ELEMENTS_PATH must be set.")
     return os.environ['ELEMENTS_PATH']
 
 
-def expand_dependencies(user_elements, all_elements):
+def _expand_element_dependencies(user_elements, all_elements):
     """Expand user requested elements using element-deps files.
 
     Arguments:
@@ -97,8 +119,7 @@ def expand_dependencies(user_elements, all_elements):
         if element in provided:
             continue
         elif element not in all_elements:
-            logger.error("Element '%s' not found", element)
+            raise MissingElementException("Element '%s' not found" % element)
-            sys.exit(1)
 
         element_obj = all_elements[element]
 
@@ -112,11 +133,6 @@ def expand_dependencies(user_elements, all_elements):
         check_queue.extend(element_deps - (final_elements | provided))
         final_elements.update(element_deps)
 
-    if "operating-system" not in provided:
-        logger.error(
-            "Please include an operating system element.")
-        sys.exit(-1)
-
     conflicts = set(user_elements) & provided
     if conflicts:
         logger.error(
@@ -124,12 +140,16 @@ def expand_dependencies(user_elements, all_elements):
         for element in conflicts:
             logger.error("%s : already provided by %s" %
                          (element, provided_by[element]))
-        sys.exit(-1)
+        raise AlreadyProvidedException()
 
-    return final_elements - provided
+    if "operating-system" not in provided:
+        raise MissingOSException("Please include an operating system element")
+
+    out = final_elements - provided
+    return([all_elements[element] for element in out])
 
 
-def find_all_elements(paths=None):
+def _find_all_elements(paths=None):
     """Build a dictionary Element() objects
 
     Walk ELEMENTS_PATH and find all elements.  Make an Element object
@@ -138,10 +158,9 @@ def find_all_elements(paths=None):
     those seen later.
 
     :param paths: A list of paths to find elements in.  If None will
-                  use ELEMENTS_PATH
+                  use ELEMENTS_PATH from environment
 
     :return: a dictionary of all elements
-
     """
 
     all_elements = {}
@@ -151,11 +170,16 @@ def find_all_elements(paths=None):
     #  ELEMENTS_PATH=path1:path2:path3
     # we want the elements in "path1" to override "path3"
     if not paths:
-        paths = reversed(get_elements_dir().split(':'))
+        paths = reversed(_get_elements_dir().split(':'))
+    else:
+        paths = reversed(paths.split(':'))
+
+    logger.debug("ELEMENTS_PATH is: %s" % paths)
+
     for path in paths:
         if not os.path.isdir(path):
-            logger.error("ELEMENT_PATH entry '%s' is not a directory", path)
+            raise InvalidElementDir("ELEMENTS_PATH entry '%s' "
-            sys.exit(1)
+                                    "is not a directory " % path)
 
         # In words : make a list of directories in "path".  Since an
         # element is a directory, this is our list of elements.
@@ -179,6 +203,93 @@ def find_all_elements(paths=None):
     return all_elements
 
 
+def _get_elements(elements, paths=None):
+    """Return the canonical list of Element objects
+
+    This function returns Element objects.  For exernal calls, use
+    get_elements which returns a simple tuple & list.
+
+    :param elements: user specified list of elements
+    :param paths: element paths, default to environment
+
+    """
+    all_elements = _find_all_elements(paths)
+    return _expand_element_dependencies(elements, all_elements)
+
+
+def get_elements(elements, paths=None):
+    """Return the canonical list of elements with their dependencies
+
+    .. note::
+
+       You probably do not want to use this!  Elements that require
+       access to the list of all other elements should generally use
+       the environment variables exported by disk-image-create below.
+
+    :param elements: user specified elements
+    :param paths: Alternative ELEMENTS_PATH; default is to use from env
+
+    :return: A de-duplicated list of tuples [(element, path),
+             (element, path) ...] with all elements and their
+             dependents, including any transitive dependencies.
+    """
+
+    elements = _get_elements(elements, paths)
+    return [(element.name, element.path) for element in elements]
+
+
+def expand_dependencies(user_elements, element_dirs):
+    """Deprecated method for expanding element dependencies.
+
+    .. warning::
+
+       DO NOT USE THIS FUNCTION.  For compatability reasons, this
+       function does not provide paths to the returned elements.  This
+       means the caller must process override rules if two elements
+       with the same name appear in element_dirs
+
+    :param user_elements: iterable enumerating the elements a user requested
+    :param elements_dir: The ELEMENTS_PATH to process
+
+    :return: a set contatining user_elements and all dependent
+             elements including any transitive dependencies.
+    """
+    logger.warning("expand_dependencies() deprecated, use get_elements")
+    elements = _get_elements(user_elements, element_dirs)
+    return set([element.name for element in elements])
+
+
+def _output_env_vars(elements):
+    """Output eval-able bash strings for IMAGE_ELEMENT vars
+
+    :param elements: list of Element objects to represent
+    """
+    # first the "legacy" environment variable that just lists the
+    # elements
+    print("export IMAGE_ELEMENT='%s'" %
+          ' '.join([element.name for element in elements]))
+
+    # Then YAML
+    output = {}
+    for element in elements:
+        output[element.name] = element.path
+    print("export IMAGE_ELEMENT_YAML='%s'" % yaml.safe_dump(output))
+
+    # Then bash array.  Unfortunately, bash can't export array
+    # variables.  So we take a compromise and produce an exported
+    # function that outputs the string to re-create the array.
+    # You can then simply do
+    #  eval declare -A element_array=$(get_image_element_array)
+    # and you have it.
+    output = ""
+    for element in elements:
+        output += '[%s]=%s ' % (element.name, element.path)
+    print("function get_image_element_array {\n"
+          "  echo \"%s\"\n"
+          "};\n"
+          "export -f get_image_element_array;" % output)
+
+
 def main():
     diskimage_builder.logging_config.setup()
 
@@ -192,36 +303,13 @@ def main():
 
     args = parser.parse_args(sys.argv[1:])
 
-    all_elements = find_all_elements()
+    elements = _get_elements(args.elements)
-
-    elements = expand_dependencies(args.elements, all_elements)
 
     if args.env:
-        # first the "legacy" environment variable that just lists the
+        _output_env_vars(elements)
-        # elements
-        print("export IMAGE_ELEMENT='%s'" % ' '.join(elements))
-
-        # Then YAML
-        output = {}
-        for element in elements:
-            output[element] = all_elements[element].path
-        print("export IMAGE_ELEMENT_YAML='%s'" % yaml.safe_dump(output))
-
-        # Then bash array.  Unfortunately, bash can't export array
-        # variables.  So we take a compromise and produce an exported
-        # function that outputs the string to re-create the array.
-        # You can then simply do
-        #  eval declare -A element_array=$(get_image_element_array)
-        # and you have it.
-        output = ""
-        for element in elements:
-            output += '[%s]=%s ' % (element, all_elements[element].path)
-        print("function get_image_element_array {\n"
-              "  echo \"%s\"\n"
-              "};\n"
-              "export -f get_image_element_array;" % output)
     else:
-        print(' '.join(elements))
+        # deprecated compatability output; doesn't include paths.
+        print(' '.join([element.name for element in elements]))
 
     return 0
 

diskimage_builder/tests/test_elementdeps.py

@@ -20,11 +20,14 @@ import testtools
 
 from diskimage_builder import element_dependencies
 
+logger = logging.getLogger(__name__)
+
 data_dir = os.path.abspath(
     os.path.join(os.path.dirname(__file__), 'test-elements'))
 
 
 def _populate_element(element_dir, element_name, element_deps=[], provides=[]):
+        logger.debug("Populate %s <%s>" % (element_name, element_dir))
         element_home = os.path.join(element_dir, element_name)
         os.mkdir(element_home)
         deps_path = os.path.join(element_home, 'element-deps')
@@ -85,75 +88,96 @@ class TestElementDeps(testtools.TestCase):
         _populate_element(self.element_dir, 'override_element', [])
         _populate_element(self.element_override_dir, 'override_element', [])
 
-        self.all_elements = element_dependencies.find_all_elements(
+        # This simulates $ELEMENTS_PATH
-            [self.element_dir, self.element_override_dir])
+        self.element_dirs = "%s:%s" % (self.element_override_dir,
+                                       self.element_dir)
+
+    # helper to return an (element, path) tuple from the standard dir
+    def _e(self, element):
+            return (element, os.path.join(self.element_dir, element))
+
+    # helper to return an (element, path) tuple from the override dir
+    def _eo(self, element):
+            return (element, os.path.join(self.element_override_dir, element))
 
     def test_non_transitive_deps(self):
-        result = element_dependencies.expand_dependencies(
+        result = element_dependencies.get_elements(['requires-foo'],
-            ['requires-foo'], self.all_elements)
+                                                   self.element_dirs)
-        self.assertEqual(set(['requires-foo', 'foo']), result)
+        self.assertItemsEqual([self._e('foo'), self._e('requires-foo')],
+                              result)
 
     def test_missing_deps(self):
-        self.assertRaises(SystemExit,
+        e = self.assertRaises(element_dependencies.MissingElementException,
-                          element_dependencies.expand_dependencies, ['fake'],
+                              element_dependencies.get_elements,
-                          self.all_elements)
+                              ['fake'],
-        self.assertIn("Element 'fake' not found",
+                              self.element_dirs)
-                      self.log_fixture.output)
+        self.assertIn("Element 'fake' not found", str(e))
+
+    def test_invalid_element_dir(self):
+        e = self.assertRaises(element_dependencies.InvalidElementDir,
+                              element_dependencies.get_elements,
+                              ['fake'],
+                              self.element_dirs + ":/not/a/dir")
+        self.assertIn("ELEMENTS_PATH entry '/not/a/dir' is not a directory",
+                      str(e))
 
     def test_transitive_deps(self):
-        result = element_dependencies.expand_dependencies(
+        result = element_dependencies.get_elements(
-            ['requires-requires-foo'], self.all_elements)
+                ['requires-requires-foo'], self.element_dirs)
-        self.assertEqual(set(['requires-requires-foo',
+
-                              'requires-foo',
+        self.assertItemsEqual([self._e('requires-requires-foo'),
-                              'foo']), result)
+                               self._e('requires-foo'),
+                               self._e('foo')], result)
 
     def test_no_deps(self):
-        result = element_dependencies.expand_dependencies(
+        result = element_dependencies.get_elements(['foo'], self.element_dirs)
-            ['foo'], self.all_elements)
+        self.assertEqual([self._e('foo')], result)
-        self.assertEqual(set(['foo']), result)
 
     def test_self(self):
-        result = element_dependencies.expand_dependencies(
+        result = element_dependencies.get_elements(['self', 'foo'],
-            ['self', 'foo'], self.all_elements)
+                                                   self.element_dirs)
-        self.assertEqual(set(['self', 'foo']), result)
+        self.assertItemsEqual([self._e('self'),
+                               self._e('foo')], result)
 
     def test_circular(self):
-        result = element_dependencies.expand_dependencies(
+        result = element_dependencies.get_elements(['circular1'],
-            ['circular1'], self.all_elements)
+                                                   self.element_dirs)
-        self.assertEqual(set(['circular1', 'circular2']), result)
+        self.assertItemsEqual([self._e('circular1'),
+                               self._e('circular2')], result)
 
     def test_provide(self):
-        result = element_dependencies.expand_dependencies(
+        result = element_dependencies.get_elements(
-            ['provides_virtual', 'requires_virtual'],
+                ['provides_virtual', 'requires_virtual'],
-            self.all_elements)
+                self.element_dirs)
-        self.assertEqual(set(['requires_virtual', 'provides_virtual']), result)
+        self.assertItemsEqual([self._e('requires_virtual'),
+                               self._e('provides_virtual')], result)
 
     def test_provide_conflict(self):
-        self.assertRaises(SystemExit,
+        self.assertRaises(element_dependencies.AlreadyProvidedException,
-                          element_dependencies.expand_dependencies,
+                          element_dependencies.get_elements,
-                          ['virtual', 'provides_virtual'],
+                                  ['virtual', 'provides_virtual'],
-                          self.element_dir)
+                                  self.element_dirs)
 
     def test_provide_virtual_ordering(self):
-        result = element_dependencies.expand_dependencies(
+        result = element_dependencies.get_elements(
-            ['requires_new_virtual', 'provides_new_virtual'],
+                ['requires_new_virtual', 'provides_new_virtual'],
-            self.all_elements)
+                self.element_dirs)
-        self.assertEqual(set(['requires_new_virtual', 'provides_new_virtual']),
+        self.assertItemsEqual(
-                         result)
+                [self._e('requires_new_virtual'),
+                 self._e('provides_new_virtual')], result)
 
     def test_no_os_element(self):
-        self.assertRaises(SystemExit,
+        self.assertRaises(element_dependencies.MissingOSException,
-                          element_dependencies.expand_dependencies,
+                          element_dependencies.get_elements,
                           ['provides_virtual'],
-                          self.all_elements)
+                          self.element_dirs)
-        self.assertIn("Please include an operating system element",
-                      self.log_fixture.output)
 
     def test_duplicated_os_passed_as_element(self):
-        self.assertRaises(SystemExit,
+        self.assertRaises(
-                          element_dependencies.expand_dependencies,
+                element_dependencies.AlreadyProvidedException,
-                          ['circular1', 'operating-system'],
+                element_dependencies.get_elements,
-                          self.all_elements)
+                ['circular1', 'operating-system'],
+                self.element_dirs)
         # ensure we get the error message about what's providing the
         # conflicting package
         self.assertIn("operating-system : already provided by ['circular1']",
@@ -162,18 +186,33 @@ class TestElementDeps(testtools.TestCase):
     def test_element_override(self):
         # make sure we picked up "override_element" from the override dir,
         # not the base dir
-        self.assertTrue('override_element' in self.all_elements)
+        result = element_dependencies.get_elements(['override_element', 'foo'],
-        self.assertEqual(os.path.join(self.element_override_dir,
+                                                   self.element_dirs)
-                                      'override_element'),
+        self.assertItemsEqual([self._e('foo'),
-                         self.all_elements['override_element'].path)
+                               self._eo('override_element')],
+                              result)
+
+    def test_expand_dependencies_deprecated(self):
+        # test the deprecated expand_dependencies call
+        result = element_dependencies.expand_dependencies(
+                ['foo', 'requires-foo'], self.element_dirs)
+        self.assertItemsEqual(['foo', 'requires-foo'], result)
+
+    def test_output_sanity(self):
+        # very basic output sanity test
+        elements = element_dependencies._get_elements(['foo', 'requires-foo'],
+                                                      self.element_dirs)
+        element_dependencies._output_env_vars(elements)
 
 
 class TestElements(testtools.TestCase):
     def test_depends_on_env(self):
         self.useFixture(
             fixtures.EnvironmentVariable('ELEMENTS_PATH', '/foo/bar'))
-        self.assertEqual('/foo/bar', element_dependencies.get_elements_dir())
+        self.assertEqual('/foo/bar',
+                         element_dependencies._get_elements_dir())
 
     def test_env_not_set(self):
         self.useFixture(fixtures.EnvironmentVariable('ELEMENTS_PATH', ''))
-        self.assertRaises(Exception, element_dependencies.get_elements_dir, ())
+        self.assertRaises(Exception,
+                          element_dependencies._get_elements_dir, ())

tox.ini

@@ -36,5 +36,5 @@ commands = python setup.py build_sphinx
 commands = sphinx-build -a -W -E -d releasenotes/build/doctrees -b html releasenotes/source releasenotes/build/html
 
 [flake8]
-ignore = E125,H202,H302,H803
+ignore = E126,E127,E125,H202,H302,H803
 exclude =  .venv,.git,.tox,dist,doc,*lib/python*,*egg,build,conf.py