module files: restricted token expansion + case sensitivity (#5474)

closes #2884
closes #4684

In #1848 we decided to use `Spec.format` to expand certain tokens in
the module file naming scheme or in the environment variable name.
Not all the tokens that are allowed in `Spec.format` make sense in
module file generation. This PR restricts the set of tokens that can
be used, and adds tests to check that the intended behavior is respected.

Additionally, the names of environment variables set/modified by module
files were, up to now, always uppercase. There are packages though that
require case sensitive variable names to honor certain behaviors (e.g.
OpenMPI). This PR restricts the uppercase transformation in variable
names to `Spec.format` tokens.

lib/spack/spack/cmd/module.py

@@ -309,8 +309,9 @@ def refresh(module_types, specs, args):
             try:
                 x.write(overwrite=True)
             except Exception as e:
-                msg = 'Could not write module file because of {0}: [{1}]'
+                msg = 'Could not write module file [{0}]'
-                tty.warn(msg.format(str(e), x.layout.filename))
+                tty.warn(msg.format(x.layout.filename))
+                tty.warn('\t--> {0} <--'.format(str(e)))
 
 
 def module(parser, args):

lib/spack/spack/hooks/module_file_generation.py

@@ -37,7 +37,12 @@ def _for_each_enabled(spec, method_name):
     """Calls a method for each enabled module"""
     for name in enabled:
         generator = spack.modules.module_types[name](spec)
-        getattr(generator, method_name)()
+        try:
+            getattr(generator, method_name)()
+        except RuntimeError as e:
+            msg = 'cannot perform the requested {0} operation on module files'
+            msg += ' [{1}]'
+            tty.warn(msg.format(method_name, str(e)))
 
 
 post_install = lambda spec: _for_each_enabled(spec, 'write')

lib/spack/spack/modules/common.py

@@ -73,6 +73,37 @@
 #: Inspections that needs to be done on spec prefixes
 prefix_inspections = configuration.get('prefix_inspections', {})
 
+#: Valid tokens for naming scheme and env variable names
+_valid_tokens = (
+    'PACKAGE',
+    'VERSION',
+    'COMPILER',
+    'COMPILERNAME',
+    'COMPILERVER',
+    'ARCHITECTURE'
+)
+
+
+def _check_tokens_are_valid(format_string, message):
+    """Checks that the tokens used in the format string are valid in
+    the context of module file and environment variable naming.
+
+    Args:
+        format_string (str): string containing the format to be checked. This
+            is supposed to be a 'template' for ``Spec.format``
+
+        message (str): first sentence of the error message in case invalid
+            tokens are found
+
+    """
+    named_tokens = re.findall(r'\${(\w*)}', format_string)
+    invalid_tokens = [x for x in named_tokens if x not in _valid_tokens]
+    if invalid_tokens:
+        msg = message
+        msg += ' [{0}]. '.format(', '.join(invalid_tokens))
+        msg += 'Did you check your "modules.yaml" configuration?'
+        raise RuntimeError(msg)
+
 
 def update_dictionary_extending_lists(target, update):
     """Updates a dictionary, but extends lists instead of overriding them.
@@ -223,6 +254,12 @@ def naming_scheme(self):
             'naming_scheme',
             '${PACKAGE}-${VERSION}-${COMPILERNAME}-${COMPILERVER}'
         )
+
+        # Ensure the named tokens we are expanding are allowed, see
+        # issue #2884 for reference
+        msg = 'some tokens cannot be part of the module naming scheme'
+        _check_tokens_are_valid(scheme, message=msg)
+
         return scheme
 
     @property
@@ -521,14 +558,26 @@ def environment_modifications(self):
         blacklist = self.conf.environment_blacklist
 
         # We may have tokens to substitute in environment commands
+
+        # Prepare a suitable transformation dictionary for the names
+        # of the environment variables. This means turn the valid
+        # tokens uppercase.
+        transform = {}
+        for token in _valid_tokens:
+            transform[token] = str.upper
+
         for x in env:
-            x.name = self.spec.format(x.name)
+            # Ensure all the tokens are valid in this context
+            msg = 'some tokens cannot be expanded in an environment variable name'  # noqa: E501
+            _check_tokens_are_valid(x.name, message=msg)
+            # Transform them
+            x.name = self.spec.format(x.name, transform=transform)
             try:
                 # Not every command has a value
                 x.value = self.spec.format(x.value)
             except AttributeError:
                 pass
-            x.name = str(x.name).replace('-', '_').upper()
+            x.name = str(x.name).replace('-', '_')
 
         return [(type(x).__name__, x) for x in env if x.name not in blacklist]
 

lib/spack/spack/spec.py

@@ -2826,9 +2826,10 @@ def colorized(self):
         return colorize_spec(self)
 
     def format(self, format_string='$_$@$%@+$+$=', **kwargs):
-        """
+        """Prints out particular pieces of a spec, depending on what is
-        Prints out particular pieces of a spec, depending on what is
+        in the format string.
-        in the format string.  The format strings you can provide are::
+
+        The format strings you can provide are::
 
             $_   Package name
             $.   Full package name (with namespace)
@@ -2870,13 +2871,36 @@ def format(self, format_string='$_$@$%@+$+$=', **kwargs):
 
         Anything else is copied verbatim into the output stream.
 
-        *Example:*  ``$_$@$+`` translates to the name, version, and options
+        Args:
-        of the package, but no dependencies, arch, or compiler.
+            format_string (str): string containing the format to be expanded
+
+            **kwargs (dict): the following list of keywords is supported
+
+                - color (bool): True if returned string is colored
+
+                - transform (dict): maps full-string formats to a callable \
+                that accepts a string and returns another one
+
+        Examples:
+
+            The following line:
+
+            .. code-block:: python
+
+                s = spec.format('$_$@$+')
+
+            translates to the name, version, and options of the package, but no
+            dependencies, arch, or compiler.
 
         TODO: allow, e.g., ``$6#`` to customize short hash length
         TODO: allow, e.g., ``$//`` for full hash.
         """
         color = kwargs.get('color', False)
+
+        # Dictionary of transformations for named tokens
+        token_transforms = {}
+        token_transforms.update(kwargs.get('transform', {}))
+
         length = len(format_string)
         out = StringIO()
         named = escape = compiler = False
@@ -2953,39 +2977,55 @@ def write(s, c):
                     named_str += c
                     continue
                 named_str = named_str.upper()
+
+                # Retrieve the token transformation from the dictionary.
+                #
+                # The default behavior is to leave the string unchanged
+                # (`lambda x: x` is the identity function)
+                token_transform = token_transforms.get(named_str, lambda x: x)
+
                 if named_str == 'PACKAGE':
                     name = self.name if self.name else ''
-                    write(fmt % self.name, '@')
+                    write(fmt % token_transform(name), '@')
                 if named_str == 'VERSION':
                     if self.versions and self.versions != _any_version:
-                        write(fmt % str(self.versions), '@')
+                        write(fmt % token_transform(str(self.versions)), '@')
                 elif named_str == 'COMPILER':
                     if self.compiler:
-                        write(fmt % self.compiler, '%')
+                        write(fmt % token_transform(self.compiler), '%')
                 elif named_str == 'COMPILERNAME':
                     if self.compiler:
-                        write(fmt % self.compiler.name, '%')
+                        write(fmt % token_transform(self.compiler.name), '%')
                 elif named_str in ['COMPILERVER', 'COMPILERVERSION']:
                     if self.compiler:
-                        write(fmt % self.compiler.versions, '%')
+                        write(
+                            fmt % token_transform(self.compiler.versions),
+                            '%'
+                        )
                 elif named_str == 'COMPILERFLAGS':
                     if self.compiler:
-                        write(fmt % str(self.compiler_flags), '%')
+                        write(
+                            fmt % token_transform(str(self.compiler_flags)),
+                            '%'
+                        )
                 elif named_str == 'OPTIONS':
                     if self.variants:
-                        write(fmt % str(self.variants), '+')
+                        write(fmt % token_transform(str(self.variants)), '+')
                 elif named_str == 'ARCHITECTURE':
                     if self.architecture and str(self.architecture):
-                        write(fmt % str(self.architecture), '=')
+                        write(
+                            fmt % token_transform(str(self.architecture)),
+                            '='
+                        )
                 elif named_str == 'SHA1':
                     if self.dependencies:
-                        out.write(fmt % str(self.dag_hash(7)))
+                        out.write(fmt % token_transform(str(self.dag_hash(7))))
                 elif named_str == 'SPACK_ROOT':
-                    out.write(fmt % spack.prefix)
+                    out.write(fmt % token_transform(spack.prefix))
                 elif named_str == 'SPACK_INSTALL':
-                    out.write(fmt % spack.store.root)
+                    out.write(fmt % token_transform(spack.store.root))
                 elif named_str == 'PREFIX':
-                    out.write(fmt % self.prefix)
+                    out.write(fmt % token_transform(self.prefix))
                 elif named_str.startswith('HASH'):
                     if named_str.startswith('HASH:'):
                         _, hashlen = named_str.split(':')

lib/spack/spack/test/data/modules/tcl/alter_environment.yaml

@@ -13,6 +13,7 @@ tcl:
     environment:
       set:
         FOO: 'foo'
+        OMPI_MCA_mpi_leave_pinned: '1'
       unset:
         - BAR
 

lib/spack/spack/test/data/modules/tcl/invalid_naming_scheme.yaml

@@ -0,0 +1,5 @@
+enable:
+  - tcl
+tcl:
+  # ${OPTIONS} is not allowed in the naming scheme, see #2884
+  naming_scheme: '${PACKAGE}/${VERSION}-${COMPILERNAME}-${OPTIONS}'

lib/spack/spack/test/data/modules/tcl/invalid_token_in_env_var_name.yaml

@@ -0,0 +1,21 @@
+enable:
+  - tcl
+tcl:
+  all:
+    filter:
+      environment_blacklist':
+        - CMAKE_PREFIX_PATH
+    environment:
+      set:
+        '${PACKAGE}_ROOT_${PREFIX}': '${PREFIX}'
+
+  'platform=test target=x86_64':
+    environment:
+      set:
+        FOO_${OPTIONS}: 'foo'
+      unset:
+        - BAR
+
+  'platform=test target=x86_32':
+    load:
+      - 'foo/bar'

lib/spack/spack/test/modules/tcl.py

@@ -121,6 +121,12 @@ def test_alter_environment(self, modulefile_content, patch_configuration):
                     if x.startswith('prepend-path CMAKE_PREFIX_PATH')
                     ]) == 0
         assert len([x for x in content if 'setenv FOO "foo"' in x]) == 1
