init: Validate whitespace in comps groups

If a package name contains leading or trailing whitespace, it will
eventually lead to issues: pungi will try to include that group, but
since it does not exist, the packages will not make it in.

The root cause is hard to find. Better report an error immediately.

Signed-off-by: Lubomír Sedlář <lsedlar@redhat.com>

pungi/phases/init.py

@@ -42,7 +42,8 @@ class InitPhase(PhaseBase):
     def run(self):
         if self.compose.has_comps:
             # write global comps and arch comps, create comps repos
-            write_global_comps(self.compose)
+            global_comps = write_global_comps(self.compose)
+            validate_comps(global_comps)
             for arch in self.compose.get_arches():
                 write_arch_comps(self.compose, arch)
                 create_comps_repo(self.compose, arch, None)
@@ -88,6 +89,8 @@ def write_global_comps(compose):
         shutil.copy2(os.path.join(tmp_dir, comps_name), comps_file_global)
         shutil.rmtree(tmp_dir)
 
+    return comps_file_global
+
 
 def write_arch_comps(compose, arch):
     comps_file_arch = compose.paths.work.comps(arch=arch)
@@ -203,3 +206,9 @@ def validate_module_defaults(path):
         raise RuntimeError(
             "There are duplicated module defaults:\n%s" % "\n".join(errors)
         )
+
+
+def validate_comps(path):
+    """Check that there are whitespace issues in comps."""
+    wrapper = CompsWrapper(path)
+    wrapper.validate()

pungi/wrappers/comps.py

@@ -49,6 +49,10 @@ TYPE_MAPPING = collections.OrderedDict([
 ])
 
 
