Unified environment modifications in config files (#14372)

* Unified environment modifications in config files

fixes #13357

This commit factors all the code that is involved in
the validation (schema) and parsing of environment modifications
from configuration files in a single place. The factored out
code is then used for module files and compiler configuration.

Attributes were separated by dashes in `compilers.yaml` files and
by underscores in `modules.yaml` files. This PR unifies the syntax
on attributes separated by underscores.

Unit testing of environment modifications in compilers
has been refactored and simplified.

8 files changed, 169 insertions, 183 deletions

diff --git a/lib/spack/docs/basic_usage.rst b/lib/spack/docs/basic_usage.rst
index 8342ab4489..56d60a29da 100644
--- a/lib/spack/docs/basic_usage.rst
+++ b/lib/spack/docs/basic_usage.rst
@@ -929,11 +929,13 @@ in GNU Autotools. If all flags are set, the order is
 Compiler environment variables and additional RPATHs
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-In the exceptional case a compiler requires setting special environment
-variables, like an explicit library load path. These can bet set in an
-extra section in the compiler configuration (the supported environment
-modification commands are: ``set``, ``unset``, ``append-path``, and
-``prepend-path``). The user can also specify additional ``RPATHs`` that the
+Sometimes compilers require setting special environment variables to
+operate correctly. Spack handles these cases by allowing custom environment
+modifications in the ``environment`` attribute of the compiler configuration
+section. See also the :ref:`configuration_environment_variables` section
+of the configuration files docs for more information.
+
+It is also possible to specify additional ``RPATHs`` that the
 compiler will add to all executables generated by that compiler.  This is
 useful for forcing certain compilers to RPATH their own runtime libraries, so
 that executables will run without the need to set ``LD_LIBRARY_PATH``.
@@ -950,28 +952,19 @@ that executables will run without the need to set ``LD_LIBRARY_PATH``.
           fc: /opt/gcc/bin/gfortran
         environment:
           unset:
-            BAD_VARIABLE: # The colon is required but the value must be empty
+            - BAD_VARIABLE
           set:
             GOOD_VARIABLE_NUM: 1
             GOOD_VARIABLE_STR: good
-          prepend-path:
+          prepend_path:
             PATH: /path/to/binutils
-          append-path:
+          append_path:
             LD_LIBRARY_PATH: /opt/gcc/lib
         extra_rpaths:
         - /path/to/some/compiler/runtime/directory
         - /path/to/some/other/compiler/runtime/directory
 
 
-.. note::
-
-   The section `environment` is interpreted as an ordered dictionary, which
-   means two things. First, environment modification are applied in the order
-   they are specified in the configuration file. Second, you cannot express
-   environment modifications that require mixing different commands, i.e. you
-   cannot `set` one variable, than `prepend-path` to another one, and than
-   again `set` a third one.
-
 ^^^^^^^^^^^^^^^^^^^^^^^
 Architecture specifiers
 ^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/lib/spack/docs/configuration.rst b/lib/spack/docs/configuration.rst
index c3ea125f19..41c9de79f9 100644
--- a/lib/spack/docs/configuration.rst
+++ b/lib/spack/docs/configuration.rst
@@ -427,6 +427,33 @@ home directory, and ``~user`` will expand to a specified user's home
 directory. The ``~`` must appear at the beginning of the path, or Spack
 will not expand it.
 
+.. _configuration_environment_variables:
+
+-------------------------
+Environment Modifications
+-------------------------
+
+Spack allows to prescribe custom environment modifications in a few places
+within its configuration files. Every time these modifications are allowed
+they are specified as a dictionary, like in the following example:
+
+.. code-block:: yaml
+
+   environment:
+     set:
+       LICENSE_FILE: '/path/to/license'
+     unset:
+     - CPATH
+     - LIBRARY_PATH
+     append_path:
+       PATH: '/new/bin/dir'
+
+The possible actions that are permitted are ``set``, ``unset``, ``append_path``,
+``prepend_path`` and finally ``remove_path``. They all require a dictionary
+of variable names mapped to the values used for the modification.
+The only exception is ``unset`` that requires just a list of variable names.
+No particular order is ensured on the execution of each of these modifications.
+
 ----------------------------
 Seeing Spack's Configuration
 ----------------------------
diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index f5f55cfd41..4809a1010e 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -39,7 +39,6 @@ import shutil
 import sys
 import traceback
 import types
-from six import iteritems
 from six import StringIO
 
 import llnl.util.tty as tty
@@ -52,6 +51,7 @@ import spack.build_systems.meson
 import spack.config
 import spack.main
 import spack.paths
+import spack.schema.environment
 import spack.store
 from spack.util.string import plural
 from spack.util.environment import (
@@ -342,21 +342,7 @@ def set_build_environment_variables(pkg, env, dirty):
     # Set environment variables if specified for
     # the given compiler
     compiler = pkg.compiler
-    environment = compiler.environment
-
-    for command, variable in iteritems(environment):
-        if command == 'set':
-            for name, value in iteritems(variable):
-                env.set(name, value)
-        elif command == 'unset':
-            for name, _ in iteritems(variable):
-                env.unset(name)
-        elif command == 'prepend-path':
-            for name, value in iteritems(variable):
-                env.prepend_path(name, value)
-        elif command == 'append-path':
-            for name, value in iteritems(variable):
-                env.append_path(name, value)
+    env.extend(spack.schema.environment.parse(compiler.environment))
 
     if compiler.extra_rpaths:
         extra_rpaths = ':'.join(compiler.extra_rpaths)
diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py
index 517c9a0270..d9407a1bf6 100644
--- a/lib/spack/spack/modules/common.py
+++ b/lib/spack/spack/modules/common.py
@@ -28,26 +28,24 @@ and is divided among four classes:
 Each of the four classes needs to be sub-classed when implementing a new
 module type.
 """
+import collections
 import copy
 import datetime
 import inspect
 import os.path
 import re
-import collections
 
-import six
 import llnl.util.filesystem
 import llnl.util.tty as tty
-
-import spack.paths
 import spack.build_environment as build_environment
-import spack.util.environment
+import spack.error
+import spack.paths
+import spack.schema.environment
 import spack.tengine as tengine
-import spack.util.path
 import spack.util.environment
-import spack.error
-import spack.util.spack_yaml as syaml
 import spack.util.file_permissions as fp
+import spack.util.path
+import spack.util.spack_yaml as syaml
 
 
 #: config section for this file
@@ -415,22 +413,7 @@ class BaseConfiguration(object):
         """List of environment modifications that should be done in the
         module.
         """
-        env_mods = spack.util.environment.EnvironmentModifications()
-        actions = self.conf.get('environment', {})
-
-        def process_arglist(arglist):
-            if method == 'unset':
-                for x in arglist:
-                    yield (x,)
-            else:
-                for x in six.iteritems(arglist):
-                    yield x
-
-        for method, arglist in actions.items():
-            for args in process_arglist(arglist):
-                getattr(env_mods, method)(*args)
-
-        return env_mods
+        return spack.schema.environment.parse(self.conf.get('environment', {}))
 
     @property
     def suffixes(self):
diff --git a/lib/spack/spack/schema/compilers.py b/lib/spack/spack/schema/compilers.py
index ffec5f8072..c994bef819 100644
--- a/lib/spack/spack/schema/compilers.py
+++ b/lib/spack/spack/schema/compilers.py
@@ -8,7 +8,7 @@
 .. literalinclude:: _spack_root/lib/spack/spack/schema/compilers.py
    :lines: 13-
 """
-
+import spack.schema.environment
 
 #: Properties for inclusion in other schemas
 properties = {
@@ -68,50 +68,7 @@ properties = {
                                 {'type': 'boolean'}
                             ]
                         },
-                        'environment': {
-                            'type': 'object',
-                            'default': {},
-                            'additionalProperties': False,
-                            'properties': {
-                                'set': {
-                                    'type': 'object',
-                                    'patternProperties': {
-                                        # Variable name
-                                        r'\w[\w-]*': {
-                                            'anyOf': [{'type': 'string'},
-                                                      {'type': 'number'}]
-                                        }
-                                    }
-                                },
-                                'unset': {
-                                    'type': 'object',
-                                    'patternProperties': {
-                                        # Variable name
-                                        r'\w[\w-]*': {'type': 'null'}
-                                    }
-                                },
-                                'prepend-path': {
-                                    'type': 'object',
-                                    'patternProperties': {
-                                        # Variable name
-                                        r'\w[\w-]*': {
-                                            'anyOf': [{'type': 'string'},
-                                                      {'type': 'number'}]
-                                        }
-                                    }
-                                },
-                                'append-path': {
-                                    'type': 'object',
-                                    'patternProperties': {
-                                        # Variable name
-                                        r'\w[\w-]*': {
-                                            'anyOf': [{'type': 'string'},
-                                                      {'type': 'number'}]
-                                        }
-                                    }
-                                }
-                            }
-                        },
+                        'environment': spack.schema.environment.definition,
                         'extra_rpaths': {
                             'type': 'array',
                             'default': [],
diff --git a/lib/spack/spack/schema/environment.py b/lib/spack/spack/schema/environment.py
new file mode 100644
index 0000000000..d251aeba96
--- /dev/null
+++ b/lib/spack/spack/schema/environment.py
@@ -0,0 +1,58 @@
+# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+"""Schema for environment modifications. Meant for inclusion in other
+schemas.
+"""
+
+array_of_strings_or_num = {
+    'type': 'array', 'default': [], 'items':
+        {'anyOf': [{'type': 'string'}, {'type': 'number'}]}
+}
+
+dictionary_of_strings_or_num = {
+    'type': 'object', 'patternProperties':
+        {r'\w[\w-]*': {'anyOf': [{'type': 'string'}, {'type': 'number'}]}}
+}
+
+definition = {
+    'type': 'object',
+    'default': {},
+    'additionalProperties': False,
+    'properties': {
+        'set': dictionary_of_strings_or_num,
+        'unset': array_of_strings_or_num,
+        'prepend_path': dictionary_of_strings_or_num,
+        'append_path': dictionary_of_strings_or_num,
+        'remove_path': dictionary_of_strings_or_num
+    }
+}
+
+
+def parse(config_obj):
+    """Returns an EnvironmentModifications object containing the modifications
+    parsed from input.
+
+    Args:
+        config_obj: a configuration dictionary conforming to the
+            schema definition for environment modifications
+    """
+    import spack.util.environment as ev
+    try:
+        from collections import Sequence  # novm
+    except ImportError:
+        from collections.abc import Sequence  # novm
+
+    env = ev.EnvironmentModifications()
+    for command, variable in config_obj.items():
+        # Distinguish between commands that take only a name as argument
+        # (e.g. unset) and commands that take a name and a value.
+        if isinstance(variable, Sequence):
+            for name in variable:
+                getattr(env, command)(name)
+        else:
+            for name, value in variable.items():
+                getattr(env, command)(name, value)
+
+    return env
diff --git a/lib/spack/spack/schema/modules.py b/lib/spack/spack/schema/modules.py
index 7fa2909d40..1fbbf614c8 100644
--- a/lib/spack/spack/schema/modules.py
+++ b/lib/spack/spack/schema/modules.py
@@ -8,6 +8,8 @@
 .. literalinclude:: _spack_root/lib/spack/spack/schema/modules.py
    :lines: 13-
 """
+import spack.schema.environment
+
 
 #: Matches a spec or a multi-valued variant but not another
 #: valid keyword.
@@ -66,17 +68,7 @@ module_file_configuration = {
                 }
             }
         },
-        'environment': {
-            'type': 'object',
-            'default': {},
-            'additionalProperties': False,
-            'properties': {
-                'set': dictionary_of_strings,
-                'unset': array_of_strings,
-                'prepend_path': dictionary_of_strings,
-                'append_path': dictionary_of_strings
-            }
-        }
+        'environment': spack.schema.environment.definition
     }
 }
 
