Allow packages to control handling of compiler flags (#4421)

* Initial work on flag trapping using functions called <flag>_handler and default_flag_handler

* Update packages so they do not obliterate flags

* Added append to EnvironmentModifications class

* changed EnvironmentModifications to have append_flags method

* changed flag_val to be a tuple

* Increased test coverage

* added documentation of flag handling

6 files changed, 169 insertions, 0 deletions

diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index 540d2a9e4e..b90a0eea17 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/lib/spack/docs/packaging_guide.rst
@@ -2390,6 +2390,94 @@ build system.
 Compiler flags
 ^^^^^^^^^^^^^^
 
+Compiler flags set by the user through the Spec object can be passed to
+the build in one of two ways. For packages inheriting from the
+``CmakePackage`` or ``AutotoolsPackage`` classes, the build environment
+passes those flags to the relevant environment variables (``CFLAGS``,
+``CXXFLAGS``, etc) that are respected by the build system. For all other
+packages, the default behavior is to inject the flags directly into the
+compiler commands using Spack's compiler wrappers.
+
+Individual packages can override the default behavior for the flag
+handling.  Packages can define a ``default_flag_handler`` method that
+applies to all sets of flags handled by Spack, or may define
+individual methods ``cflags_handler``, ``cxxflags_handler``,
+etc. Spack will apply the individual method for a flag set if it
+exists, otherwise the ``default_flag_handler`` method if it exists,
+and fall back on the default for that package class if neither exists.
+
+These methods are defined on the package class, and take two
+parameters in addition to the packages itself. The ``env`` parameter
+is an ``EnvironmentModifications`` object that can be used to change
+the build environment. The ``flag_val`` parameter is a tuple. Its
+first entry is the name of the flag (``cflags``, ``cxxflags``, etc.)
+and its second entry is a list of the values for that flag.
+
+There are three primary idioms that can be combined to create whatever
+behavior the package requires.
+
+1. The default behavior for packages inheriting from
+``AutotoolsPackage`` or ``CmakePackage``.
+
+.. code-block:: python
+
+    def default_flag_handler(self, env, flag_val):
+        env.append_flags(flag_val[0].upper(), ' '.join(flag_val[1]))
+        return []
+
+2. The default behavior for other packages
+
+.. code-block:: python
+
+    def default_flag_handler(self, env, flag_val):
+        return flag_val[1]
+
+
+3. Packages may have additional flags to add to the build. These flags
+can be added to either idiom above. For example:
+
+.. code-block:: python
+
+    def default_flag_handler(self, env, flag_val):
+        flags = flag_val[1]
+        flags.append('-flag')
+        return flags
+
+or
+
+.. code-block:: python
+
+    def default_flag_handler(self, env, flag_val):
+        env.append_flags(flag_val[0].upper(), ' '.join(flag_val[1]))
+        env.append_flags(flag_val[0].upper(), '-flag')
+        return []
+
+Packages may also opt for methods that include aspects of any of the
+idioms above. E.g.
+
+.. code-block:: python
+
+    def default_flag_handler(self, env, flag_val):
+        flags = []
+        if len(flag_val[1]) > 3:
+            env.append_flags(flag_val[0].upper(), ' '.join(flag_val[1][3:]))
+            flags = flag_val[1][:3]
+        else:
+            flags = flag_val[1]
+        flags.append('-flag')
+        return flags
+
+Because these methods can pass values through environment variables,
+it is important not to override these variables unnecessarily in other
+package methods. In the ``setup_environment`` and
+``setup_dependent_environment`` methods, use the ``append_flags``
+method of the ``EnvironmentModifications`` class to append values to a
+list of flags whenever there is no good reason to override the
+existing value. In the ``install`` method and other methods that can
+operate on the build environment directly through the ``env``
+variable, test for environment variable existance before overriding
+values to add compiler flags.
+
 In rare circumstances such as compiling and running small unit tests, a
 package developer may need to know what are the appropriate compiler
 flags to enable features like ``OpenMP``, ``c++11``, ``c++14`` and
diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index 49fecdb59c..571f0f8c49 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -467,6 +467,19 @@ def setup_package(pkg, dirty=False):
     for s in pkg.spec.traverse():
         s.package.spec = s
 
+    # Trap spack-tracked compiler flags as appropriate.
+    # Must be before set_compiler_environment_variables
+    # Current implementation of default flag handler relies on this being
+    # the first thing to affect the spack_env (so there is no appending), or
+    # on no other build_environment methods trying to affect these variables
+    # (CFLAGS, CXXFLAGS, etc). Currently both are true, either is sufficient.
+    for flag in spack.spec.FlagMap.valid_compiler_flags():
+        trap_func = getattr(pkg, flag + '_handler',
+                            getattr(pkg, 'default_flag_handler',
+                                    lambda x, y: y[1]))
+        flag_val = pkg.spec.compiler_flags[flag]
+        pkg.spec.compiler_flags[flag] = trap_func(spack_env, (flag, flag_val))
+
     set_compiler_environment_variables(pkg, spack_env)
     set_build_environment_variables(pkg, spack_env, dirty)
     pkg.architecture.platform.setup_platform_environment(pkg, spack_env)
diff --git a/lib/spack/spack/build_systems/autotools.py b/lib/spack/spack/build_systems/autotools.py
index 84f019f1b3..1ef38ddc2d 100644
--- a/lib/spack/spack/build_systems/autotools.py
+++ b/lib/spack/spack/build_systems/autotools.py
@@ -181,6 +181,29 @@ class AutotoolsPackage(PackageBase):
         """Override to provide another place to build the package"""
         return self.configure_directory
 
+    def default_flag_handler(self, spack_env, flag_val):
+        # Relies on being the first thing that can affect the spack_env
+        # EnvironmentModification after it is instantiated or no other
+        # method trying to affect these variables. Currently both are true
+        # flag_val is a tuple (flag, value_list).
+        spack_env.set(flag_val[0].upper(),
+                      ' '.join(flag_val[1]))
+        return []
+
+    def patch(self):
+        """Patches config.guess if
+        :py:attr:``~.AutotoolsPackage.patch_config_guess`` is True
+
+        :raise RuntimeError: if something goes wrong when patching
+            ``config.guess``
+        """
+
+        if self.patch_config_guess and self.spec.satisfies(
+                'arch=linux-rhel7-ppc64le'
+        ):
+            if not self._do_patch_config_guess():
+                raise RuntimeError('Failed to find suitable config.guess')
+
     @run_before('autoreconf')
     def delete_configure_to_force_update(self):
         if self.force_autoreconf:
diff --git a/lib/spack/spack/build_systems/cmake.py b/lib/spack/spack/build_systems/cmake.py
index 240c8a8b18..b339858348 100644
--- a/lib/spack/spack/build_systems/cmake.py
+++ b/lib/spack/spack/build_systems/cmake.py
@@ -132,6 +132,15 @@ class CMakePackage(PackageBase):
         """
         return join_path(self.stage.source_path, 'spack-build')
 
+    def default_flag_handler(self, spack_env, flag_val):
+        # Relies on being the first thing that can affect the spack_env
+        # EnvironmentModification after it is instantiated or no other
+        # method trying to affect these variables. Currently both are true
+        # flag_val is a tuple (flag, value_list)
+        spack_env.set(flag_val[0].upper(),
+                      ' '.join(flag_val[1]))
+        return []
+
     def cmake_args(self):
         """Produces a list containing all the arguments that must be passed to
         cmake, except:
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index bb760bfd2f..ff278dd6b6 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -63,6 +63,15 @@ class SetEnv(NameValueModifier):
         os.environ[self.name] = str(self.value)
 
 
+class AppendFlagsEnv(NameValueModifier):
+
+    def execute(self):
+        if self.name in os.environ and os.environ[self.name]:
+            os.environ[self.name] += self.separator + str(self.value)
+        else:
+            os.environ[self.name] = str(self.value)
+
+
 class UnsetEnv(NameModifier):
 
     def execute(self):
@@ -171,6 +180,20 @@ class EnvironmentModifications(object):
         item = SetEnv(name, value, **kwargs)
         self.env_modifications.append(item)
 
+    def append_flags(self, name, value, sep=' ', **kwargs):
+        """
+        Stores in the current object a request to append to an env variable
+
+        Args:
+            name: name of the environment variable to be appended to
+            value: value to append to the environment variable
+        Appends with spaces separating different additions to the variable
+        """
+        kwargs.update(self._get_outside_caller_attributes())
+        kwargs.update({'separator': sep})
+        item = AppendFlagsEnv(name, value, **kwargs)
+        self.env_modifications.append(item)
+
     def unset(self, name, **kwargs):
         """
         Stores in the current object a request to unset an environment variable
diff --git a/lib/spack/spack/test/environment.py b/lib/spack/spack/test/environment.py
index 671d6ee6c9..7f3b7bf2bd 100644
--- a/lib/spack/spack/test/environment.py
+++ b/lib/spack/spack/test/environment.py
@@ -110,6 +110,19 @@ def test_set(env):
     assert str(3) == os.environ['B']
 
 
+def test_append_flags(env):
+    """Tests appending to a value in the environment."""
+
+    # Store a couple of commands
+    env.append_flags('APPEND_TO_ME', 'flag1')
+    env.append_flags('APPEND_TO_ME', 'flag2')
+
+    # ... execute the commands
+    env.apply_modifications()
+
+    assert 'flag1 flag2' == os.environ['APPEND_TO_ME']
+
+
 def test_unset(env):
     """Tests unsetting values in the environment."""