From b2d9cd65df2a109db6431f85aca42f1a9fa1d790 Mon Sep 17 00:00:00 2001
From: Rafael Sarmiento <rafael.sarmiento@cscs.ch>
Date: Fri, 17 May 2019 14:58:35 +0200
Subject: [PATCH 1/4] add containerplatform

---
 reframe/core/containerplatform.py   | 97 +++++++++++++++++++++++++++++
 reframe/core/exceptions.py          |  4 ++
 unittests/test_containerplatform.py | 52 ++++++++++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 reframe/core/containerplatform.py
 create mode 100644 unittests/test_containerplatform.py

diff --git a/reframe/core/containerplatform.py b/reframe/core/containerplatform.py
new file mode 100644
index 0000000000..38d12e3fa7
--- /dev/null
+++ b/reframe/core/containerplatform.py
@@ -0,0 +1,97 @@
+import abc
+
+import reframe.core.fields as fields
+import reframe.utility.typecheck as typ
+from reframe.core.exceptions import ContainerPlatformError
+
+
+class ContainerPlatform:
+    """The abstract base class of any container platform.
+
+    Concrete container platforms inherit from this class and must override the
+    :func:`emit_prepare_cmds` and :func:`emit_launch_cmds` abstract functions.
+    """
+
+    registry = fields.TypedField('registry', str, type(None))
+    image = fields.TypedField('image', str, type(None))
+    requires_mpi = fields.TypedField('requires_mpi', bool)
+    commands = fields.TypedField('commands', typ.List[str], type(None))
+    mount_points = fields.TypedField('mount_points',
+                                     typ.List[typ.Tuple[str, str]], type(None))
+
+    def __init__(self):
+        self.registry = None
+        self.image = None
+        self.requires_mpi = False
+        self.commands = None
+        self.mount_points  = []
+
+    @abc.abstractmethod
+    def emit_prepare_cmds(self):
+        """xxx."""
+
+    @abc.abstractmethod
+    def emit_launch_cmds(self):
+        """Returns the command for running with this container platform.
+
+        :raises: `ContainerPlatformError` in case of missing mandatory
+        fields.
+
+        .. note:
+            This method is relevant only to developers of new container
+            plataforms.
+        """
+
+
+class Docker(ContainerPlatform):
+    """An implementation of the container platform to run containers with
+    Docker.
+    """
+    # def __init__(self):
+    #     super().__init__()
+
+    def emit_prepare_cmds(self):
+        pass
+
+    def emit_launch_commands(self):
+        docker_opts = []
+
+        if self.image is None:
+            raise ContainerPlatformError('Please, specify the name of'
+                                         'the image')
+
+        if self.commands is None:
+            raise ContainerPlatformError('Please, specify a command')
+
+        if self.registry:
+            self.image = '%s/%s' % (self.registry, self.image)
+
+        self.mount_points.append(('$PWD', '/stagedir'))
+        for mp in self.mount_points:
+            docker_opts.append('-v %s:%s' % mp)
+
+        cmd_base = "docker run %s %s bash -c 'cd /stagedir; %s'"
+
+        return [cmd_base % (' '.join(docker_opts), self.image,
+                            '; '.join(self.commands))]
+
+
+class ContainerPlatformField(fields.TypedField):
+    """A field representing a build system.
+
+    You may either assign an instance of :class:`ContainerPlatform:` or a
+    string representing the name of the concrete class of a build system.
+    """
+
+    def __init__(self, fieldname, *other_types):
+        super().__init__(fieldname, ContainerPlatform, *other_types)
+
+    def __set__(self, obj, value):
+        if isinstance(value, str):
+            try:
+                value = globals()[value]()
+            except KeyError:
+                raise ValueError(
+                    'unknown container platform: %s' % value) from None
+
+        super().__set__(obj, value)
diff --git a/reframe/core/exceptions.py b/reframe/core/exceptions.py
index 6375a08c7f..829d994c9c 100644
--- a/reframe/core/exceptions.py
+++ b/reframe/core/exceptions.py
@@ -108,6 +108,10 @@ class BuildSystemError(ReframeError):
     """Raised when a build system is not configured properly."""
 
 