diff --git a/lib/spack/spack/test/build_environment.py b/lib/spack/spack/test/build_environment.py
index 4d0f09c50f..a7a55af0ca 100644
--- a/lib/spack/spack/test/build_environment.py
+++ b/lib/spack/spack/test/build_environment.py
@@ -14,7 +14,6 @@ import spack.spec
 from spack.paths import build_env_path
 from spack.build_environment import dso_suffix, _static_to_shared_library
 from spack.util.executable import Executable
-from spack.util.spack_yaml import syaml_dict, syaml_str
 from spack.util.environment import EnvironmentModifications
 
 from llnl.util.filesystem import LibraryList, HeaderList
@@ -65,6 +64,18 @@ def build_environment(working_env):
         del os.environ[name]
 
 
+@pytest.fixture
+def ensure_env_variables(config, mock_packages, monkeypatch, working_env):
+    """Returns a function that takes a dictionary and updates os.environ
+    for the test lifetime accordingly. Plugs-in mock config and repo.
+    """
+    def _ensure(env_mods):
+        for name, value in env_mods.items():
+            monkeypatch.setenv(name, value)
+
+    return _ensure
+
+
 def test_static_to_shared_library(build_environment):
     os.environ['SPACK_TEST_COMMAND'] = 'dump-args'
 
