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.

8 files changed, 170 insertions, 22 deletions

diff --git a/lib/spack/spack/cmd/module.py b/lib/spack/spack/cmd/module.py
index e3e559794b..422ccdc196 100644
--- a/lib/spack/spack/cmd/module.py
+++ b/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}]'
-                tty.warn(msg.format(str(e), x.layout.filename))
+                msg = 'Could not write module file [{0}]'
+                tty.warn(msg.format(x.layout.filename))
+                tty.warn('\t--> {0} <--'.format(str(e)))
 
 
 def module(parser, args):
diff --git a/lib/spack/spack/hooks/module_file_generation.py b/lib/spack/spack/hooks/module_file_generation.py
index 292c195e7b..1e7eb6d6e7 100644
--- a/lib/spack/spack/hooks/module_file_generation.py
+++ b/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')
diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py
index 89f6234705..3907e5e654 100644
--- a/lib/spack/spack/modules/common.py
+++ b/lib/spack/spack/modules/common.py
@@ -73,6 +73,37 @@ configuration = spack.config.get_config('modules')
 #: 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 @@ class BaseConfiguration(object):
             '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 @@ class BaseContext(tengine.Context):
         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]
 
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 2e144fc80f..624c28285a 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2826,9 +2826,10 @@ class Spec(object):
         return colorize_spec(self)
 
     def format(self, format_string='$_$@$%@+$+$=', **kwargs):
-        """
-        Prints out particular pieces of a spec, depending on what is
-        in the format string.  The format strings you can provide are::
+        """Prints out particular pieces of a spec, depending on what is
+        in the format string.
+
+        The format strings you can provide are::
 
             $_   Package name
             $.   Full package name (with namespace)
@@ -2870,13 +2871,36 @@ class Spec(object):
 
         Anything else is copied verbatim into the output stream.
 
-        *Example:*  ``$_$@$+`` translates to the name, version, and options
-        of the package, but no dependencies, arch, or compiler.
+        Args:
+            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 @@ class Spec(object):
                     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(':')
diff --git a/lib/spack/spack/test/data/modules/tcl/alter_environment.yaml b/lib/spack/spack/test/data/modules/tcl/alter_environment.yaml
index 3a8736a640..c9f00c6360 100644
--- a/lib/spack/spack/test/data/modules/tcl/alter_environment.yaml
+++ b/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
 
diff --git a/lib/spack/spack/test/data/modules/tcl/invalid_naming_scheme.yaml b/lib/spack/spack/test/data/modules/tcl/invalid_naming_scheme.yaml
new file mode 100644
index 0000000000..2f72ba4aab
--- /dev/null
+++ b/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}'
diff --git a/lib/spack/spack/test/data/modules/tcl/invalid_token_in_env_var_name.yaml b/lib/spack/spack/test/data/modules/tcl/invalid_token_in_env_var_name.yaml
new file mode 100644
index 0000000000..297028531b
--- /dev/null
+++ b/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'
diff --git a/lib/spack/spack/test/modules/tcl.py b/lib/spack/spack/test/modules/tcl.py
index a0a6b69340..61000efcb8 100644
--- a/lib/spack/spack/test/modules/tcl.py
+++ b/lib/spack/spack/test/modules/tcl.py
@@ -121,6 +121,12 @@ class TestTcl(object):
                     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 @@ class TestTcl(object):
 
         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."""