+class ContainerPlatformError(ReframeError):
+    """Raised when a container platform is not configured properly."""
+
+
 class BuildError(ReframeError):
     """Raised when a build fails."""
 
diff --git a/unittests/test_containerplatform.py b/unittests/test_containerplatform.py
new file mode 100644
index 0000000000..f9a682929b
--- /dev/null
+++ b/unittests/test_containerplatform.py
@@ -0,0 +1,52 @@
+import abc
+import unittest
+
+import reframe.core.containerplatform as cp
+from reframe.core.exceptions import ContainerPlatformError
+
+
+class _ContainerPlatformTest:
+    @abc.abstractmethod
+    def create_container_platform(self):
+        pass
+
+    def setUp(self):
+        self.container_platform = self.create_container_platform()
+
+
+class TestDocker(_ContainerPlatformTest, unittest.TestCase):
+    def create_container_platform(self):
+        return cp.Docker()
+
+    def test_mount_points(self):
+        self.container_platform.image = 'name:tag'
+        self.container_platform.mount_points = [('/path/one', '/one'),
+                                                ('/path/two', '/two')]
+        self.container_platform.commands = ['cmd1', 'cmd2']
+        expected = [
+            "docker run -v /path/one:/one -v /path/two:/two -v $PWD:/stagedir "
+            "name:tag bash -c 'cd /stagedir; cmd1; cmd2'"
+        ]
+        self.assertEqual(expected,
+                         self.container_platform.emit_launch_commands())
+
+    def test_missing_image(self):
+        self.container_platform.commands = ['cmd']
+        self.assertRaises(ContainerPlatformError,
+                          self.container_platform.emit_launch_commands)
+
+    def test_missing_commands(self):
+        self.container_platform.image = 'name:tag'
+        self.assertRaises(ContainerPlatformError,
+                          self.container_platform.emit_launch_commands)
+
+    def test_custom_registry(self):
+        self.container_platform.registry = 'registry/custom'
+        self.container_platform.image = 'name:tag'
+        self.container_platform.commands = ['cmd']
+        expected = [
+            "docker run -v $PWD:/stagedir registry/custom/name:tag "
+            "bash -c 'cd /stagedir; cmd'"
+        ]
+        self.assertEqual(expected,
+                         self.container_platform.emit_launch_commands())

From d27a37d3c458e17660bc4baa74e4909b2f1e11af Mon Sep 17 00:00:00 2001
From: Rafael Sarmiento <rafael.sarmiento@cscs.ch>
Date: Fri, 24 May 2019 11:11:26 +0200
Subject: [PATCH 2/4] fix comments

---
 .../{containerplatform.py => containers.py}   | 77 +++++++++++--------
 reframe/core/exceptions.py                    |  2 +-
 unittests/test_containerplatform.py           | 52 -------------
 unittests/test_containers.py                  | 68 ++++++++++++++++
 4 files changed, 113 insertions(+), 86 deletions(-)
 rename reframe/core/{containerplatform.py => containers.py} (53%)
 delete mode 100644 unittests/test_containerplatform.py
 create mode 100644 unittests/test_containers.py

diff --git a/reframe/core/containerplatform.py b/reframe/core/containers.py
similarity index 53%
rename from reframe/core/containerplatform.py
rename to reframe/core/containers.py
index 38d12e3fa7..2e3bd46eef 100644
--- a/reframe/core/containerplatform.py
+++ b/reframe/core/containers.py
@@ -2,7 +2,7 @@
 
 import reframe.core.fields as fields
 import reframe.utility.typecheck as typ
-from reframe.core.exceptions import ContainerPlatformError
+from reframe.core.exceptions import ContainerError
 
 
 class ContainerPlatform:
@@ -15,72 +15,83 @@ class ContainerPlatform:
     registry = fields.TypedField('registry', str, type(None))
     image = fields.TypedField('image', str, type(None))
     requires_mpi = fields.TypedField('requires_mpi', bool)
-    commands = fields.TypedField('commands', typ.List[str], type(None))
+    commands = fields.TypedField('commands', typ.List[str])
     mount_points = fields.TypedField('mount_points',
-                                     typ.List[typ.Tuple[str, str]], type(None))
+                                     typ.List[typ.Tuple[str, str]])
+    workdir = fields.TypedField('workdir', str, type(None))
 
     def __init__(self):
         self.registry = None
         self.image = None
         self.requires_mpi = False
