Repository: osbuild/osbuild
Pull request #552 (2020-12-16 18:26:09)

Format abstraction
Separate the external from the internal representation. This is being done by a having converting between the external manifest description and internal data structures in a format specific module. For now there is only one such module `v1`, which corresponds to manifest schema version `1`. It contains most format code that was distributed over osbuild.

This is the first step towards prepare DAG (direct acyclic pipelines).


Patch:
diff --git a/osbuild/__init__.py b/osbuild/__init__.py
index fed324ef..19b2934f 100644
--- a/osbuild/__init__.py
+++ b/osbuild/__init__.py
@@ -8,14 +8,12 @@ The utility module `osbuild.util` provides access to common functionality
 independent of osbuild but used across the osbuild codebase.
 """
 
-
-from .pipeline import Assembler, load, load_build, Pipeline, Stage
+from .pipeline import Assembler, Manifest, Pipeline, Stage
 
 
 __all__ = [
     "Assembler",
-    "load",
-    "load_build",
+    "Manifest",
     "Pipeline",
     "Stage",
 ]
diff --git a/osbuild/formats/__init__.py b/osbuild/formats/__init__.py
new file mode 100644
index 00000000..8eeb5e19
--- /dev/null
+++ b/osbuild/formats/__init__.py
@@ -0,0 +1,3 @@
+"""
+Concrete representation of manifest descriptions
+"""
diff --git a/osbuild/formats/v1.py b/osbuild/formats/v1.py
new file mode 100644
index 00000000..859438a1
--- /dev/null
+++ b/osbuild/formats/v1.py
@@ -0,0 +1,136 @@
+# Version 1 of the manifest description
+
+from typing import Dict
+from osbuild.meta import Index, ValidationResult
+from ..pipeline import Manifest, Pipeline, detect_host_runner
+
+
+def describe(manifest: Manifest, *, with_id=False) -> Dict:
+    """Create the manifest description for the pipeline"""
+    def describe_stage(stage):
+        description = {"name": stage.name}
+        if stage.options:
+            description["options"] = stage.options
+        if with_id:
+            description["id"] = stage.id
+        return description
+
+    def describe_pipeline(pipeline: Pipeline) -> Dict:
+        description = {}
+        if pipeline.build:
+            build = pipeline.build
+            description["build"] = {
+                "pipeline": describe_pipeline(build),
+                "runner": pipeline.runner
+            }
+
+        if pipeline.stages:
+            stages = [describe_stage(s) for s in pipeline.stages]
+            description["stages"] = stages
+
+        if pipeline.assembler:
+            assembler = describe_stage(pipeline.assembler)
+            description["assembler"] = assembler
+        return description
+
+    description = {
+        "pipeline": describe_pipeline(manifest.pipeline)
+    }
+
+    if manifest.sources:
+        description["sources"] = manifest.sources
+
+    return description
+
+
+def load_build(description: Dict, sources_options: Dict):
+    pipeline = description.get("pipeline")
+    if pipeline:
+        build_pipeline = load_pipeline(pipeline, sources_options)
+    else:
+        build_pipeline = None
+
+    return build_pipeline, description["runner"]
+
+
+def load_pipeline(description: Dict, sources_options: Dict) -> Pipeline:
+    build = description.get("build")
+    if build:
+        build_pipeline, runner = load_build(build, sources_options)
+    else:
+        build_pipeline, runner = None, detect_host_runner()
+
+    pipeline = Pipeline(runner, build_pipeline)
+
+    for s in description.get("stages", []):
+        pipeline.add_stage(s["name"], sources_options, s.get("options", {}))
+
+    a = description.get("assembler")
+    if a:
+        pipeline.set_assembler(a["name"], a.get("options", {}))
+
+    return pipeline
+
+
+def load(description: Dict) -> Manifest:
+    """Load a manifest description"""
+
+    pipeline = description.get("pipeline", {})
+    sources = description.get("sources", {})
+
+    pipeline = load_pipeline(pipeline, sources)
+    manifest = Manifest(pipeline, sources)
+
+    return manifest
+
+
+def validate(manifest: Dict, index: Index) -> ValidationResult:
+    """Validate a OSBuild manifest
+
+    This function will validate a OSBuild manifest, including
+    all its stages and assembler and build manifests. It will
+    try to validate as much as possible and not stop on errors.
+    The result is a `ValidationResult` object that can be used
+    to check the overall validation status and iterate all the
+    individual validation errors.
+    """
+
+    schema = index.get_schema("Manifest")
+    result = schema.validate(manifest)
+
+    # main pipeline
+    pipeline = manifest.get("pipeline", {})
+
+    # recursively validate the build pipeline  as a "normal"
+    # pipeline in order to validate its stages and assembler
+    # options; for this it is being re-parented in a new plain
+    # {"pipeline": ...} dictionary. NB: Any nested structural
+    # errors might be detected twice, but de-duplicated by the
+    # `ValidationResult.merge` call
+    build = pipeline.get("build", {}).get("pipeline")
+    if build:
+        res = validate({"pipeline": build}, index=index)
+        result.merge(res, path=["pipeline", "build"])
+
+    stages = pipeline.get("stages", [])
+    for i, stage in enumerate(stages):
+        name = stage["name"]
+        schema = index.get_schema("Stage", name)
+        res = schema.validate(stage)
+        result.merge(res, path=["pipeline", "stages", i])
+
+    asm = pipeline.get("assembler", {})
+    if asm:
+        name = asm["name"]
+        schema = index.get_schema("Assembler", name)
+        res = schema.validate(asm)
+        result.merge(res, path=["pipeline", "assembler"])
+
+    # sources
+    sources = manifest.get("sources", {})
+    for name, source in sources.items():
+        schema = index.get_schema("Source", name)
+        res = schema.validate(source)
+        result.merge(res, path=["sources", name])
+
+    return result
diff --git a/osbuild/main_cli.py b/osbuild/main_cli.py
index fbaa4481..75cd968c 100644
--- a/osbuild/main_cli.py
+++ b/osbuild/main_cli.py
@@ -14,6 +14,7 @@ import sys
 import osbuild
 import osbuild.meta
 import osbuild.monitor
+from osbuild.formats import v1 as fmt
 
 
 RESET = "\033[0m"
@@ -22,33 +23,6 @@ RED = "\033[31m"
 GREEN = "\033[32m"
 
 
-def mark_checkpoints(pipeline, checkpoints):
-    points = set(checkpoints)
-
-    def mark_stage(stage):
-        c = stage.id
-        if c in points:
-            stage.checkpoint = True
-            points.remove(c)
-
-    def mark_assembler(assembler):
-        c = assembler.id
-        if c in points:
-            assembler.checkpoint = True
-            points.remove(c)
-
-    def mark_pipeline(pl):
-        for stage in pl.stages:
-            mark_stage(stage)
-        if pl.assembler:
-            mark_assembler(pl.assembler)
-        if pl.build:
-            mark_pipeline(pl.build)
-
-    mark_pipeline(pipeline)
-    return points
-
-
 def parse_manifest(path):
     if path == "-":
         manifest = json.load(sys.stdin)
@@ -102,11 +76,11 @@ def parse_arguments(sys_argv):
 # pylint: disable=too-many-branches
 def osbuild_cli():
     args = parse_arguments(sys.argv)
-    manifest = parse_manifest(args.manifest_path)
+    desc = parse_manifest(args.manifest_path)
 
     # first thing after parsing is validation of the input
     index = osbuild.meta.Index(args.libdir)
-    res = osbuild.meta.validate(manifest, index)
+    res = fmt.validate(desc, index)
     if not res:
         if args.json or args.inspect:
             json.dump(res.as_dict(), sys.stdout)
@@ -115,13 +89,11 @@ def osbuild_cli():
             show_validation(res, args.manifest_path)
         return 2
 
-    pipeline = manifest.get("pipeline", {})
-    sources_options = manifest.get("sources", {})
-
-    pipeline = osbuild.load(pipeline, sources_options)
+    manifest = fmt.load(desc)
+    pipeline = manifest.pipeline
 
     if args.checkpoint:
-        missed = mark_checkpoints(pipeline, args.checkpoint)
+        missed = manifest.mark_checkpoints(args.checkpoint)
         if missed:
             for checkpoint in missed:
                 print(f"Checkpoint {BOLD}{checkpoint}{RESET} not found!")
@@ -129,9 +101,7 @@ def osbuild_cli():
             return 1
 
     if args.inspect:
-        result = {"pipeline": pipeline.description(with_id=True)}
-        if sources_options:
-            result["sources"] = sources_options
+        result = fmt.describe(manifest, with_id=True)
         json.dump(result, sys.stdout)
         sys.stdout.write("\n")
         return 0
@@ -144,7 +114,7 @@ def osbuild_cli():
     monitor = osbuild.monitor.make(monitor_name, sys.stdout.fileno())
 
     try:
-        r = pipeline.run(
+        r = manifest.build(
             args.store,
             monitor,
             args.libdir,
diff --git a/osbuild/meta.py b/osbuild/meta.py
index 81e37e73..691337b3 100644
--- a/osbuild/meta.py
+++ b/osbuild/meta.py
@@ -25,7 +25,7 @@ import copy
 import os
 import json
 from collections import deque
-from typing import Dict, Iterable, List, Optional
+from typing import Iterable, List, Optional
 
 import jsonschema
 
@@ -429,55 +429,3 @@ class Index:
         self._schemata[(klass, name)] = schema
 
         return schema
-
-
-def validate(manifest: Dict, index: Index) -> ValidationResult:
-    """Validate a OSBuild manifest
-
-    This function will validate a OSBuild manifest, including
-    all its stages and assembler and build manifests. It will
-    try to validate as much as possible and not stop on errors.
-    The result is a `ValidationResult` object that can be used
-    to check the overall validation status and iterate all the
-    individual validation errors.
-    """
-
-    schema = index.get_schema("Manifest")
-    result = schema.validate(manifest)
-
-    # main pipeline
-    pipeline = manifest.get("pipeline", {})
-
-    # recursively validate the build pipeline  as a "normal"
-    # pipeline in order to validate its stages and assembler
-    # options; for this it is being re-parented in a new plain
-    # {"pipeline": ...} dictionary. NB: Any nested structural
-    # errors might be detected twice, but de-duplicated by the
-    # `ValidationResult.merge` call
-    build = pipeline.get("build", {}).get("pipeline")
-    if build:
-        res = validate({"pipeline": build}, index=index)
-        result.merge(res, path=["pipeline", "build"])
-
-    stages = pipeline.get("stages", [])
-    for i, stage in enumerate(stages):
-        name = stage["name"]
-        schema = index.get_schema("Stage", name)
-        res = schema.validate(stage)
-        result.merge(res, path=["pipeline", "stages", i])
-
-    asm = pipeline.get("assembler", {})
-    if asm:
-        name = asm["name"]
-        schema = index.get_schema("Assembler", name)
-        res = schema.validate(asm)
-        result.merge(res, path=["pipeline", "assembler"])
-
-    # sources
-    sources = manifest.get("sources", {})
-    for name, source in sources.items():
-        schema = index.get_schema("Source", name)
-        res = schema.validate(source)
-        result.merge(res, path=["sources", name])
-
-    return result
diff --git a/osbuild/pipeline.py b/osbuild/pipeline.py
index 9773fc1b..39914084 100644
--- a/osbuild/pipeline.py
+++ b/osbuild/pipeline.py
@@ -3,6 +3,7 @@ import hashlib
 import json
 import os
 import tempfile
+from typing import Dict
 
 from .api import API
 from . import buildroot
@@ -49,14 +50,6 @@ class Stage:
         m.update(json.dumps(self.options, sort_keys=True).encode())
         return m.hexdigest()
 
-    def description(self, *, with_id=False):
-        description = {"name": self.name}
-        if self.options:
-            description["options"] = self.options
-        if with_id:
-            description["id"] = self.id
-        return description
-
     def run(self,
             tree,
             runner,
@@ -113,14 +106,6 @@ class Assembler:
         m.update(json.dumps(self.options, sort_keys=True).encode())
         return m.hexdigest()
 
-    def description(self, *, with_id=False):
-        description = {"name": self.name}
-        if self.options:
-            description["options"] = self.options
-        if with_id:
-            description["id"] = self.id
-        return description
-
     def run(self, tree, runner, build_tree, monitor, libdir, output_dir, var="/var/tmp"):
         with buildroot.BuildRoot(build_tree, runner, libdir, var=var) as build_root:
 
@@ -181,22 +166,6 @@ class Pipeline:
         build = self.build.tree_id if self.build else None
         self.assembler = Assembler(name, build, self.tree_id, options or {})
 
-    def description(self, *, with_id=False):
-        description = {}
-        if self.build:
-            description["build"] = {
-                "pipeline": self.build.description(with_id=with_id),
-                "runner": self.runner
-            }
-        if self.stages:
-            stages = [s.description(with_id=with_id) for s in self.stages]
-            description["stages"] = stages
-        if self.assembler:
-            assembler = self.assembler.description(with_id=with_id)
-            description["assembler"] = assembler
-
-        return description
-
     def build_stages(self, object_store, monitor, libdir):
         results = {"success": True}
 
@@ -359,36 +328,45 @@ class Pipeline:
         return results
 
 
-def detect_host_runner():
-    """Use os-release(5) to detect the runner for the host"""
-    osname = osrelease.describe_os(*osrelease.DEFAULT_PATHS)
-    return "org.osbuild." + osname
+class Manifest:
+    """Representation of a pipeline and its sources"""
+
+    def __init__(self, pipeline: Pipeline, source_options: Dict):
+        self.pipeline = pipeline
+        self.sources = source_options
 
+    def build(self, store, monitor, libdir, output_directory):
+        return self.pipeline.run(store, monitor, libdir, output_directory)
 
-def load_build(description, sources_options):
-    pipeline = description.get("pipeline")
-    if pipeline:
-        build_pipeline = load(pipeline, sources_options)
-    else:
-        build_pipeline = None
 
-    return build_pipeline, description["runner"]
+    def mark_checkpoints(self, checkpoints):
+        points = set(checkpoints)
 
+        def mark_stage(stage):
+            c = stage.id
+            if c in points:
+                stage.checkpoint = True
+                points.remove(c)
 
-def load(description, sources_options):
-    build = description.get("build")
-    if build:
-        build_pipeline, runner = load_build(build, sources_options)
-    else:
-        build_pipeline, runner = None, detect_host_runner()
+        def mark_assembler(assembler):
+            c = assembler.id
+            if c in points:
+                assembler.checkpoint = True
+                points.remove(c)
 
-    pipeline = Pipeline(runner, build_pipeline)
+        def mark_pipeline(pl):
+            for stage in pl.stages:
+                mark_stage(stage)
+            if pl.assembler:
+                mark_assembler(pl.assembler)
+            if pl.build:
+                mark_pipeline(pl.build)
 
-    for s in description.get("stages", []):
-        pipeline.add_stage(s["name"], sources_options, s.get("options", {}))
+        mark_pipeline(self.pipeline)
+        return points
 
-    a = description.get("assembler")
-    if a:
-        pipeline.set_assembler(a["name"], a.get("options", {}))
 
-    return pipeline
+def detect_host_runner():
+    """Use os-release(5) to detect the runner for the host"""
+    osname = osrelease.describe_os(*osrelease.DEFAULT_PATHS)
+    return "org.osbuild." + osname


Test patch:
diff --git a/test/mod/test_osbuild.py b/test/mod/test_osbuild.py
index f99279c1..5c1c96d4 100644
--- a/test/mod/test_osbuild.py
+++ b/test/mod/test_osbuild.py
@@ -11,6 +11,7 @@ import unittest
 
 import osbuild
 import osbuild.meta
+from osbuild.formats import v1 as fmt
 from osbuild.monitor import NullMonitor
 from osbuild.pipeline import detect_host_runner
 from .. import test
@@ -29,20 +30,10 @@ class TestDescriptions(unittest.TestCase):
             {"build": None}
         ]
         for pipeline in cases:
+            manifest = {"pipeline": pipeline}
             with self.subTest(pipeline):
-                self.assertEqual(osbuild.load(pipeline, {}).description(), {})
-
-    def test_stage(self):
-        name = "org.osbuild.test"
-        options = {"one": 1}
-        cases = [
-            (osbuild.Stage(name, {}, None, None, {}), {"name": name}),
-            (osbuild.Stage(name, {}, None, None, None), {"name": name}),
-            (osbuild.Stage(name, {}, None, None, options), {"name": name, "options": options}),
-        ]
-        for stage, description in cases:
-            with self.subTest(description):
-                self.assertEqual(stage.description(), description)
+                desc = fmt.describe(fmt.load(manifest))
+                self.assertEqual(desc["pipeline"], {})
 
     @unittest.skipUnless(test.TestBase.can_bind_mount(), "root-only")
     def test_stage_run(self):
@@ -65,18 +56,6 @@ class TestDescriptions(unittest.TestCase):
         self.assertEqual(res.success, True)
         self.assertEqual(res.id, stage.id)
 
-    def test_assembler(self):
-        name = "org.osbuild.test"
-        options = {"one": 1}
-        cases = [
-            (osbuild.Assembler(name, None, None, {}), {"name": name}),
-            (osbuild.Assembler(name, None, None, None), {"name": name}),
-            (osbuild.Assembler(name, None, None, options), {"name": name, "options": options}),
-        ]
-        for assembler, description in cases:
-            with self.subTest(description):
-                self.assertEqual(assembler.description(), description)
-
     @unittest.skipUnless(test.TestBase.can_bind_mount(), "root-only")
     def test_assembler_run(self):
         asm = osbuild.Assembler("org.osbuild.noop", None, None, {})
@@ -107,26 +86,30 @@ class TestDescriptions(unittest.TestCase):
         pipeline.add_stage("org.osbuild.test", {}, {"one": 2})
         pipeline.set_assembler("org.osbuild.test")
 
-        self.assertEqual(pipeline.description(), {
-            "build": {
-                "pipeline": {
-                    "stages": [
-                        {
-                            "name": "org.osbuild.test",
-                            "options": {"one": 1}
-                        }
-                    ]
+        manifest = osbuild.Manifest(pipeline, {})
+
+        self.assertEqual(fmt.describe(manifest), {
+            "pipeline": {
+                "build": {
+                    "pipeline": {
+                        "stages": [
+                            {
+                                "name": "org.osbuild.test",
+                                "options": {"one": 1}
+                            }
+                        ]
+                    },
+                    "runner": "org.osbuild.test"
                 },
-                "runner": "org.osbuild.test"
-            },
-            "stages": [
-                {
-                    "name": "org.osbuild.test",
-                    "options": {"one": 2}
+                "stages": [
+                    {
+                        "name": "org.osbuild.test",
+                        "options": {"one": 2}
+                    }
+                ],
+                "assembler": {
+                    "name": "org.osbuild.test"
                 }
-            ],
-            "assembler": {
-                "name": "org.osbuild.test"
             }
         })
 
@@ -174,7 +157,7 @@ class TestDescriptions(unittest.TestCase):
         index = osbuild.meta.Index(os.curdir)
 
         # an empty manifest is OK
-        res = osbuild.meta.validate({}, index)
+        res = fmt.validate({}, index)
         self.assertEqual(res.valid, True)
 
         # something totally invalid (by Ondřej Budai)
@@ -187,7 +170,7 @@ class TestDescriptions(unittest.TestCase):
             }
         }
 
-        res = osbuild.meta.validate(totally_invalid, index)
+        res = fmt.validate(totally_invalid, index)
         self.assertEqual(res.valid, False)
         # The top-level 'osbuild' is an additional property
         self.assertEqual(len(res), 1)
@@ -201,7 +184,7 @@ class TestDescriptions(unittest.TestCase):
             }
         }
 
-        res = osbuild.meta.validate(no_runner, index)
+        res = fmt.validate(no_runner, index)
         self.assertEqual(res.valid, False)
         self.assertEqual(len(res), 1)  # missing runner
         lst = res[".pipeline.build"]
@@ -227,7 +210,7 @@ class TestDescriptions(unittest.TestCase):
             }
         }
 
-        res = osbuild.meta.validate(no_runner_extra, index)
+        res = fmt.validate(no_runner_extra, index)
         self.assertEqual(res.valid, False)
         self.assertEqual(len(res), 3)
         lst = res[".pipeline.build.pipeline"]
@@ -251,7 +234,7 @@ class TestDescriptions(unittest.TestCase):
             }
         }
 
-        res = osbuild.meta.validate(stage_check, index)
+        res = fmt.validate(stage_check, index)
         self.assertEqual(res.valid, False)
         self.assertEqual(len(res), 2)
         lst = res[".pipeline.stages[0].options"]
@@ -270,7 +253,7 @@ class TestDescriptions(unittest.TestCase):
             }
         }
 
-        res = osbuild.meta.validate(assembler_check, index)
+        res = fmt.validate(assembler_check, index)
         self.assertEqual(res.valid, False)
         self.assertEqual(len(res), 2)
         lst = res[".pipeline.assembler.options"]
diff --git a/test/test.py b/test/test.py
index eacd5f51..7fbe4d85 100644
--- a/test/test.py
+++ b/test/test.py
@@ -11,7 +11,7 @@ import sys
 import tempfile
 import unittest
 
-import osbuild
+from osbuild.formats import v1 as fmt
 from osbuild.util import linux
 
 
@@ -353,11 +353,9 @@ class OSBuild(contextlib.AbstractContextManager):
         """
 
         manifest_json = json.loads(manifest_data)
-        manifest_pipeline = manifest_json.get("pipeline", {})
-        manifest_sources = manifest_json.get("sources", {})
 
-        manifest_parsed = osbuild.load(manifest_pipeline, manifest_sources)
-        return manifest_parsed.tree_id
+        manifest = fmt.load(manifest_json)
+        return manifest.pipeline.tree_id
 
     @contextlib.contextmanager
     def map_object(self, obj):