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

16 files changed, 203 insertions, 12 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."""
 
diff --git a/var/spack/repos/builtin/packages/clhep/package.py b/var/spack/repos/builtin/packages/clhep/package.py
index e9bf85c97f..7120fffac6 100644
--- a/var/spack/repos/builtin/packages/clhep/package.py
+++ b/var/spack/repos/builtin/packages/clhep/package.py
@@ -78,12 +78,18 @@ class Clhep(CMakePackage):
         cmake_args = []
 
         if '+cxx11' in spec:
-            env['CXXFLAGS'] = self.compiler.cxx11_flag
+            if 'CXXFLAGS' in env and env['CXXFLAGS']:
+                env['CXXFLAGS'] += ' ' + self.compiler.cxx11_flag
+            else:
+                env['CXXFLAGS'] = self.compiler.cxx11_flag
             cmake_args.append('-DCLHEP_BUILD_CXXSTD=' +
                               self.compiler.cxx11_flag)
 
         if '+cxx14' in spec:
-            env['CXXFLAGS'] = self.compiler.cxx14_flag
+            if 'CXXFLAGS' in env and env['CXXFLAGS']:
+                env['CXXFLAGS'] += ' ' + self.compiler.cxx14_flag
+            else:
+                env['CXXFLAGS'] = self.compiler.cxx14_flag
             cmake_args.append('-DCLHEP_BUILD_CXXSTD=' +
                               self.compiler.cxx14_flag)
 
diff --git a/var/spack/repos/builtin/packages/elpa/package.py b/var/spack/repos/builtin/packages/elpa/package.py
index 85d1706b2b..61e957c0c9 100644
--- a/var/spack/repos/builtin/packages/elpa/package.py
+++ b/var/spack/repos/builtin/packages/elpa/package.py
@@ -71,8 +71,8 @@ class Elpa(AutotoolsPackage):
         spack_env.set('FC', spec['mpi'].mpifc)
         spack_env.set('CXX', spec['mpi'].mpicxx)
 
-        spack_env.set('LDFLAGS', spec['lapack'].libs.search_flags)
-        spack_env.set('LIBS', spec['lapack'].libs.link_flags)
+        spack_env.append_flags('LDFLAGS', spec['lapack'].libs.search_flags)
+        spack_env.append_flags('LIBS', spec['lapack'].libs.link_flags)
         spack_env.set('SCALAPACK_LDFLAGS', spec['scalapack'].libs.joined())
 
     def configure_args(self):
diff --git a/var/spack/repos/builtin/packages/ferret/package.py b/var/spack/repos/builtin/packages/ferret/package.py
index f2a32fdd70..6a4c0902f6 100644
--- a/var/spack/repos/builtin/packages/ferret/package.py
+++ b/var/spack/repos/builtin/packages/ferret/package.py
@@ -98,7 +98,12 @@ class Ferret(Package):
         ln('-sf',
            libz_prefix + '/lib',
            libz_prefix + '/lib64')
-        os.environ['LDFLAGS'] = '-lquadmath'
+
+        if 'LDFLAGS' in env and env['LDFLAGS']:
+            env['LDFLAGS'] += ' ' + '-lquadmath'
+        else:
+            env['LDFLAGS'] = '-lquadmath'
+
         with working_dir('FERRET', create=False):
             os.environ['LD_X11'] = '-L/usr/lib/X11 -lX11'
             os.environ['HOSTTYPE'] = 'x86_64-linux'
diff --git a/var/spack/repos/builtin/packages/git/package.py b/var/spack/repos/builtin/packages/git/package.py
index f01cc37d7b..9dc9e460af 100644
--- a/var/spack/repos/builtin/packages/git/package.py
+++ b/var/spack/repos/builtin/packages/git/package.py
@@ -155,7 +155,7 @@ class Git(AutotoolsPackage):
     depends_on('m4',       type='build')
 
     def setup_environment(self, spack_env, run_env):
-        spack_env.set('LDFLAGS', '-L{0} -lintl'.format(
+        spack_env.append_flags('LDFLAGS', '-L{0} -lintl'.format(
             self.spec['gettext'].prefix.lib))
 
     def configure_args(self):
diff --git a/var/spack/repos/builtin/packages/libint/package.py b/var/spack/repos/builtin/packages/libint/package.py
index d267a8ea88..34600e742a 100644
--- a/var/spack/repos/builtin/packages/libint/package.py
+++ b/var/spack/repos/builtin/packages/libint/package.py
@@ -85,6 +85,7 @@ class Libint(AutotoolsPackage):
     def configure_args(self):
 
         config_args = ['--enable-shared']
+
         optflags = self.optflags
 
         # Optimization flag names have changed in libint 2
diff --git a/var/spack/repos/builtin/packages/libxc/package.py b/var/spack/repos/builtin/packages/libxc/package.py
index f018bacfa3..e2fe25c455 100644
--- a/var/spack/repos/builtin/packages/libxc/package.py
+++ b/var/spack/repos/builtin/packages/libxc/package.py
@@ -71,8 +71,15 @@ class Libxc(Package):
             if which('xiar'):
                 env['AR'] = 'xiar'
 
-        env['CFLAGS']  = optflags
-        env['FCFLAGS'] = optflags
+        if 'CFLAGS' in env and env['CFLAGS']:
+            env['CFLAGS'] += ' ' + optflags
+        else:
+            env['CFLAGS'] = optflags
+
+        if 'FCFLAGS' in env and env['FCFLAGS']:
+            env['FCFLAGS'] += ' ' + optflags
+        else:
+            env['FCFLAGS'] = optflags
 
         configure('--prefix={0}'.format(prefix),
                   '--enable-shared')
diff --git a/var/spack/repos/builtin/packages/libxpm/package.py b/var/spack/repos/builtin/packages/libxpm/package.py
index 09d0b0ec61..40234b1b2d 100644
--- a/var/spack/repos/builtin/packages/libxpm/package.py
+++ b/var/spack/repos/builtin/packages/libxpm/package.py
@@ -46,5 +46,5 @@ class Libxpm(AutotoolsPackage):
     depends_on('util-macros', type='build')
 
     def setup_environment(self, spack_env, run_env):
-        spack_env.set('LDFLAGS', '-L{0} -lintl'.format(
+        spack_env.append_flags('LDFLAGS', '-L{0} -lintl'.format(
             self.spec['gettext'].prefix.lib))
diff --git a/var/spack/repos/builtin/packages/llvm-lld/package.py b/var/spack/repos/builtin/packages/llvm-lld/package.py
index 4624451e69..0ebe2e6b89 100644
--- a/var/spack/repos/builtin/packages/llvm-lld/package.py
+++ b/var/spack/repos/builtin/packages/llvm-lld/package.py
@@ -38,7 +38,10 @@ class LlvmLld(Package):
     depends_on('cmake', type='build')
 
     def install(self, spec, prefix):
-        env['CXXFLAGS'] = self.compiler.cxx11_flag
+        if 'CXXFLAGS' in env and env['CXXFLAGS']:
+            env['CXXFLAGS'] += ' ' + self.compiler.cxx11_flag
+        else:
+            env['CXXFLAGS'] = self.compiler.cxx11_flag
 
         with working_dir('spack-build', create=True):
             cmake('..',
diff --git a/var/spack/repos/builtin/packages/llvm/package.py b/var/spack/repos/builtin/packages/llvm/package.py
index 1f88d94882..fc8823f1a8 100644
--- a/var/spack/repos/builtin/packages/llvm/package.py
+++ b/var/spack/repos/builtin/packages/llvm/package.py
@@ -325,7 +325,7 @@ class Llvm(CMakePackage):
     conflicts('+lldb',        when='~clang')
 
     def setup_environment(self, spack_env, run_env):
-        spack_env.set('CXXFLAGS', self.compiler.cxx11_flag)
+        spack_env.append_flags('CXXFLAGS', self.compiler.cxx11_flag)
 
     def build_type(self):
         if '+debug' in self.spec:
diff --git a/var/spack/repos/builtin/packages/zlib/package.py b/var/spack/repos/builtin/packages/zlib/package.py
index 0d9822f287..30fcef95e1 100644
--- a/var/spack/repos/builtin/packages/zlib/package.py
+++ b/var/spack/repos/builtin/packages/zlib/package.py
@@ -57,7 +57,7 @@ class Zlib(Package):
 
     def setup_environment(self, spack_env, run_env):
         if '+pic' in self.spec:
-            spack_env.set('CFLAGS', self.compiler.pic_flag)
+            spack_env.append_flags('CFLAGS', self.compiler.pic_flag)
 
     def install(self, spec, prefix):
         config_args = []