-        self.commands = None
+        self.commands = []
         self.mount_points  = []
+        self.workdir = None
 
     @abc.abstractmethod
     def emit_prepare_cmds(self):
-        """xxx."""
+        """Returns commands that are necessary before running with this
+        container platform.
+
+        :raises: `ContainerError` in case of errors.
+
+        .. note:
+            This method is relevant only to developers of new container
+            platforms.
+        """
 
     @abc.abstractmethod
     def emit_launch_cmds(self):
         """Returns the command for running with this container platform.
 
-        :raises: `ContainerPlatformError` in case of missing mandatory
-        fields.
+        :raises: `ContainerError` in case of errors.
+
+        .. note:
+            This method is relevant only to developers of new container
+            platforms.
+        """
+        if self.registry:
+            self.image = '/'.join([self.registry, self.image])
+
+    @abc.abstractmethod
+    def validate(self):
+        """Validates this container platform.
+
+        :raises: `ContainerError` in case of errors.
 
         .. note:
             This method is relevant only to developers of new container
-            plataforms.
+            platforms.
         """
+        if self.image is None:
+            raise ContainerError('no image specified.')
+
+        if not self.commands:
+            raise ContainerError('no command specified')
 
 
 class Docker(ContainerPlatform):
-    """An implementation of the container platform to run containers with
+    """An implementation of ContainerPlatform to run containers with
     Docker.
     """
-    # def __init__(self):
-    #     super().__init__()
-
     def emit_prepare_cmds(self):
         pass
 
     def emit_launch_commands(self):
+        super().emit_launch_cmds()
         docker_opts = []
-
-        if self.image is None:
-            raise ContainerPlatformError('Please, specify the name of'
-                                         'the image')
-
-        if self.commands is None:
-            raise ContainerPlatformError('Please, specify a command')
-
-        if self.registry:
-            self.image = '%s/%s' % (self.registry, self.image)
-
-        self.mount_points.append(('$PWD', '/stagedir'))
-        for mp in self.mount_points:
-            docker_opts.append('-v %s:%s' % mp)
-
-        cmd_base = "docker run %s %s bash -c 'cd /stagedir; %s'"
-
-        return [cmd_base % (' '.join(docker_opts), self.image,
-                            '; '.join(self.commands))]
+        docker_opts = ['-v "%s":"%s"' % mp for mp in self.mount_points]
+        run_cmd = 'docker run %s %s bash -c ' % (' '.join(docker_opts),
+                                                 self.image)
+        return run_cmd + "'" + '; '.join(
+            ['cd ' + self.workdir] + self.commands) + "'"
 
 
 class ContainerPlatformField(fields.TypedField):
-    """A field representing a build system.
+    """A field representing a container platforms.
 
     You may either assign an instance of :class:`ContainerPlatform:` or a
-    string representing the name of the concrete class of a build system.
+    string representing the name of the concrete class of a container platform.
     """
 
     def __init__(self, fieldname, *other_types):
diff --git a/reframe/core/exceptions.py b/reframe/core/exceptions.py
index 829d994c9c..ad0673a1f7 100644
--- a/reframe/core/exceptions.py
+++ b/reframe/core/exceptions.py
@@ -108,7 +108,7 @@ class BuildSystemError(ReframeError):
     """Raised when a build system is not configured properly."""
 
 
-class ContainerPlatformError(ReframeError):
+class ContainerError(ReframeError):
     """Raised when a container platform is not configured properly."""
 
 
