Merge lp:~sergiusens/snapcraft/yaml_init into lp:~snappy-dev/snapcraft/core

[Sergio Schvezov]

Simplifying snapcraft.yaml.Config.__init__

The complexity is no longer 22 but 5 for __init__

[Leo Arias (elopio)]

lgtm. Tests pass here.
I love that now not all the exceptions are sys exit. And thanks for using _ for variable names.

review: Approve

[lp:~sergiusens/snapcraft/yaml_init]

164. By Sergio Schvezov on 2015-09-07

Merged ubuntu-core into yaml_init.

165. By Sergio Schvezov on 2015-09-07

Merged ubuntu-core into yaml_init.

166. By Sergio Schvezov on 2015-09-08

Merged ubuntu-core into yaml_init.

[Michael Vogt (mvo)]

I did not do a full review (unless you want one) but what I see looks very good indeed.

[Sergio Schvezov (sergiusens)]

On Wed, Sep 9, 2015 at 10:49 AM, Michael Vogt <email address hidden>
wrote:

> I did not do a full review (unless you want one) but what I see looks very
> good indeed.
>

Thanks, an approve (aka full review) would be nice.

=== modified file 'snapcraft/cmds.py'
--- snapcraft/cmds.py	2015-08-31 17:42:31 +0000
+++ snapcraft/cmds.py	2015-09-08 12:03:45 +0000
@@ -52,7 +52,7 @@
 
 
 def shell(args):
-    config = snapcraft.yaml.Config()
+    config = _load_config()
     common.env = config.stage_env()
     userCommand = args.userCommand
     if not userCommand:
@@ -67,7 +67,7 @@
     if not os.path.exists(os.path.join(common.get_snapdir(), "meta")):
         arches = [snapcraft.common.get_arch(), ]
 
-        config = snapcraft.yaml.Config()
+        config = _load_config()
 
         # FIXME this should be done in a more contained manner
         common.env = config.snap_env()
@@ -243,7 +243,7 @@
         forceCommand = cmds[0]
         cmds = common.COMMAND_ORDER[0:common.COMMAND_ORDER.index(cmds[0]) + 1]
 
-    config = snapcraft.yaml.Config()
+    config = _load_config()
 
     # Install local packages that we need
     if config.build_tools:
@@ -282,3 +282,20 @@
 def _check_call(args, **kwargs):
     logger.info('Running: %s', ' '.join(shlex.quote(arg) for arg in args))
     return subprocess.check_call(args, **kwargs)
+
+
+def _load_config():
+    try:
+        return snapcraft.yaml.Config()
+    except snapcraft.yaml.SnapcraftYamlFileError as e:
+        logger.error(
+            'Could not find {}.  Are you sure you are in the right directory?\n'
+            'To start a new project, use \'snapcraft init\''.format(e.file))
+        sys.exit(1)
+    except snapcraft.yaml.SnapcraftSchemaError as e:
+        msg = "Issues while validating snapcraft.yaml: {}".format(e.message)
+        logger.error(msg)
+        sys.exit(1)
+    except snapcraft.yaml.SnapcraftLogicError as e:
+        logger.error('Issue detected while analyzing snapcraft.yaml: {}'.format(e.message))
+        sys.exit(1)

=== modified file 'snapcraft/tests/test_yaml.py'
--- snapcraft/tests/test_yaml.py	2015-09-08 12:03:45 +0000
+++ snapcraft/tests/test_yaml.py	2015-09-08 12:03:45 +0000
@@ -14,7 +14,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import jsonschema
 import logging
 import os
 import tempfile
@@ -71,14 +70,10 @@
         self.useFixture(fake_logger)
 
         # no snapcraft.yaml
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(snapcraft.yaml.SnapcraftYamlFileError) as raised:
             snapcraft.yaml.Config()
 
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        self.assertEqual(
-            "Could not find snapcraft.yaml.  Are you sure you're in the right directory?\n"
-            "To start a new project, use 'snapcraft init'\n",
-            fake_logger.output)
+        self.assertEqual(raised.exception.file, 'snapcraft.yaml')
 
     def test_config_loop(self):
         fake_logger = fixtures.FakeLogger(level=logging.ERROR)
