Use Spec.format for token substitution in modules (#1848)

This replaces a custom token-based substitution format with calls to
Spec.format in modules.py

This also resolves a couple issues:

- LmodModules set configuration globally instead of in its initializer
  which meant test-specific configuration was not applied
- Added support for setting hash_length=0 for LmodModules. This only
  affects the module filename and not the directory names for the
  hierarchy tokens in the path. This includes an additional unit test.

lib/spack/spack/modules.py

@@ -131,15 +131,13 @@ def dependencies(spec, request='all'):
     # FIXME : step among nodes that refer to the same package?
     seen = set()
     seen_add = seen.add
-    l = [xx
+    l = sorted(
-         for xx in sorted(
+        spec.traverse(order='post',
-             spec.traverse(order='post',
+                      cover='nodes',
-                           depth=True,
+                      deptype=('link', 'run'),
-                           cover='nodes',
+                      root=False),
-                           deptype=('link', 'run'),
+        reverse=True)
-                           root=False),
+    return [x for x in l if not (x in seen or seen_add(x))]
-             reverse=True)]
-    return [xx for ii, xx in l if not (xx in seen or seen_add(xx))]
 
 
 def update_dictionary_extending_lists(target, update):
@@ -238,6 +236,10 @@ def filter_blacklisted(specs, module_name):
         yield x
 
 
+def format_env_var_name(name):
+    return name.replace('-', '_').upper()
+
+
 class EnvModule(object):
     name = 'env_module'
     formats = {}
@@ -274,51 +276,30 @@ def naming_scheme(self):
             naming_scheme = self.default_naming_format
         return naming_scheme
 
-    @property
-    def tokens(self):
-        """Tokens that can be substituted in environment variable values
-        and naming schemes
-        """
-        tokens = {
-            'name': self.spec.name,
-            'version': self.spec.version,
-            'compiler': self.spec.compiler,
-            'prefix': self.spec.package.prefix
-        }
-        return tokens
-
-    @property
-    def upper_tokens(self):
-        """Tokens that can be substituted in environment variable names"""
-        upper_tokens = {
-            'name': self.spec.name.replace('-', '_').upper()
-        }
-        return upper_tokens
-
     @property
     def use_name(self):
         """
         Subclasses should implement this to return the name the module command
         uses to refer to the package.
         """
-        naming_tokens = self.tokens
+        name = self.spec.format(self.naming_scheme)
-        naming_scheme = self.naming_scheme
-        name = naming_scheme.format(**naming_tokens)
         # Not everybody is working on linux...
         parts = name.split('/')
         name = join_path(*parts)
         # Add optional suffixes based on constraints
+        path_elements = [name] + self._get_suffixes()
+        return '-'.join(path_elements)
+
+    def _get_suffixes(self):
         configuration, _ = parse_config_options(self)
-        suffixes = [name]
+        suffixes = []
         for constraint, suffix in configuration.get('suffixes', {}).items():
             if constraint in self.spec:
                 suffixes.append(suffix)
-        # Always append the hash to make the module file unique
+        hash_length = configuration.get('hash_length', 7)
-        hash_length = configuration.pop('hash_length', 7)
         if hash_length != 0:
             suffixes.append(self.spec.dag_hash(length=hash_length))
-        name = '-'.join(suffixes)
+        return suffixes
-        return name
 
     @property
     def category(self):
@@ -456,8 +437,9 @@ def prerequisite(self, spec):
     def process_environment_command(self, env):
         for command in env:
             # Token expansion from configuration file
-            name = command.args.get('name', '').format(**self.upper_tokens)
+            name = format_env_var_name(
-            value = str(command.args.get('value', '')).format(**self.tokens)
+                self.spec.format(command.args.get('name', '')))
+            value = self.spec.format(str(command.args.get('value', '')))
             command.update_args(name=name, value=value)
             # Format the line int the module file
             try:
@@ -501,7 +483,7 @@ class Dotkit(EnvModule):
     autoload_format = 'dk_op {module_file}\n'
 
     default_naming_format = \
-        '{name}-{version}-{compiler.name}-{compiler.version}'
+        '${PACKAGE}-${VERSION}-${COMPILERNAME}-${COMPILERVER}'
 
     @property
     def file_name(self):
@@ -544,7 +526,7 @@ class TclModule(EnvModule):
     prerequisite_format = 'prereq {module_file}\n'
 
     default_naming_format = \
-        '{name}-{version}-{compiler.name}-{compiler.version}'
+        '${PACKAGE}-${VERSION}-${COMPILERNAME}-${COMPILERVER}'
 
     @property
     def file_name(self):
@@ -595,8 +577,9 @@ def process_environment_command(self, env):
         }
         for command in env:
             # Token expansion from configuration file
