diff options
-rw-r--r-- | lib/spack/spack/cmd/module.py | 5 | ||||
-rw-r--r-- | lib/spack/spack/hooks/module_file_generation.py | 7 | ||||
-rw-r--r-- | lib/spack/spack/modules/common.py | 53 | ||||
-rw-r--r-- | lib/spack/spack/spec.py | 74 | ||||
-rw-r--r-- | lib/spack/spack/test/data/modules/tcl/alter_environment.yaml | 1 | ||||
-rw-r--r-- | lib/spack/spack/test/data/modules/tcl/invalid_naming_scheme.yaml | 5 | ||||
-rw-r--r-- | lib/spack/spack/test/data/modules/tcl/invalid_token_in_env_var_name.yaml | 21 | ||||
-rw-r--r-- | lib/spack/spack/test/modules/tcl.py | 26 |
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.""" |