+class CompsValidationError(ValueError):
+    pass
+
+
 class CompsFilter(object):
     """
     Processor for extended comps file. This class treats the input as just an
@@ -230,6 +234,25 @@ class CompsWrapper(object):
                 return [pkg.name for pkg in grp.packages]
         raise KeyError('No such group %r' % group)
 
+    def validate(self):
+        """Check that no package name contains whitespace, and raise a
+        RuntimeError if there is a problem.
+        """
+        errors = []
+        for group in self.get_comps_groups():
+            for pkg in self.get_packages(group):
+                stripped_pkg = pkg.strip()
+                if pkg != stripped_pkg:
+                    errors.append(
+                        "Package name %s in group '%s' contains leading or trailing whitespace"
+                        % (stripped_pkg, group)
+                    )
+
+        if errors:
+            raise CompsValidationError(
+                "Comps file contains errors:\n%s" % "\n".join(errors)
+            )
+
     def write_comps(self, comps_obj=None, target_file=None):
         if not comps_obj:
             comps_obj = self.generate_comps()

tests/fixtures/comps-ws.xml

@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE comps PUBLIC "-//Red Hat, Inc.//DTD Comps info//EN" "comps.dtd">
+<comps>
+  <group>
+    <id>core</id>
+    <name>Core</name>
+    <description>Smallest possible installation</description>
+    <default>true</default>
+    <uservisible>false</uservisible>
+    <packagelist>
+      <packagereq type="mandatory"> foo </packagereq>
+      <packagereq type="mandatory">bar </packagereq>
+      <packagereq type="mandatory"> baz</packagereq>
+    </packagelist>
+  </group>
+</comps>

tests/test_comps_wrapper.py

@@ -12,7 +12,7 @@ import sys
 
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
 
-from pungi.wrappers.comps import CompsWrapper, CompsFilter
+from pungi.wrappers.comps import CompsWrapper, CompsFilter, CompsValidationError
 from tests.helpers import FIXTURE_DIR
 
 COMPS_FILE = os.path.join(FIXTURE_DIR, 'comps.xml')
@@ -20,6 +20,7 @@ COMPS_FORMATTED_FILE = os.path.join(FIXTURE_DIR, 'comps-formatted.xml')
 COMPS_GROUP_FILE = os.path.join(FIXTURE_DIR, 'comps-group.xml')
 COMPS_ENVIRONMENT_FILE = os.path.join(FIXTURE_DIR, 'comps-env.xml')
 COMPS_FILE_WITH_TYPO = os.path.join(FIXTURE_DIR, 'comps-typo.xml')
+COMPS_FILE_WITH_WHITESPACE = os.path.join(FIXTURE_DIR, 'comps-ws.xml')
 
 
 class CompsWrapperTest(unittest.TestCase):
@@ -98,6 +99,28 @@ class CompsWrapperTest(unittest.TestCase):
             'Package dummy-bash in group core has unknown type',
             str(ctx.exception))
 
+    def test_validate_correct(self):
+        comps = CompsWrapper(COMPS_FILE)
+        comps.validate()
+
+    def test_validate_with_whitespace(self):
+        comps = CompsWrapper(COMPS_FILE_WITH_WHITESPACE)
+        with self.assertRaises(CompsValidationError) as ctx:
+            comps.validate()
+
+        self.assertIn(
+            "Package name foo in group 'core' contains leading or trailing whitespace",
+            str(ctx.exception),
+        )
+        self.assertIn(
+            "Package name bar in group 'core' contains leading or trailing whitespace",
+            str(ctx.exception),
+        )
+        self.assertIn(
+            "Package name baz in group 'core' contains leading or trailing whitespace",
+            str(ctx.exception),
+        )
+
 
 COMPS_IN_FILE = os.path.join(FIXTURE_DIR, 'comps.xml.in')
 

tests/test_initphase.py

@@ -12,9 +12,10 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
 
 from pungi import Modulemd
 from pungi.phases import init
-from tests.helpers import DummyCompose, PungiTestCase, touch
+from tests.helpers import DummyCompose, PungiTestCase, touch, mk_boom
 
 
+@mock.patch("pungi.phases.init.validate_comps")
 @mock.patch("pungi.phases.init.validate_module_defaults")
 @mock.patch("pungi.phases.init.write_module_defaults")
 @mock.patch("pungi.phases.init.write_global_comps")
@@ -33,6 +34,7 @@ class TestInitPhase(PungiTestCase):
         write_global,
         write_defaults,
         validate_defaults,
+        validate_comps,
     ):
         compose = DummyCompose(self.topdir, {})
         compose.has_comps = True
@@ -72,6 +74,7 @@ class TestInitPhase(PungiTestCase):
         write_global,
         write_defaults,
         validate_defaults,
+        validate_comps,
     ):
         compose = DummyCompose(self.topdir, {})
         compose.has_comps = True
@@ -82,6 +85,9 @@ class TestInitPhase(PungiTestCase):
         phase.run()
 
         self.assertEqual(write_global.mock_calls, [mock.call(compose)])
+        self.assertEqual(
+            validate_comps.call_args_list, [mock.call(write_global.return_value)]
+        )
         self.assertEqual(write_prepopulate.mock_calls, [mock.call(compose)])
         self.assertItemsEqual(write_arch.mock_calls,
                               [mock.call(compose, 'x86_64'), mock.call(compose, 'amd64')])
@@ -110,6 +116,7 @@ class TestInitPhase(PungiTestCase):
         write_global,
         write_defaults,
         validate_defaults,
+        validate_comps,
     ):
         compose = DummyCompose(self.topdir, {})
         compose.has_comps = False
@@ -118,6 +125,7 @@ class TestInitPhase(PungiTestCase):
         phase.run()
 
         self.assertItemsEqual(write_global.mock_calls, [])
+        self.assertEqual(validate_comps.call_args_list, [])
         self.assertItemsEqual(write_prepopulate.mock_calls, [mock.call(compose)])
         self.assertItemsEqual(write_arch.mock_calls, [])
         self.assertItemsEqual(create_comps.mock_calls, [])
@@ -134,6 +142,7 @@ class TestInitPhase(PungiTestCase):
         write_global,
         write_defaults,
         validate_defaults,
+        validate_comps,
     ):
         compose = DummyCompose(self.topdir, {})
         compose.has_comps = False
@@ -142,6 +151,7 @@ class TestInitPhase(PungiTestCase):
         phase.run()
 
         self.assertItemsEqual(write_global.mock_calls, [])
+        self.assertEqual(validate_comps.call_args_list, [])
         self.assertItemsEqual(write_prepopulate.mock_calls, [mock.call(compose)])
         self.assertItemsEqual(write_arch.mock_calls, [])
         self.assertItemsEqual(create_comps.mock_calls, [])
@@ -517,5 +527,25 @@ class TestValidateModuleDefaults(PungiTestCase):
         init.validate_module_defaults(self.topdir)
 
 
+@mock.patch("pungi.phases.init.CompsWrapper")
+class TestValidateComps(unittest.TestCase):
+    def test_ok(self, CompsWrapper):
+        init.validate_comps("/path")
+
+        self.assertEqual(
+            CompsWrapper.mock_calls, [mock.call("/path"), mock.call().validate()]
+        )
+
+    def test_fail(self, CompsWrapper):
+        CompsWrapper.return_value.validate.side_effect = mk_boom()
+
+        with self.assertRaises(Exception):
+            init.validate_comps("/path")
+
+        self.assertEqual(
+            CompsWrapper.mock_calls, [mock.call("/path"), mock.call().validate()]
+        )
+
+
 if __name__ == "__main__":
     unittest.main()