@@ -119,79 +130,58 @@ def test_cc_not_changed_by_modules(monkeypatch, working_env):
     assert os.environ['ANOTHER_VAR'] == 'THIS_IS_SET'
 
 
-@pytest.mark.usefixtures('config', 'mock_packages')
-def test_compiler_config_modifications(monkeypatch, working_env):
-    s = spack.spec.Spec('cmake')
-    s.concretize()
-    pkg = s.package
-
-    os.environ['SOME_VAR_STR'] = ''
-    os.environ['SOME_VAR_NUM'] = '0'
-    os.environ['PATH_LIST'] = '/path/third:/path/forth'
-    os.environ['EMPTY_PATH_LIST'] = ''
-    os.environ.pop('NEW_PATH_LIST', None)
-
-    env_mod = syaml_dict()
-    set_cmd = syaml_dict()
-    env_mod[syaml_str('set')] = set_cmd
-
-    set_cmd[syaml_str('SOME_VAR_STR')] = syaml_str('SOME_STR')
-    set_cmd[syaml_str('SOME_VAR_NUM')] = 1
-
-    monkeypatch.setattr(pkg.compiler, 'environment', env_mod)
-    spack.build_environment.setup_package(pkg, False)
-    assert os.environ['SOME_VAR_STR'] == 'SOME_STR'
-    assert os.environ['SOME_VAR_NUM'] == str(1)
-
-    env_mod = syaml_dict()
-    unset_cmd = syaml_dict()
-    env_mod[syaml_str('unset')] = unset_cmd
-
-    unset_cmd[syaml_str('SOME_VAR_STR')] = None
-
-    monkeypatch.setattr(pkg.compiler, 'environment', env_mod)
-    assert 'SOME_VAR_STR' in os.environ
-    spack.build_environment.setup_package(pkg, False)
-    assert 'SOME_VAR_STR' not in os.environ
-
-    env_mod = syaml_dict()
-    set_cmd = syaml_dict()
-    env_mod[syaml_str('set')] = set_cmd
-    append_cmd = syaml_dict()
-    env_mod[syaml_str('append-path')] = append_cmd
-    unset_cmd = syaml_dict()
-    env_mod[syaml_str('unset')] = unset_cmd
-    prepend_cmd = syaml_dict()
-    env_mod[syaml_str('prepend-path')] = prepend_cmd
-
-    set_cmd[syaml_str('EMPTY_PATH_LIST')] = syaml_str('/path/middle')
-
-    append_cmd[syaml_str('PATH_LIST')] = syaml_str('/path/last')
-    append_cmd[syaml_str('EMPTY_PATH_LIST')] = syaml_str('/path/last')
-    append_cmd[syaml_str('NEW_PATH_LIST')] = syaml_str('/path/last')
-
-    unset_cmd[syaml_str('SOME_VAR_NUM')] = None
+@pytest.mark.parametrize('initial,modifications,expected', [
+    # Set and unset variables
+    ({'SOME_VAR_STR': '', 'SOME_VAR_NUM': '0'},
+     {'set': {'SOME_VAR_STR': 'SOME_STR', 'SOME_VAR_NUM': 1}},
+     {'SOME_VAR_STR': 'SOME_STR', 'SOME_VAR_NUM': '1'}),
+    ({'SOME_VAR_STR': ''},
+     {'unset': ['SOME_VAR_STR']},
+     {'SOME_VAR_STR': None}),
+    ({},  # Set a variable that was not defined already
+     {'set': {'SOME_VAR_STR': 'SOME_STR'}},
+     {'SOME_VAR_STR': 'SOME_STR'}),
+    # Append and prepend to the same variable
+    ({'EMPTY_PATH_LIST': '/path/middle'},
+     {'prepend_path': {'EMPTY_PATH_LIST': '/path/first'},
+      'append_path': {'EMPTY_PATH_LIST': '/path/last'}},
+     {'EMPTY_PATH_LIST': '/path/first:/path/middle:/path/last'}),
+    # Append and prepend from empty variables
+    ({'EMPTY_PATH_LIST': '', 'SOME_VAR_STR': ''},
+     {'prepend_path': {'EMPTY_PATH_LIST': '/path/first'},
+      'append_path': {'SOME_VAR_STR': '/path/last'}},
+     {'EMPTY_PATH_LIST': '/path/first', 'SOME_VAR_STR': '/path/last'}),
+    ({},  # Same as before but on variables that were not defined
+     {'prepend_path': {'EMPTY_PATH_LIST': '/path/first'},
+      'append_path': {'SOME_VAR_STR': '/path/last'}},
+     {'EMPTY_PATH_LIST': '/path/first', 'SOME_VAR_STR': '/path/last'}),
+    # Remove a path from a list
+    ({'EMPTY_PATH_LIST': '/path/first:/path/middle:/path/last'},
+     {'remove_path': {'EMPTY_PATH_LIST': '/path/middle'}},
+     {'EMPTY_PATH_LIST': '/path/first:/path/last'}),
+    ({'EMPTY_PATH_LIST': '/only/path'},
+     {'remove_path': {'EMPTY_PATH_LIST': '/only/path'}},
+     {'EMPTY_PATH_LIST': ''}),
+])
+def test_compiler_config_modifications(
+        initial, modifications, expected, ensure_env_variables, monkeypatch
+):
+    # Set the environment as per prerequisites
+    ensure_env_variables(initial)
 