+        assert len([
+            x for x in content if 'setenv OMPI_MCA_mpi_leave_pinned "1"' in x
+        ]) == 1
+        assert len([
+            x for x in content if 'setenv OMPI_MCA_MPI_LEAVE_PINNED "1"' in x
+        ]) == 0
         assert len([x for x in content if 'unsetenv BAR' in x]) == 1
         assert len([x for x in content if 'setenv MPILEAKS_ROOT' in x]) == 1
 
@@ -168,6 +174,26 @@ def test_naming_scheme(self, factory, patch_configuration):
 
         assert writer.conf.naming_scheme == expected
 
+    def test_invalid_naming_scheme(self, factory, patch_configuration):
+        """Tests the evaluation of an invalid naming scheme."""
+
+        patch_configuration('invalid_naming_scheme')
+
+        # Test that having invalid tokens in the naming scheme raises
+        # a RuntimeError
+        writer, _ = factory('mpileaks')
+        with pytest.raises(RuntimeError):
+            writer.layout.use_name
+
+    def test_invalid_token_in_env_name(self, factory, patch_configuration):
+        """Tests setting environment variables with an invalid name."""
+
+        patch_configuration('invalid_token_in_env_var_name')
+
+        writer, _ = factory('mpileaks')
+        with pytest.raises(RuntimeError):
+            writer.write()
+
     def test_conflicts(self, modulefile_content, patch_configuration):
         """Tests adding conflicts to the module."""