-            name = command.args.get('name', '').format(**self.upper_tokens)
+            name = format_env_var_name(
-            value = str(command.args.get('value', '')).format(**self.tokens)
+                self.spec.format(command.args.get('name', '')))
+            value = self.spec.format(str(command.args.get('value', '')))
             command.update_args(name=name, value=value)
             # Format the line int the module file
             try:
@@ -614,7 +597,6 @@ def process_environment_command(self, env):
                 tty.warn(details.format(**command.args))
 
     def module_specific_content(self, configuration):
-        naming_tokens = self.tokens
         # Conflict
         conflict_format = configuration.get('conflict', [])
         f = string.Formatter()
@@ -635,7 +617,7 @@ def module_specific_content(self, configuration):
                                                  nformat=self.naming_scheme,
                                                  cformat=item))
                         raise SystemExit('Module generation aborted.')
-                line = line.format(**naming_tokens)
+                line = self.spec.format(line)
             yield line
 
 # To construct an arbitrary hierarchy of module files:
@@ -672,24 +654,24 @@ class LmodModule(EnvModule):
 
     family_format = 'family("{family}")\n'
 
-    path_part_with_hash = join_path('{token.name}', '{token.version}-{token.hash}')  # NOQA: ignore=E501
     path_part_without_hash = join_path('{token.name}', '{token.version}')
 
-    # TODO : Check that extra tokens specified in configuration file
-    # TODO : are actually virtual dependencies
-    configuration = CONFIGURATION.get('lmod', {})
-    hierarchy_tokens = configuration.get('hierarchical_scheme', [])
-    hierarchy_tokens = hierarchy_tokens + ['mpi', 'compiler']
-
     def __init__(self, spec=None):
         super(LmodModule, self).__init__(spec)
+
+        self.configuration = CONFIGURATION.get('lmod', {})
+        hierarchy_tokens = self.configuration.get('hierarchical_scheme', [])
+        # TODO : Check that the extra hierarchy tokens specified in the
+        # TODO : configuration file are actually virtual dependencies
+        self.hierarchy_tokens = hierarchy_tokens + ['mpi', 'compiler']
+
         # Sets the root directory for this architecture
         self.modules_root = join_path(LmodModule.path, self.spec.architecture)
+
         # Retrieve core compilers
         self.core_compilers = self.configuration.get('core_compilers', [])
         # Keep track of the requirements that this package has in terms
-        # of virtual packages
+        # of virtual packages that participate in the hierarchical structure
-        # that participate in the hierarchical structure
         self.requires = {'compiler': self.spec.compiler}
         # For each virtual dependency in the hierarchy
         for x in self.hierarchy_tokens:
@@ -740,10 +722,10 @@ def token_to_path(self, name, value):
         # CompilerSpec does not have an hash
         if name == 'compiler':
             return self.path_part_without_hash.format(token=value)
-        # For virtual providers add a small part of the hash
+        # In this case the hierarchy token refers to a virtual provider
-        # to distinguish among different variants in a directory hierarchy
+        path = self.path_part_without_hash.format(token=value)
-        value.hash = value.dag_hash(length=6)
+        path = '-'.join([path, value.dag_hash(length=7)])
-        return self.path_part_with_hash.format(token=value)
+        return path
 
     @property
     def file_name(self):
@@ -756,7 +738,10 @@ def file_name(self):
 
     @property
     def use_name(self):
-        return self.token_to_path('', self.spec)
+        path_elements = [self.spec.format("${PACKAGE}/${VERSION}")]
+        # The remaining elements are filename suffixes
+        path_elements.extend(self._get_suffixes())
+        return '-'.join(path_elements)
 
     def modulepath_modifications(self):
         # What is available is what we require plus what we provide

lib/spack/spack/spec.py

@@ -2201,10 +2201,12 @@ def format(self, format_string='$_$@$%@+$+$=', **kwargs):
             ${OPTIONS}       Options
             ${ARCHITECTURE}  Architecture
             ${SHA1}          Dependencies 8-char sha1 prefix
