Fix views system module loading (#600)

* Fix systems modules in views
* Fix .view_module(.lua) in test and name
* Add CHANGELOG line and bump version
* add shared function to derive view module name

and expose as a variable on the view class so
we do not need to redundantly define it! I also
had added some redundancy to testing, so I migrated
that into one shared function

Signed-off-by: Georgia Stuart <[email protected]>
Co-authored-by: vsoch <[email protected]>

Author: georgiastuart

CHANGELOG.md

@@ -14,6 +14,7 @@ and **Merged pull requests**. Critical items to know are:
 The versions coincide with releases on pip. Only major versions will be released as tags on Github.
 
 ## [0.0.x](https://github.com/singularityhub/singularity-hpc/tree/main) (0.0.x)
+ - fix views .view_module modulefile and loading (0.1.14)
  - support for system modules, depends on, in views and editor envars (0.1.13)
  - Wrappers now supported for shell/exec/run container commands (0.1.12)
  - Update add to return container yaml (0.1.11)

shpc/main/modules/templates/includes/load_view.lua

@@ -1,6 +1,6 @@
 local view_dir = string.gsub(myFileName():match("(.*[/])") or ".","/%a*/$","")
 local view_name = string.gsub(view_dir,".*/","")
-local view_module = '.view_' .. view_name
+local view_module = '.view_module'
 local view_modulefile = view_dir .. '/' .. view_module .. '.lua'
 
 if isFile(view_modulefile) then

shpc/main/modules/templates/includes/load_view.tcl

@@ -1,6 +1,6 @@
 set view_dir "[file dirname [file dirname ${ModulesCurrentModulefile}] ]"
 set view_name "[file tail ${view_dir}]"
-set view_module ".view_${view_name}"
+set view_module ".view_module"
 set view_modulefile "${view_dir}/${view_module}"
 
 if {[file exists ${view_modulefile}]} {

shpc/main/modules/templates/view_module.lua

@@ -3,5 +3,5 @@
 --
 
 {% for module in system_modules %}
-module load("{{ module }}"){% endfor %}{% for depends in depends_on %}
+load("{{ module }}"){% endfor %}{% for depends in depends_on %}
 depends_on("{{ depends }}"){% endfor %}

shpc/main/modules/views.py

@@ -19,6 +19,15 @@
 supported_view_variables = {"system_modules": [], "depends_on": []}
 
 
+# Shared functions
+def get_view_module_path(extension):
+    """
+    Get a view module file name based on an extension
+    """
+    modulefile_extension = ".lua" if extension == "lua" else ""
+    return ".view_module%s" % modulefile_extension
+
+
 class ViewModule:
     """
     A ViewModule is a .view_module written to the base of a view.
@@ -39,13 +48,20 @@ def write(self, view_dir, view_config):
         if not os.path.exists(view_dir):
             return
         template = self.template.load("view_module.%s" % self.module_extension)
-        view_module_file = os.path.join(view_dir, ".view_module")
+
+        # Assemble the .view_module.<extension> full path
+        view_module_file = os.path.join(
+            view_dir, get_view_module_path(self.module_extension)
+        )
         out = template.render(
             system_modules=view_config["view"].get("system_modules", []),
             depends_on=view_config["view"].get("depends_on", []),
         )
         utils.write_file(view_module_file, out)
-        logger.info("Wrote updated .view_module: %s" % view_module_file)
+        logger.info(
+            "Wrote updated .view_module.%s: %s"
+            % (self.module_extension, view_module_file)
+        )
 
 
 class ViewsHandler:
@@ -292,6 +308,13 @@ def path(self):
         """
         return os.path.join(self.settings.views_base, self.name)
 
+    @property
+    def module_path(self):
+        """
+        Path to view module, with needed extension.
+        """
+        return os.path.join(self.path, get_view_module_path(self.module_extension))
+
     def reload(self):
         """
         Reload the view's config (given a change)

shpc/tests/test_views.py

@@ -108,36 +108,10 @@ def test_views(tmp_path, module_sys, module_file, container_tech, remote):
     )
 
     # Before adding any attributes, this file should not exist
-    view_module = os.path.join(view.path, ".view_module")
-    assert not os.path.exists(view_module)
+    assert not os.path.exists(view.module_path)
 
-    # Try adding valid attributes
-    for attribute, check_content in [
-        ["system_modules", "module"],
-        ["depends_on", "depends"],
-    ]:
-        assert not view._config["view"][attribute]
-        view_handler.add_variable(view_name, attribute, "openmpi")
-        assert os.path.exists(view_module)
-
-        # Make sure we have openmpi in the content
-        content = utils.read_file(view_module)
-        assert "openmpi" in content and check_content in content
-
-        # Reload the view config file
-        view.reload()
-        assert "openmpi" in view._config["view"][attribute]
-
-        # We can't add unknown variables
-        with pytest.raises(SystemExit):
-            view_handler.add_variable(view_name, attribute.replace("_", "-"), [1, 2, 3])
-
-        # Try removing now
-        view_handler.remove_variable(view_name, attribute, "openmpi")
-        view.reload()
-        assert not view._config["view"][attribute]
-        content = utils.read_file(view_module)
-        assert "openmpi" not in content and check_content not in content
+    # Shared function to check view and handler
+    check_view(view, view_handler)
 
     # Ensure we can uninstall
     view_handler.delete(view_name, force=True)
@@ -185,38 +159,47 @@ def test_view_components(tmp_path, module_sys, module_file, container_tech, remo
 
     # Before adding any attributes, this file should not exist
     view = client.views[view_name]
-    view_module = os.path.join(view.path, ".view_module")
-    assert not os.path.exists(view_module)
+    assert not os.path.exists(view.module_path)
+
+    check_view(view, view_handler)
+
+    # Ensure we can uninstall
+    view_handler.delete(view_name, force=True)
+    assert not os.path.exists(view.path)
 
+
+def check_view(view, view_handler):
+    """
+    Shared function for checking content of view.
+    """
     # Try adding valid attributes
     for attribute, check_content in [
-        ["system_modules", "module"],
+        ["system_modules", None],
         ["depends_on", "depends"],
     ]:
         assert not view._config["view"][attribute]
-        view_handler.add_variable(view_name, attribute, "openmpi")
-        assert os.path.exists(view_module)
+        view_handler.add_variable(view.name, attribute, "openmpi")
+        assert os.path.exists(view.module_path)
 
         # Make sure we have openmpi in the content
-        content = utils.read_file(view_module)
-        print(content)
-        assert "openmpi" in content and check_content in content
+        content = utils.read_file(view.module_path)
+        assert "openmpi" in content
+        if check_content:
+            assert check_content in content
 
         # Reload the view config file
         view.reload()
         assert "openmpi" in view._config["view"][attribute]
 
         # We can't add unknown variables
         with pytest.raises(SystemExit):
-            view_handler.add_variable(view_name, attribute.replace("_", "-"), [1, 2, 3])
+            view_handler.add_variable(view.name, attribute.replace("_", "-"), [1, 2, 3])
 
         # Try removing now
-        view_handler.remove_variable(view_name, attribute, "openmpi")
+        view_handler.remove_variable(view.name, attribute, "openmpi")
         view.reload()
         assert not view._config["view"][attribute]
-        content = utils.read_file(view_module)
-        assert "openmpi" not in content and check_content not in content
-
-    # Ensure we can uninstall
-    view_handler.delete(view_name, force=True)
-    assert not os.path.exists(view.path)
+        content = utils.read_file(view.module_path)
+        assert "openmpi" not in content
+        if check_content:
+            assert check_content not in content

shpc/version.py

@@ -2,7 +2,7 @@
 __copyright__ = "Copyright 2021-2022, Vanessa Sochat"
 __license__ = "MPL 2.0"
 
-__version__ = "0.1.13"
+__version__ = "0.1.14"
 AUTHOR = "Vanessa Sochat"
 EMAIL = "[email protected]"
 NAME = "singularity-hpc"