@@ -99,11 +94,10 @@
     plugin: go
     after: [p1]
 """)
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(snapcraft.yaml.SnapcraftLogicError) as raised:
             snapcraft.yaml.Config()
 
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        self.assertEqual('Circular dependency chain!\n', fake_logger.output)
+        self.assertEqual(raised.exception.message, 'circular dependency chain found in parts definition')
 
     @unittest.mock.patch('snapcraft.yaml.Config.load_plugin')
     def test_invalid_yaml_missing_name(self, mock_loadPlugin):
@@ -122,13 +116,10 @@
     plugin: go
     stage-packages: [fswebcam]
 """)
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
             snapcraft.yaml.Config()
 
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        self.assertEqual(
-            'Issues while validating snapcraft.yaml: \'name\' is a required property\n',
-            fake_logger.output)
+        self.assertEqual(raised.exception.message, '\'name\' is a required property')
 
     @unittest.mock.patch('snapcraft.yaml.Config.load_plugin')
     def test_invalid_yaml_invalid_name_as_number(self, mock_loadPlugin):
@@ -147,13 +138,10 @@
     plugin: go
     stage-packages: [fswebcam]
 """)
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
             snapcraft.yaml.Config()
 
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        self.assertEqual(
-            'Issues while validating snapcraft.yaml: 1 is not of type \'string\'\n',
-            fake_logger.output)
+        self.assertEqual(raised.exception.message, '1 is not of type \'string\'')
 
     @unittest.mock.patch('snapcraft.yaml.Config.load_plugin')
     def test_invalid_yaml_invalid_name_chars(self, mock_loadPlugin):
@@ -172,13 +160,10 @@
     plugin: go
     stage-packages: [fswebcam]
 """)
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
             snapcraft.yaml.Config()
 
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        self.assertEqual(
-            'Issues while validating snapcraft.yaml: \'myapp@me_1.0\' does not match \'^[a-z0-9][a-z0-9+-]*$\'\n',
-            fake_logger.output)
+        self.assertEqual(raised.exception.message, '\'myapp@me_1.0\' does not match \'^[a-z0-9][a-z0-9+-]*$\'')
 
     @unittest.mock.patch('snapcraft.yaml.Config.load_plugin')
     def test_invalid_yaml_missing_description(self, mock_loadPlugin):