-    prepend_cmd[syaml_str('PATH_LIST')] = syaml_str('/path/first:/path/second')
-    prepend_cmd[syaml_str('EMPTY_PATH_LIST')] = syaml_str('/path/first')
-    prepend_cmd[syaml_str('NEW_PATH_LIST')] = syaml_str('/path/first')
-    prepend_cmd[syaml_str('SOME_VAR_NUM')] = syaml_str('/8')
+    # Monkeypatch a pkg.compiler.environment with the required modifications
+    pkg = spack.spec.Spec('cmake').concretized().package
+    monkeypatch.setattr(pkg.compiler, 'environment', modifications)
 
-    assert 'SOME_VAR_NUM' in os.environ
-    monkeypatch.setattr(pkg.compiler, 'environment', env_mod)
+    # Trigger the modifications
     spack.build_environment.setup_package(pkg, False)
-    # Check that the order of modifications is respected and the
-    # variable was unset before it was prepended.
-    assert os.environ['SOME_VAR_NUM'] == '/8'
-
-    expected = '/path/first:/path/second:/path/third:/path/forth:/path/last'
-    assert os.environ['PATH_LIST'] == expected
-
-    expected = '/path/first:/path/middle:/path/last'
-    assert os.environ['EMPTY_PATH_LIST'] == expected
 
-    expected = '/path/first:/path/last'
-    assert os.environ['NEW_PATH_LIST'] == expected
+    # Check they were applied
+    for name, value in expected.items():
+        if value is not None:
+            assert os.environ[name] == value
+            continue
+        assert name not in os.environ
 
 
 @pytest.mark.regression('9107')