diff --git a/unittests/test_containerplatform.py b/unittests/test_containerplatform.py
deleted file mode 100644
index f9a682929b..0000000000
--- a/unittests/test_containerplatform.py
+++ /dev/null
@@ -1,52 +0,0 @@
-import abc
-import unittest
-
-import reframe.core.containerplatform as cp
-from reframe.core.exceptions import ContainerPlatformError
-
-
-class _ContainerPlatformTest:
-    @abc.abstractmethod
-    def create_container_platform(self):
-        pass
-
-    def setUp(self):
-        self.container_platform = self.create_container_platform()
-
-
-class TestDocker(_ContainerPlatformTest, unittest.TestCase):
-    def create_container_platform(self):
-        return cp.Docker()
-
-    def test_mount_points(self):
-        self.container_platform.image = 'name:tag'
-        self.container_platform.mount_points = [('/path/one', '/one'),
-                                                ('/path/two', '/two')]
-        self.container_platform.commands = ['cmd1', 'cmd2']
-        expected = [
-            "docker run -v /path/one:/one -v /path/two:/two -v $PWD:/stagedir "
-            "name:tag bash -c 'cd /stagedir; cmd1; cmd2'"
-        ]
-        self.assertEqual(expected,
-                         self.container_platform.emit_launch_commands())
-
-    def test_missing_image(self):
-        self.container_platform.commands = ['cmd']
-        self.assertRaises(ContainerPlatformError,
-                          self.container_platform.emit_launch_commands)
-
-    def test_missing_commands(self):
-        self.container_platform.image = 'name:tag'
-        self.assertRaises(ContainerPlatformError,
-                          self.container_platform.emit_launch_commands)
-
-    def test_custom_registry(self):
-        self.container_platform.registry = 'registry/custom'
-        self.container_platform.image = 'name:tag'
-        self.container_platform.commands = ['cmd']
-        expected = [
-            "docker run -v $PWD:/stagedir registry/custom/name:tag "
-            "bash -c 'cd /stagedir; cmd'"
-        ]
-        self.assertEqual(expected,
-                         self.container_platform.emit_launch_commands())
diff --git a/unittests/test_containers.py b/unittests/test_containers.py
new file mode 100644
index 0000000000..4c15eb2114
--- /dev/null
+++ b/unittests/test_containers.py
@@ -0,0 +1,68 @@
+import abc
+import unittest
+
+import pytest
+import reframe.core.containers as containers
+from reframe.core.exceptions import ContainerError
+
+
+class _ContainerPlatformTest:
+    @abc.abstractmethod
+    def create_container_platform(self):
+        pass
+
+    @property
+    @abc.abstractmethod
+    def exp_cmd_mount_points(self):
+        pass
+
+    @property
+    @abc.abstractmethod
+    def exp_cmd_custom_registry(self):
+        pass
+
+    def setUp(self):
+        self.container_platform = self.create_container_platform()
+
+    def test_mount_points(self):
+        self.container_platform.image = 'name:tag'
+        self.container_platform.mount_points = [('/path/one', '/one'),
+                                                ('/path/two', '/two')]
+        self.container_platform.commands = ['cmd1', 'cmd2']
+        self.container_platform.workdir = '/stagedir'
+        self.assertEqual(self.exp_cmd_mount_points,
+                         self.container_platform.emit_launch_commands())
+
+    def test_missing_image(self):
+        self.container_platform.commands = ['cmd']
+        with pytest.raises(ContainerError):
+            self.container_platform.validate()
+
+    def test_missing_commands(self):
+        self.container_platform.image = 'name:tag'
+        with pytest.raises(ContainerError):
+            self.container_platform.validate()
+
+    def test_custom_registry(self):
+        self.container_platform.registry = 'registry/custom'
+        self.container_platform.image = 'name:tag'
+        self.container_platform.commands = ['cmd']
+        self.container_platform.mount_points = [('/path/one', '/one')]
+        self.container_platform.workdir = '/stagedir'
+        self.assertEqual(self.exp_cmd_custom_registry,
+                         self.container_platform.emit_launch_commands())
+
+
+class TestDocker(_ContainerPlatformTest, unittest.TestCase):
+    def create_container_platform(self):
+        return containers.Docker()
+
+    @property
+    def exp_cmd_mount_points(self):
+        return ('docker run -v "/path/one":"/one" -v "/path/two":"/two" '
+                "name:tag bash -c 'cd /stagedir; cmd1; cmd2'")
+
+    @property
+    def exp_cmd_custom_registry(self):
+        return ('docker run -v "/path/one":"/one" registry/custom/name:tag '
+                "bash -c 'cd /stagedir; cmd'")

From c7b09d3dbdb80b377b0a4a779fe350d2700ae934 Mon Sep 17 00:00:00 2001
From: Rafael Sarmiento <rafael.sarmiento@cscs.ch>
Date: Mon, 27 May 2019 10:51:29 +0200
Subject: [PATCH 3/4] fix comments