@@ -196,13 +181,10 @@
     plugin: go
     stage-packages: [fswebcam]
 """)
-        with self.assertRaises(SystemExit) as raised:
+        with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
             snapcraft.yaml.Config()
 
-        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
-        self.assertEqual(
-            'Issues while validating snapcraft.yaml: \'description\' is a required property\n',
-            fake_logger.output)
+        self.assertEqual(raised.exception.message, '\'description\' is a required property')
 
 
 class TestValidation(TestCase):
@@ -236,7 +218,7 @@
             with self.subTest(key=key):
                 del data[key]
 
-                with self.assertRaises(jsonschema.ValidationError) as raised:
+                with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
                     snapcraft.yaml._validate_snapcraft_yaml(data)
 
                 expected_message = '\'{}\' is a required property'.format(key)
@@ -254,7 +236,7 @@
             with self.subTest(key=name):
                 data['name'] = name
 
-                with self.assertRaises(jsonschema.ValidationError) as raised:
+                with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
                     snapcraft.yaml._validate_snapcraft_yaml(data)
 
                 expected_message = '\'{}\' does not match \'^[a-z0-9][a-z0-9+-]*$\''.format(name)
@@ -262,7 +244,7 @@
 
     def test_summary_too_long(self):
         self.data['summary'] = 'a' * 80
-        with self.assertRaises(jsonschema.ValidationError) as raised:
+        with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
             snapcraft.yaml._validate_snapcraft_yaml(self.data)
 
         expected_message = '\'{}\' is too long'.format(self.data['summary'])
@@ -289,7 +271,7 @@
             with self.subTest(key=t):
                 data['type'] = t
 
-                with self.assertRaises(jsonschema.ValidationError) as raised:
+                with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
                     snapcraft.yaml._validate_snapcraft_yaml(data)
 
                 expected_message = '\'{}\' is not one of [\'app\', \'framework\']'.format(t)
@@ -320,7 +302,7 @@
             }
         ]
 
-        with self.assertRaises(jsonschema.ValidationError) as raised:
+        with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
             snapcraft.yaml._validate_snapcraft_yaml(self.data)
 
         expected_message = '\'name\' is a required property'
@@ -331,18 +313,18 @@
         mock_the_open.side_effect = FileNotFoundError()
 
         with unittest.mock.patch('snapcraft.yaml.open', mock_the_open, create=True):
-            with self.assertRaises(snapcraft.yaml.SchemaNotFoundError) as raised:
+            with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
                 snapcraft.yaml._validate_snapcraft_yaml(self.data)
 
         expected_path = os.path.join(snapcraft.common.get_schemadir(), 'snapcraft.yaml')
         mock_the_open.assert_called_once_with(expected_path)
-        expected_message = 'Schema is missing, could not validate snapcraft.yaml, check installation'
+        expected_message = 'snapcraft validation file is missing from installation path'
         self.assertEqual(raised.exception.message, expected_message)
 
     def test_icon_missing(self):
         self.mock_path_exists.return_value = False
 
-        with self.assertRaises(jsonschema.ValidationError) as raised:
+        with self.assertRaises(snapcraft.yaml.SnapcraftSchemaError) as raised:
             snapcraft.yaml._validate_snapcraft_yaml(self.data)
 
         expected_message = '\'my-icon.png\' is not a \'icon-path\''

=== modified file 'snapcraft/yaml.py'
--- snapcraft/yaml.py	2015-08-27 22:04:07 +0000
+++ snapcraft/yaml.py	2015-09-08 12:03:45 +0000
@@ -15,7 +15,6 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import logging
-import sys
 
 import yaml
 import jsonschema
@@ -34,23 +33,34 @@
     return os.path.exists(instance)
 
 
-class SchemaNotFoundError(Exception):
-
-    def __init__(self, message):
-        self.message = message
-
-
-def _validate_snapcraft_yaml(snapcraft_yaml):
-    schema_file = os.path.abspath(os.path.join(common.get_schemadir(), 'snapcraft.yaml'))
-
-    try:
-        with open(schema_file) as fp:
-            schema = yaml.load(fp)
-    except FileNotFoundError:
-        raise SchemaNotFoundError('Schema is missing, could not validate snapcraft.yaml, check installation')
-
-    format_check = jsonschema.FormatChecker()
-    jsonschema.validate(snapcraft_yaml, schema, format_checker=format_check)
+class SnapcraftYamlFileError(Exception):
+
+    @property
+    def file(self):
+        return self._file
+
+    def __init__(self, yaml_file):
+        self._file = yaml_file
+
+
+class SnapcraftLogicError(Exception):
+
+    @property
+    def message(self):
+        return self._message
+
+    def __init__(self, message):
+        self._message = message
+
+
+class SnapcraftSchemaError(Exception):
+
+    @property
+    def message(self):
+        return self._message
+
+    def __init__(self, message):
+        self._message = message
 
 
 class Config:
@@ -58,25 +68,10 @@
     def __init__(self):
         self.build_tools = []
         self.all_parts = []
-        afterRequests = {}
-
-        try:
-            with open("snapcraft.yaml", 'r') as fp:
-                self.data = yaml.load(fp)
-        except FileNotFoundError:
-            logger.error("Could not find snapcraft.yaml.  Are you sure you're in the right directory?\nTo start a new project, use 'snapcraft init'")
-            sys.exit(1)
-
-        # Make sure the loaded snapcraft yaml follows the schema
-        try:
-            _validate_snapcraft_yaml(self.data)
-        except SchemaNotFoundError as e:
-            logger.error(e.message)
-            sys.exit(1)
-        except jsonschema.ValidationError as e:
-            msg = "Issues while validating snapcraft.yaml: {}".format(e.message)
-            logger.error(msg)
-            sys.exit(1)
+        after_requests = {}
+
+        self.data = _snapcraft_yaml_load()
+        _validate_snapcraft_yaml(self.data)
 
         self.build_tools = self.data.get('build-tools', [])
 
@@ -88,45 +83,52 @@
                 del properties["plugin"]
 
             if "after" in properties:
-                afterRequests[part_name] = properties["after"]
+                after_requests[part_name] = properties["after"]
                 del properties["after"]
 
             # TODO: support 'filter' or 'blacklist' field to filter what gets put in snap/
 
             self.load_plugin(part_name, plugin_name, properties)
 
-        # Grab all required dependencies, if not already specified
-        newParts = self.all_parts.copy()
-        while newParts:
-            part = newParts.pop(0)
+        self._load_missing_part_plugins()
+        self._compute_part_dependencies(after_requests)
+        self.all_parts = self._sort_parts()
+
+    def _load_missing_part_plugins(self):
+        new_parts = self.all_parts.copy()
+        while new_parts:
+            part = new_parts.pop(0)
             requires = part.config.get('requires', [])
-            for requiredPart in requires:
-                alreadyPresent = False
+            for required_part in requires:
+                present = False
                 for p in self.all_parts:
-                    if requiredPart in p.names():
-                        alreadyPresent = True
+                    if required_part in p.names():
+                        present = True
                         break
-                if not alreadyPresent:
-                    newParts.append(self.load_plugin(requiredPart, requiredPart, {}))
-
-        # Gather lists of dependencies
+                if not present:
+                    new_parts.append(self.load_plugin(required_part, required_part, {}))
+
+    def _compute_part_dependencies(self, after_requests):
+        '''Gather the lists of dependencies and adds to all_parts.'''
+
         for part in self.all_parts:
-            depNames = part.config.get('requires', []) + afterRequests.get(part.names()[0], [])
-            for dep in depNames:
-                foundIt = False
+            dep_names = part.config.get('requires', []) + after_requests.get(part.names()[0], [])
+            for dep in dep_names:
+                found = False
                 for i in range(len(self.all_parts)):
                     if dep in self.all_parts[i].names():
                         part.deps.append(self.all_parts[i])
-                        foundIt = True
+                        found = True
                         break
-                if not foundIt:
-                    logger.error("Could not find part name %s", dep)
-                    sys.exit(1)
-
-        # Now sort them (this is super inefficient, but easy-ish to follow)
-        sortedParts = []
+                if not found:
+                    raise SnapcraftLogicError('part name missing {}'.format(dep))
+
+    def _sort_parts(self):
+        '''Performs an inneficient but easy to follow sorting of parts.'''
+        sorted_parts = []
+
         while self.all_parts:
-            topPart = None
+            top_part = None
             for part in self.all_parts:
                 mentioned = False
                 for other in self.all_parts:
@@ -134,14 +136,14 @@
                         mentioned = True
                         break
                 if not mentioned:
-                    topPart = part
+                    top_part = part
                     break
-            if not topPart:
-                logger.error("Circular dependency chain!")
-                sys.exit(1)
-            sortedParts = [topPart] + sortedParts
-            self.all_parts.remove(topPart)
-        self.all_parts = sortedParts
+            if not top_part:
+                raise SnapcraftLogicError('circular dependency chain found in parts definition')
+            sorted_parts = [top_part] + sorted_parts
+            self.all_parts.remove(top_part)
+
+        return sorted_parts
 
     def load_plugin(self, part_name, plugin_name, properties, load_code=True):
         part = snapcraft.plugin.load_plugin(part_name, plugin_name, properties, load_code=load_code)
@@ -196,3 +198,25 @@
             env += part.env(root)
 
         return env
+
+
+def _validate_snapcraft_yaml(snapcraft_yaml):
+    schema_file = os.path.abspath(os.path.join(common.get_schemadir(), 'snapcraft.yaml'))
+
+    try:
+        with open(schema_file) as fp:
+            schema = yaml.load(fp)
+            format_check = jsonschema.FormatChecker()
+            jsonschema.validate(snapcraft_yaml, schema, format_checker=format_check)
+    except FileNotFoundError:
+        raise SnapcraftSchemaError('snapcraft validation file is missing from installation path')
+    except jsonschema.ValidationError as e:
+        raise SnapcraftSchemaError(e.message)
+
+
+def _snapcraft_yaml_load(yaml_file='snapcraft.yaml'):
+    try:
+        with open(yaml_file) as fp:
+            return yaml.load(fp)
+    except FileNotFoundError:
+        raise SnapcraftYamlFileError(yaml_file)