+            ${HASH:len}      DAG hash with optional length specifier
 
             ${SPACK_ROOT}    The spack root directory
             ${SPACK_INSTALL} The default spack install directory,
                              ${SPACK_PREFIX}/opt
+            ${PREFIX}        The package prefix
 
         Optionally you can provide a width, e.g. ``$20_`` for a 20-wide name.
         Like printf, you can provide '-' for left justification, e.g.
@@ -2327,6 +2329,15 @@ def write(s, c):
                     out.write(fmt % spack.prefix)
                 elif named_str == 'SPACK_INSTALL':
                     out.write(fmt % spack.install_path)
+                elif named_str == 'PREFIX':
+                    out.write(fmt % self.prefix)
+                elif named_str.startswith('HASH'):
+                    if named_str.startswith('HASH:'):
+                        _, hashlen = named_str.split(':')
+                        hashlen = int(hashlen)
+                    else:
+                        hashlen = None
+                    out.write(fmt % (self.dag_hash(hashlen)))
 
                 named = False
 

lib/spack/spack/test/modules.py

@@ -27,6 +27,7 @@
 
 import StringIO
 import spack.modules
+import spack.spec
 from spack.test.mock_packages_test import MockPackagesTest
 
 FILE_REGISTRY = collections.defaultdict(StringIO.StringIO)
@@ -167,7 +168,7 @@ class TclTests(ModuleFileGeneratorTests):
             'all': {
                 'filter': {'environment_blacklist': ['CMAKE_PREFIX_PATH']},
                 'environment': {
-                    'set': {'{name}_ROOT': '{prefix}'}
+                    'set': {'${PACKAGE}_ROOT': '${PREFIX}'}
                 }
             },
             'platform=test target=x86_64': {
@@ -196,9 +197,9 @@ class TclTests(ModuleFileGeneratorTests):
     configuration_conflicts = {
         'enable': ['tcl'],
         'tcl': {
-            'naming_scheme': '{name}/{version}-{compiler.name}',
+            'naming_scheme': '${PACKAGE}/${VERSION}-${COMPILERNAME}',
             'all': {
-                'conflict': ['{name}', 'intel/14.0.1']
+                'conflict': ['${PACKAGE}', 'intel/14.0.1']
             }
         }
     }
@@ -206,9 +207,9 @@ class TclTests(ModuleFileGeneratorTests):
     configuration_wrong_conflicts = {
         'enable': ['tcl'],
         'tcl': {
-            'naming_scheme': '{name}/{version}-{compiler.name}',
+            'naming_scheme': '${PACKAGE}/${VERSION}-${COMPILERNAME}',
             'all': {
-                'conflict': ['{name}/{compiler.name}']
+                'conflict': ['${PACKAGE}/${COMPILERNAME}']
             }
         }
     }
@@ -371,6 +372,13 @@ class LmodTests(ModuleFileGeneratorTests):
         }
     }
 
+    configuration_no_hash = {
+        'enable': ['lmod'],
+        'lmod': {
+            'hash_length': 0
+        }
+    }
+
     configuration_alter_environment = {
         'enable': ['lmod'],
         'lmod': {
@@ -455,6 +463,24 @@ def test_blacklist(self):
             len([x for x in content if 'if not isloaded(' in x]), 1)
         self.assertEqual(len([x for x in content if 'load(' in x]), 1)
 
+    def test_no_hash(self):
+        # Make sure that virtual providers (in the hierarchy) always
+        # include a hash. Make sure that the module file for the spec
+        # does not include a hash if hash_length is 0.
+        spack.modules.CONFIGURATION = self.configuration_no_hash
+        spec = spack.spec.Spec(mpileaks_spec_string)
+        spec.concretize()
+        module = spack.modules.LmodModule(spec)
+        path = module.file_name
+        mpiSpec = spec['mpi']
+        mpiElement = "{0}/{1}-{2}/".format(
+            mpiSpec.name, mpiSpec.version, mpiSpec.dag_hash(length=7))
+        self.assertTrue(mpiElement in path)
+        mpileaksSpec = spec
+        mpileaksElement = "{0}/{1}.lua".format(
+            mpileaksSpec.name, mpileaksSpec.version)
+        self.assertTrue(path.endswith(mpileaksElement))
+
 
 class DotkitTests(MockPackagesTest):