---
 reframe/core/containers.py   | 17 +++++++++--------
 unittests/test_containers.py | 10 +++++-----
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/reframe/core/containers.py b/reframe/core/containers.py
index 2e3bd46eef..03b68e6a2b 100644
--- a/reframe/core/containers.py
+++ b/reframe/core/containers.py
@@ -5,7 +5,7 @@
 from reframe.core.exceptions import ContainerError
 
 
-class ContainerPlatform:
+class ContainerPlatform(abc.ABC):
     """The abstract base class of any container platform.
 
     Concrete container platforms inherit from this class and must override the
@@ -64,28 +64,29 @@ def validate(self):
             platforms.
         """
         if self.image is None:
-            raise ContainerError('no image specified.')
+            raise ContainerError('no image specified')
 
         if not self.commands:
-            raise ContainerError('no command specified')
+            raise ContainerError('no commands specified')
 
 
 class Docker(ContainerPlatform):
-    """An implementation of ContainerPlatform to run containers with
-    Docker.
-    """
+    """An implementation of ContainerPlatform to run containers with Docker."""
     def emit_prepare_cmds(self):
         pass
 
-    def emit_launch_commands(self):
+    def emit_launch_cmds(self):
         super().emit_launch_cmds()
-        docker_opts = []
         docker_opts = ['-v "%s":"%s"' % mp for mp in self.mount_points]
         run_cmd = 'docker run %s %s bash -c ' % (' '.join(docker_opts),
                                                  self.image)
         return run_cmd + "'" + '; '.join(
             ['cd ' + self.workdir] + self.commands) + "'"
 
+    def validate(self):
+        super().validate()
+        pass
+
 
 class ContainerPlatformField(fields.TypedField):
     """A field representing a container platforms.
diff --git a/unittests/test_containers.py b/unittests/test_containers.py
index 4c15eb2114..96b5f2f346 100644
--- a/unittests/test_containers.py
+++ b/unittests/test_containers.py
@@ -6,7 +6,7 @@
 from reframe.core.exceptions import ContainerError
 
 
-class _ContainerPlatformTest:
+class _ContainerPlatformTest(abc.ABC):
     @abc.abstractmethod
     def create_container_platform(self):
         pass
@@ -30,8 +30,8 @@ def test_mount_points(self):
                                                 ('/path/two', '/two')]
         self.container_platform.commands = ['cmd1', 'cmd2']
         self.container_platform.workdir = '/stagedir'
-        self.assertEqual(self.exp_cmd_mount_points,
-                         self.container_platform.emit_launch_commands())
+        assert (self.exp_cmd_mount_points ==
+                self.container_platform.emit_launch_cmds())
 
     def test_missing_image(self):
         self.container_platform.commands = ['cmd']
@@ -49,8 +49,8 @@ def test_custom_registry(self):
         self.container_platform.commands = ['cmd']
         self.container_platform.mount_points = [('/path/one', '/one')]
         self.container_platform.workdir = '/stagedir'
-        self.assertEqual(self.exp_cmd_custom_registry,
-                         self.container_platform.emit_launch_commands())
+        assert (self.exp_cmd_custom_registry ==
+                self.container_platform.emit_launch_cmds())
 
 
 class TestDocker(_ContainerPlatformTest, unittest.TestCase):

From 15826e963498f06c1ad693999a5f07d8aae03941 Mon Sep 17 00:00:00 2001
From: Vasileios Karakasis <karakasis@cscs.ch>
Date: Mon, 27 May 2019 12:02:57 +0200
Subject: [PATCH 4/4] Coding style fixes

---
 reframe/core/containers.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/reframe/core/containers.py b/reframe/core/containers.py
index 03b68e6a2b..4db50b6dd9 100644
--- a/reframe/core/containers.py
+++ b/reframe/core/containers.py
@@ -72,6 +72,7 @@ def validate(self):
 
 class Docker(ContainerPlatform):
     """An implementation of ContainerPlatform to run containers with Docker."""
+
     def emit_prepare_cmds(self):
         pass
 
@@ -85,7 +86,6 @@ def emit_launch_cmds(self):
 
     def validate(self):
         super().validate()
-        pass
 
 
 class ContainerPlatformField(fields.TypedField):