unparser: handle package-level loops, if statements, and with blocks

Many packages implement logic at the class level to handle complex dependencies and
conflicts. Others have started using `with when("@1.0"):` blocks since we added that
capability. The loops and other control logic can cause some pure directive logic not to
be removed by our package hashing logic -- and in many cases that's a lot of code that
will cause unnecessary rebuilds.

This commit changes the unparser so that it will descend into these blocks. Specifically:

  1. Descend into loops, if statements, and with blocks at the class level.
  2. Don't look inside function definitions (in or outside a class).
  3. Don't look at nested class definitions (they don't have directives)
  4. Add logic to *remove* empty loops/with blocks/if statements if all directives
     in them were removed.

This allows our package hash to ignore a lot of pure metadata that it was not ignoring
before, and makes it less sensitive.

In addition, we add `maintainers` and `tags` to the list of metadata attributes that
Spack should remove from packages when constructing canonoical source for a package
hash.

- [x] Make unparser handle if/for/while/with at class level.
- [x] Add tests for control logic removal.
- [x] Add a test to ensure that all packages are not only unparseable, but also
      that their canonical source is still compilable. This is a test for
      our control logic removal.
- [x] Add another unparse test package that has complex logic.

5 files changed, 1039 insertions, 74 deletions

diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 5c49ccc00c..eb3ff79a29 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -676,8 +676,17 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
     maintainers = []  # type: List[str]
 
     #: List of attributes to be excluded from a package's hash.
-    metadata_attrs = ['homepage', 'url', 'urls', 'list_url', 'extendable',
-                      'parallel', 'make_jobs']
+    metadata_attrs = [
+        "homepage",
+        "url",
+        "urls",
+        "list_url",
+        "extendable",
+        "parallel",
+        "make_jobs",
+        "maintainers",
+        "tags",
+    ]
 
     #: Boolean. If set to ``True``, the smoke/install test requires a compiler.
     #: This is currently used by smoke tests to ensure a compiler is available
diff --git a/lib/spack/spack/test/data/unparse/trilinos.txt b/lib/spack/spack/test/data/unparse/trilinos.txt
new file mode 100644
index 0000000000..ae60c2088c
--- /dev/null
+++ b/lib/spack/spack/test/data/unparse/trilinos.txt
@@ -0,0 +1,786 @@
+ # -*- python -*-
+# Copyright 2013-2021 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)
+"""This is an unparser test package.
+
+``trilinos`` was chosen because it's one of the most complex packages in Spack, because
+it has a lot of nested  ``with when():`` blocks, and because it has loops and nested
+logic at the package level.
+
+"""
+
+import os
+import sys
+
+from spack import *
+from spack.build_environment import dso_suffix
+from spack.error import NoHeadersError
+from spack.operating_systems.mac_os import macos_version
+from spack.pkg.builtin.kokkos import Kokkos
+
+# Trilinos is complicated to build, as an inspiration a couple of links to
+# other repositories which build it:
+# https://github.com/hpcugent/easybuild-easyblocks/blob/master/easybuild/easyblocks/t/trilinos.py#L111
+# https://github.com/koecher/candi/blob/master/deal.II-toolchain/packages/trilinos.package
+# https://gitlab.com/configurations/cluster-config/blob/master/trilinos.sh
+# https://github.com/Homebrew/homebrew-science/blob/master/trilinos.rb and some
+# relevant documentation/examples:
+# https://github.com/trilinos/Trilinos/issues/175
+
+
+class Trilinos(CMakePackage, CudaPackage):
+    """The Trilinos Project is an effort to develop algorithms and enabling
+    technologies within an object-oriented software framework for the solution
+    of large-scale, complex multi-physics engineering and scientific problems.
+    A unique design feature of Trilinos is its focus on packages.
+    """
+    homepage = "https://trilinos.org/"
+    url      = "https://github.com/trilinos/Trilinos/archive/trilinos-release-12-12-1.tar.gz"
+    git      = "https://github.com/trilinos/Trilinos.git"
+
+    maintainers = ['keitat', 'sethrj', 'kuberry']
+
+    tags = ['e4s']
+
+    # ###################### Versions ##########################
+
+    version('master', branch='master')
+    version('develop', branch='develop')
+    version('13.2.0', commit='4a5f7906a6420ee2f9450367e9cc95b28c00d744')  # tag trilinos-release-13-2-0
+    version('13.0.1', commit='4796b92fb0644ba8c531dd9953e7a4878b05c62d', preferred=True)  # tag trilinos-release-13-0-1
+    version('13.0.0', commit='9fec35276d846a667bc668ff4cbdfd8be0dfea08')  # tag trilinos-release-13-0-0
+    version('12.18.1', commit='55a75997332636a28afc9db1aee4ae46fe8d93e7')  # tag trilinos-release-12-8-1
+    version('12.14.1', sha256='52a4406cca2241f5eea8e166c2950471dd9478ad6741cbb2a7fc8225814616f0')
+    version('12.12.1', sha256='5474c5329c6309224a7e1726cf6f0d855025b2042959e4e2be2748bd6bb49e18')
+    version('12.10.1', sha256='ab81d917196ffbc21c4927d42df079dd94c83c1a08bda43fef2dd34d0c1a5512')
+    version('12.8.1', sha256='d20fe60e31e3ba1ef36edecd88226240a518f50a4d6edcc195b88ee9dda5b4a1')
+    version('12.6.4', sha256='1c7104ba60ee8cc4ec0458a1c4f6a26130616bae7580a7b15f2771a955818b73')
+    version('12.6.3', sha256='4d28298bb4074eef522db6cd1626f1a934e3d80f292caf669b8846c0a458fe81')
+    version('12.6.2', sha256='8be7e3e1166cc05aea7f856cc8033182e8114aeb8f87184cb38873bfb2061779')
+    version('12.6.1', sha256='4b38ede471bed0036dcb81a116fba8194f7bf1a9330da4e29c3eb507d2db18db')
+    version('12.4.2', sha256='fd2c12e87a7cedc058bcb8357107ffa2474997aa7b17b8e37225a1f7c32e6f0e')
+    version('12.2.1', sha256='088f303e0dc00fb4072b895c6ecb4e2a3ad9a2687b9c62153de05832cf242098')
+    version('12.0.1', sha256='eee7c19ca108538fa1c77a6651b084e06f59d7c3307dae77144136639ab55980')
+    version('11.14.3', sha256='e37fa5f69103576c89300e14d43ba77ad75998a54731008b25890d39892e6e60')
+    version('11.14.2', sha256='f22b2b0df7b88e28b992e19044ba72b845292b93cbbb3a948488199647381119')
+    version('11.14.1', sha256='f10fc0a496bf49427eb6871c80816d6e26822a39177d850cc62cf1484e4eec07')
+
+    # ###################### Variants ##########################
+
+    # Build options
+    variant('complex', default=False, description='Enable complex numbers in Trilinos')
+    variant('cuda_rdc', default=False, description='turn on RDC for CUDA build')
+    variant('cxxstd', default='14', values=['11', '14', '17'], multi=False)
+    variant('debug', default=False, description='Enable runtime safety and debug checks')
+    variant('explicit_template_instantiation', default=True, description='Enable explicit template instantiation (ETI)')
+    variant('float', default=False, description='Enable single precision (float) numbers in Trilinos')
+    variant('fortran', default=True, description='Compile with Fortran support')
+    variant('gotype', default='long_long',
+            values=('int', 'long', 'long_long', 'all'),
+            multi=False,
+            description='global ordinal type for Tpetra')
+    variant('openmp', default=False, description='Enable OpenMP')
+    variant('python', default=False, description='Build PyTrilinos wrappers')
+    variant('shared', default=True, description='Enables the build of shared libraries')
+    variant('wrapper', default=False, description="Use nvcc-wrapper for CUDA build")
+
+    # TPLs (alphabet order)
+    variant('adios2',       default=False, description='Enable ADIOS2')
+    variant('boost',        default=False, description='Compile with Boost')
+    variant('hdf5',         default=False, description='Compile with HDF5')
+    variant('hypre',        default=False, description='Compile with Hypre preconditioner')
+    variant('mpi',          default=True, description='Compile with MPI parallelism')
+    variant('mumps',        default=False, description='Compile with support for MUMPS solvers')
+    variant('suite-sparse', default=False, description='Compile with SuiteSparse solvers')
+    variant('superlu-dist', default=False, description='Compile with SuperluDist solvers')
+    variant('superlu',      default=False, description='Compile with SuperLU solvers')
+    variant('strumpack',    default=False, description='Compile with STRUMPACK solvers')
+    variant('x11',          default=False, description='Compile with X11 when +exodus')
+
+    # Package options (alphabet order)
+    variant('amesos',       default=True, description='Compile with Amesos')
+    variant('amesos2',      default=True, description='Compile with Amesos2')
+    variant('anasazi',      default=True, description='Compile with Anasazi')
+    variant('aztec',        default=True, description='Compile with Aztec')
+    variant('belos',        default=True, description='Compile with Belos')
+    variant('chaco',        default=False, description='Compile with Chaco from SEACAS')
+    variant('epetra',       default=True, description='Compile with Epetra')
+    variant('epetraext',    default=True, description='Compile with EpetraExt')
+    variant('exodus',       default=False, description='Compile with Exodus from SEACAS')
+    variant('ifpack',       default=True, description='Compile with Ifpack')
+    variant('ifpack2',      default=True, description='Compile with Ifpack2')
+    variant('intrepid',     default=False, description='Enable Intrepid')
+    variant('intrepid2',    default=False, description='Enable Intrepid2')
+    variant('isorropia',    default=False, description='Compile with Isorropia')
+    variant('gtest',        default=False, description='Build vendored Googletest')
+    variant('kokkos',       default=True, description='Compile with Kokkos')
+    variant('ml',           default=True, description='Compile with ML')
+    variant('minitensor',   default=False, description='Compile with MiniTensor')
+    variant('muelu',        default=True, description='Compile with Muelu')
+    variant('nox',          default=False, description='Compile with NOX')
+    variant('piro',         default=False, description='Compile with Piro')
+    variant('phalanx',      default=False, description='Compile with Phalanx')
+    variant('rol',          default=False, description='Compile with ROL')
+    variant('rythmos',      default=False, description='Compile with Rythmos')
+    variant('sacado',       default=True, description='Compile with Sacado')
+    variant('stk',          default=False, description='Compile with STK')
+    variant('shards',       default=False, description='Compile with Shards')
+    variant('shylu',        default=False, description='Compile with ShyLU')
+    variant('stokhos',      default=False, description='Compile with Stokhos')
+    variant('stratimikos',  default=False, description='Compile with Stratimikos')
+    variant('teko',         default=False, description='Compile with Teko')
+    variant('tempus',       default=False, description='Compile with Tempus')
+    variant('tpetra',       default=True, description='Compile with Tpetra')
+    variant('trilinoscouplings', default=False, description='Compile with TrilinosCouplings')
+    variant('zoltan',       default=False, description='Compile with Zoltan')
+    variant('zoltan2',      default=False, description='Compile with Zoltan2')
+
+    # Internal package options (alphabetical order)
+    variant('basker',                    default=False, description='Compile with the Basker solver in Amesos2')
+    variant('epetraextbtf',              default=False, description='Compile with BTF in EpetraExt')
+    variant('epetraextexperimental',     default=False, description='Compile with experimental in EpetraExt')
+    variant('epetraextgraphreorderings', default=False, description='Compile with graph reorderings in EpetraExt')
+
+    # External package options
+    variant('dtk',      default=False, description='Enable DataTransferKit (deprecated)')
+    variant('scorec',   default=False, description='Enable SCOREC')
+    variant('mesquite', default=False, description='Enable Mesquite (deprecated)')
+
+    resource(name='dtk',
+             git='https://github.com/ornl-cees/DataTransferKit.git',
+             commit='4fe4d9d56cfd4f8a61f392b81d8efd0e389ee764',  # branch dtk-3.0
+             placement='DataTransferKit',
+             when='+dtk @12.14.0:12.14')
+    resource(name='dtk',
+             git='https://github.com/ornl-cees/DataTransferKit.git',
+             commit='edfa050cd46e2274ab0a0b7558caca0079c2e4ca',  # tag 3.1-rc1
+             placement='DataTransferKit',
+             submodules=True,
+             when='+dtk @12.18.0:12.18')
+    resource(name='scorec',
+             git='https://github.com/SCOREC/core.git',
+             commit='73c16eae073b179e45ec625a5abe4915bc589af2',  # tag v2.2.5
+             placement='SCOREC',
+             when='+scorec')
+    resource(name='mesquite',
+             url='https://github.com/trilinos/mesquite/archive/trilinos-release-12-12-1.tar.gz',
+             sha256='e0d09b0939dbd461822477449dca611417316e8e8d8268fd795debb068edcbb5',
+             placement='packages/mesquite',
+             when='+mesquite @12.12.1:12.16')
+    resource(name='mesquite',
+             git='https://github.com/trilinos/mesquite.git',
+             commit='20a679679b5cdf15bf573d66c5dc2b016e8b9ca1',  # branch trilinos-release-12-12-1
+             placement='packages/mesquite',
+             when='+mesquite @12.18.1:12.18')
+    resource(name='mesquite',
+             git='https://github.com/trilinos/mesquite.git',
+             tag='develop',
+             placement='packages/mesquite',
+             when='+mesquite @master')
+
+    # ###################### Conflicts ##########################
+
+    # Epetra packages
+    with when('~epetra'):
+        conflicts('+amesos')
+        conflicts('+aztec')
+        conflicts('+epetraext')
+        conflicts('+ifpack')
+        conflicts('+isorropia')
+        conflicts('+ml', when='@13.2:')
+    with when('~epetraext'):
+        conflicts('+isorropia')
+        conflicts('+teko')
+        conflicts('+epetraextbtf')
+        conflicts('+epetraextexperimental')
+        conflicts('+epetraextgraphreorderings')
+
+    # Tpetra packages
+    with when('~kokkos'):
+        conflicts('+cuda')
+        conflicts('+tpetra')
+        conflicts('+intrepid2')
+        conflicts('+phalanx')
+    with when('~tpetra'):
+        conflicts('+amesos2')
+        conflicts('+dtk')
+        conflicts('+ifpack2')
+        conflicts('+muelu')
+        conflicts('+teko')
+        conflicts('+zoltan2')
+
+    with when('+teko'):
+        conflicts('~amesos')
+        conflicts('~anasazi')
+        conflicts('~aztec')
+        conflicts('~ifpack')
+        conflicts('~ml')
+        conflicts('~stratimikos')
+        conflicts('@:12 gotype=long')
+
+    # Known requirements from tribits dependencies
+    conflicts('+aztec', when='~fortran')
+    conflicts('+basker', when='~amesos2')
+    conflicts('+minitensor', when='~boost')
+    conflicts('+ifpack2', when='~belos')
+    conflicts('+intrepid', when='~sacado')
+    conflicts('+intrepid', when='~shards')
+    conflicts('+intrepid2', when='~shards')
+    conflicts('+isorropia', when='~zoltan')
+    conflicts('+phalanx', when='~sacado')
+    conflicts('+scorec', when='~mpi')
+    conflicts('+scorec', when='~shards')
+    conflicts('+scorec', when='~stk')
+    conflicts('+scorec', when='~zoltan')
+    conflicts('+tempus', when='~nox')
+    conflicts('+zoltan2', when='~zoltan')
+
+    # Only allow DTK with Trilinos 12.14, 12.18
+    conflicts('+dtk', when='~boost')
+    conflicts('+dtk', when='~intrepid2')
+    conflicts('+dtk', when='@:12.12,13:')
+
+    # Installed FindTrilinos are broken in SEACAS if Fortran is disabled
+    # see https://github.com/trilinos/Trilinos/issues/3346
+    conflicts('+exodus', when='@:13.0.1 ~fortran')
+    # Only allow Mesquite with Trilinos 12.12 and up, and master
+    conflicts('+mesquite', when='@:12.10,master')
+    # Strumpack is only available as of mid-2021
+    conflicts('+strumpack', when='@:13.0')
+    # Can only use one type of SuperLU
+    conflicts('+superlu-dist', when='+superlu')
+    # For Trilinos v11 we need to force SuperLUDist=OFF, since only the
+    # deprecated SuperLUDist v3.3 together with an Amesos patch is working.
+    conflicts('+superlu-dist', when='@11.4.1:11.14.3')
+    # see https://github.com/trilinos/Trilinos/issues/3566
+    conflicts('+superlu-dist', when='+float+amesos2+explicit_template_instantiation^superlu-dist@5.3.0:')
+    # Amesos, conflicting types of double and complex SLU_D
+    # see https://trilinos.org/pipermail/trilinos-users/2015-March/004731.html
+    # and https://trilinos.org/pipermail/trilinos-users/2015-March/004802.html
+    conflicts('+superlu-dist', when='+complex+amesos2')
+    # https://github.com/trilinos/Trilinos/issues/2994
+    conflicts(
+        '+shared', when='+stk platform=darwin',
+        msg='Cannot build Trilinos with STK as a shared library on Darwin.'
+    )
+    conflicts('+adios2', when='@:12.14.1')
+    conflicts('cxxstd=11', when='@master:')
+    conflicts('cxxstd=11', when='+wrapper ^cuda@6.5.14')
+    conflicts('cxxstd=14', when='+wrapper ^cuda@6.5.14:8.0.61')
+    conflicts('cxxstd=17', when='+wrapper ^cuda@6.5.14:10.2.89')
+
+    # Multi-value gotype only applies to trilinos through 12.14
+    conflicts('gotype=all', when='@12.15:')
+
+    # CUDA without wrapper requires clang
+    for _compiler in spack.compilers.supported_compilers():
+        if _compiler != 'clang':
+            conflicts('+cuda', when='~wrapper %' + _compiler,
+                      msg='trilinos~wrapper+cuda can only be built with the '
+                      'Clang compiler')
+    conflicts('+cuda_rdc', when='~cuda')
+    conflicts('+wrapper', when='~cuda')
+    conflicts('+wrapper', when='%clang')
+
+    # Old trilinos fails with new CUDA (see #27180)
+    conflicts('@:13.0.1 +cuda', when='^cuda@11:')
+
+    # stokhos fails on xl/xl_r
+    conflicts('+stokhos', when='%xl')
+    conflicts('+stokhos', when='%xl_r')
+
+    # Fortran mangling fails on Apple M1 (see spack/spack#25900)
+    conflicts('@:13.0.1 +fortran', when='target=m1')
+
+    # ###################### Dependencies ##########################
+
+    depends_on('adios2', when='+adios2')
+    depends_on('blas')
+    depends_on('boost', when='+boost')
+    depends_on('cgns', when='+exodus')
+    depends_on('hdf5+hl', when='+hdf5')
+    depends_on('hypre~internal-superlu~int64', when='+hypre')
+    depends_on('kokkos-nvcc-wrapper', when='+wrapper')
+    depends_on('lapack')
+    # depends_on('perl', type=('build',)) # TriBITS finds but doesn't use...
+    depends_on('libx11', when='+x11')
+    depends_on('matio', when='+exodus')
+    depends_on('metis', when='+zoltan')
+    depends_on('mpi', when='+mpi')
+    depends_on('netcdf-c', when="+exodus")
+    depends_on('parallel-netcdf', when='+exodus+mpi')
+    depends_on('parmetis', when='+mpi +zoltan')
+    depends_on('parmetis', when='+scorec')
+    depends_on('py-mpi4py', when='+mpi+python', type=('build', 'run'))
+    depends_on('py-numpy', when='+python', type=('build', 'run'))
+    depends_on('python', when='+python')
+    depends_on('python', when='@13.2: +ifpack +hypre', type='build')
+    depends_on('python', when='@13.2: +ifpack2 +hypre', type='build')
+    depends_on('scalapack', when='+mumps')
+    depends_on('scalapack', when='+strumpack+mpi')
+    depends_on('strumpack+shared', when='+strumpack')
+    depends_on('suite-sparse', when='+suite-sparse')
+    depends_on('superlu-dist', when='+superlu-dist')
+    depends_on('superlu@4.3 +pic', when='+superlu')
+    depends_on('swig', when='+python')
+    depends_on('zlib', when='+zoltan')
+
+    # Trilinos' Tribits config system is limited which makes it very tricky to
+    # link Amesos with static MUMPS, see
+    # https://trilinos.org/docs/dev/packages/amesos2/doc/html/classAmesos2_1_1MUMPS.html
+    # One could work it out by getting linking flags from mpif90 --showme:link
+    # (or alike) and adding results to -DTrilinos_EXTRA_LINK_FLAGS together
+    # with Blas and Lapack and ScaLAPACK and Blacs and -lgfortran and it may
+    # work at the end. But let's avoid all this by simply using shared libs
+    depends_on('mumps@5.0:+shared', when='+mumps')
+
+    for _flag in ('~mpi', '+mpi'):
+        depends_on('hdf5' + _flag, when='+hdf5' + _flag)
+        depends_on('mumps' + _flag, when='+mumps' + _flag)
+    for _flag in ('~openmp', '+openmp'):
+        depends_on('mumps' + _flag, when='+mumps' + _flag)
+
+    depends_on('hwloc', when='@13: +kokkos')
+    depends_on('hwloc+cuda', when='@13: +kokkos+cuda')
+    depends_on('hypre@develop', when='@master: +hypre')
+    depends_on('netcdf-c+mpi+parallel-netcdf', when="+exodus+mpi@12.12.1:")
+    depends_on('superlu-dist@4.4:5.3', when='@12.6.2:12.12.1+superlu-dist')
+    depends_on('superlu-dist@5.4:6.2.0', when='@12.12.2:13.0.0+superlu-dist')
+    depends_on('superlu-dist@6.3.0:', when='@13.0.1:99 +superlu-dist')
+    depends_on('superlu-dist@:4.3', when='@11.14.1:12.6.1+superlu-dist')
+    depends_on('superlu-dist@develop', when='@master: +superlu-dist')
+
+    # ###################### Patches ##########################
+
+    patch('umfpack_from_suitesparse.patch', when='@11.14.1:12.8.1')
+    for _compiler in ['xl', 'xl_r', 'clang']:
+        patch('xlf_seacas.patch', when='@12.10.1:12.12.1 %' + _compiler)
+        patch('xlf_tpetra.patch', when='@12.12.1 %' + _compiler)
+    patch('fix_clang_errors_12_18_1.patch', when='@12.18.1%clang')
+    patch('cray_secas_12_12_1.patch', when='@12.12.1%cce')
+    patch('cray_secas.patch', when='@12.14.1:%cce')
+
+    # workaround an NVCC bug with c++14 (https://github.com/trilinos/Trilinos/issues/6954)
+    # avoid calling deprecated functions with CUDA-11
+    patch('fix_cxx14_cuda11.patch', when='@13.0.0:13.0.1 cxxstd=14 ^cuda@11:')
+    # Allow building with +teko gotype=long
+    patch('https://github.com/trilinos/Trilinos/commit/b17f20a0b91e0b9fc5b1b0af3c8a34e2a4874f3f.patch',
+          sha256='dee6c55fe38eb7f6367e1896d6bc7483f6f9ab8fa252503050cc0c68c6340610',
+          when='@13.0.0:13.0.1 +teko gotype=long')
+
+    def flag_handler(self, name, flags):
+        is_cce = self.spec.satisfies('%cce')
+
+        if name == 'cxxflags':
+            spec = self.spec
+            if '+mumps' in spec:
+                # see https://github.com/trilinos/Trilinos/blob/master/packages/amesos/README-MUMPS
+                flags.append('-DMUMPS_5_0')
+            if '+stk platform=darwin' in spec:
+                flags.append('-DSTK_NO_BOOST_STACKTRACE')
+            if '+stk%intel' in spec:
+                # Workaround for Intel compiler segfaults with STK and IPO
+                flags.append('-no-ipo')
+            if '+wrapper' in spec:
+                flags.append('--expt-extended-lambda')
+        elif name == 'ldflags' and is_cce:
+            flags.append('-fuse-ld=gold')
+
+        if is_cce:
+            return (None, None, flags)
+        return (flags, None, None)
+
+    def url_for_version(self, version):
+        url = "https://github.com/trilinos/Trilinos/archive/trilinos-release-{0}.tar.gz"
+        return url.format(version.dashed)
+
+    def setup_dependent_run_environment(self, env, dependent_spec):
+        if '+cuda' in self.spec:
+            # currently Trilinos doesn't perform the memory fence so
+            # it relies on blocking CUDA kernel launch. This is needed
+            # in case the dependent app also run a CUDA backend via Trilinos
+            env.set('CUDA_LAUNCH_BLOCKING', '1')
+
+    def setup_dependent_package(self, module, dependent_spec):
+        if '+wrapper' in self.spec:
+            self.spec.kokkos_cxx = self.spec["kokkos-nvcc-wrapper"].kokkos_cxx
+        else:
+            self.spec.kokkos_cxx = spack_cxx
+
+    def setup_build_environment(self, env):
+        spec = self.spec
+        if '+cuda' in spec and '+wrapper' in spec:
+            if '+mpi' in spec:
+                env.set('OMPI_CXX', spec["kokkos-nvcc-wrapper"].kokkos_cxx)
+                env.set('MPICH_CXX', spec["kokkos-nvcc-wrapper"].kokkos_cxx)
+                env.set('MPICXX_CXX', spec["kokkos-nvcc-wrapper"].kokkos_cxx)
+            else:
+                env.set('CXX', spec["kokkos-nvcc-wrapper"].kokkos_cxx)
+
+    def cmake_args(self):
+        options = []
+
+        spec = self.spec
+        define = CMakePackage.define
+        define_from_variant = self.define_from_variant
+
+        def _make_definer(prefix):
+            def define_enable(suffix, value=None):
+                key = prefix + suffix
+                if value is None:
+                    # Default to lower-case spec
+                    value = suffix.lower()
+                elif isinstance(value, bool):
+                    # Explicit true/false
+                    return define(key, value)
+                return define_from_variant(key, value)
+            return define_enable
+
+        # Return "Trilinos_ENABLE_XXX" for spec "+xxx" or boolean value
+        define_trilinos_enable = _make_definer("Trilinos_ENABLE_")
+        # Same but for TPLs
+        define_tpl_enable = _make_definer("TPL_ENABLE_")
+
+        # #################### Base Settings #######################
+
+        options.extend([
+            define('Trilinos_VERBOSE_CONFIGURE', False),
+            define_from_variant('BUILD_SHARED_LIBS', 'shared'),
+            define_from_variant('CMAKE_CXX_STANDARD', 'cxxstd'),
+            define_trilinos_enable('ALL_OPTIONAL_PACKAGES', False),
+            define_trilinos_enable('ALL_PACKAGES', False),
+            define_trilinos_enable('CXX11', True),
+            define_trilinos_enable('DEBUG', 'debug'),
+            define_trilinos_enable('EXAMPLES', False),
+            define_trilinos_enable('SECONDARY_TESTED_CODE', True),
+            define_trilinos_enable('TESTS', False),
+            define_trilinos_enable('Fortran'),
+            define_trilinos_enable('OpenMP'),
+            define_trilinos_enable('EXPLICIT_INSTANTIATION',
+                                   'explicit_template_instantiation')
+        ])
+
+        # ################## Trilinos Packages #####################
+
+        options.extend([
+            define_trilinos_enable('Amesos'),
+            define_trilinos_enable('Amesos2'),
+            define_trilinos_enable('Anasazi'),
+            define_trilinos_enable('AztecOO', 'aztec'),
+            define_trilinos_enable('Belos'),
+            define_trilinos_enable('Epetra'),
+            define_trilinos_enable('EpetraExt'),
+            define_trilinos_enable('FEI', False),
+            define_trilinos_enable('Gtest'),
+            define_trilinos_enable('Ifpack'),
+            define_trilinos_enable('Ifpack2'),
+            define_trilinos_enable('Intrepid'),
+            define_trilinos_enable('Intrepid2'),
+            define_trilinos_enable('Isorropia'),
+            define_trilinos_enable('Kokkos'),
+            define_trilinos_enable('MiniTensor'),
+            define_trilinos_enable('Mesquite'),
+            define_trilinos_enable('ML'),
+            define_trilinos_enable('MueLu'),
+            define_trilinos_enable('NOX'),
+            define_trilinos_enable('Pamgen', False),
+            define_trilinos_enable('Panzer', False),
+            define_trilinos_enable('Pike', False),
+            define_trilinos_enable('Piro'),
+            define_trilinos_enable('Phalanx'),
+            define_trilinos_enable('PyTrilinos', 'python'),
+            define_trilinos_enable('ROL'),
+            define_trilinos_enable('Rythmos'),
+            define_trilinos_enable('Sacado'),
+            define_trilinos_enable('SCOREC'),
+            define_trilinos_enable('Shards'),
+            define_trilinos_enable('ShyLU'),
+            define_trilinos_enable('STK'),
+            define_trilinos_enable('Stokhos'),
+            define_trilinos_enable('Stratimikos'),
+            define_trilinos_enable('Teko'),
+            define_trilinos_enable('Tempus'),
+            define_trilinos_enable('Tpetra'),
+            define_trilinos_enable('TrilinosCouplings'),
+            define_trilinos_enable('Zoltan'),
+            define_trilinos_enable('Zoltan2'),
+            define_tpl_enable('Cholmod', False),
+            define_from_variant('EpetraExt_BUILD_BTF', 'epetraextbtf'),
+            define_from_variant('EpetraExt_BUILD_EXPERIMENTAL',
+                                'epetraextexperimental'),
+            define_from_variant('EpetraExt_BUILD_GRAPH_REORDERINGS',
+                                'epetraextgraphreorderings'),
+            define_from_variant('Amesos2_ENABLE_Basker', 'basker'),
+        ])
+
+        if '+dtk' in spec:
+            options.extend([
+                define('Trilinos_EXTRA_REPOSITORIES', 'DataTransferKit'),
+                define_trilinos_enable('DataTransferKit', True),
+            ])
+
+        if '+exodus' in spec:
+            options.extend([
+                define_trilinos_enable('SEACAS', True),
+                define_trilinos_enable('SEACASExodus', True),
+                define_trilinos_enable('SEACASIoss', True),
+                define_trilinos_enable('SEACASEpu', True),
+                define_trilinos_enable('SEACASExodiff', True),
+                define_trilinos_enable('SEACASNemspread', True),
+                define_trilinos_enable('SEACASNemslice', True),
+            ])
+        else:
+            options.extend([
+                define_trilinos_enable('SEACASExodus', False),
+                define_trilinos_enable('SEACASIoss', False),
+            ])
+
+        if '+chaco' in spec:
+            options.extend([
+                define_trilinos_enable('SEACAS', True),
+                define_trilinos_enable('SEACASChaco', True),
+            ])
+        else:
+            # don't disable SEACAS, could be needed elsewhere
+            options.extend([
+                define_trilinos_enable('SEACASChaco', False),
+                define_trilinos_enable('SEACASNemslice', False)
+            ])
+
+        if '+stratimikos' in spec:
+            # Explicitly enable Thyra (ThyraCore is required). If you don't do
+            # this, then you get "NOT setting ${pkg}_ENABLE_Thyra=ON since
+            # Thyra is NOT enabled at this point!" leading to eventual build
+            # errors if using MueLu because `Xpetra_ENABLE_Thyra` is set to
+            # off.
+            options.append(define_trilinos_enable('Thyra', True))
+
+            # Add thyra adapters based on package enables
+            options.extend(
+                define_trilinos_enable('Thyra' + pkg + 'Adapters', pkg.lower())
+                for pkg in ['Epetra', 'EpetraExt', 'Tpetra'])
+
+        # ######################### TPLs #############################
+
+        def define_tpl(trilinos_name, spack_name, have_dep):
+            options.append(define('TPL_ENABLE_' + trilinos_name, have_dep))
+            if not have_dep:
+                return
+            depspec = spec[spack_name]
+            libs = depspec.libs
+            try:
+                options.extend([
+                    define(trilinos_name + '_INCLUDE_DIRS',
+                           depspec.headers.directories),
+                ])
+            except NoHeadersError:
+                # Handle case were depspec does not have headers
+                pass
+
+            options.extend([
+                define(trilinos_name + '_ROOT', depspec.prefix),
+                define(trilinos_name + '_LIBRARY_NAMES', libs.names),
+                define(trilinos_name + '_LIBRARY_DIRS', libs.directories),
+            ])
+
+        # Enable these TPLs explicitly from variant options.
+        # Format is (TPL name, variant name, Spack spec name)
+        tpl_variant_map = [
+            ('ADIOS2', 'adios2', 'adios2'),
+            ('Boost', 'boost', 'boost'),
+            ('CUDA', 'cuda', 'cuda'),
+            ('HDF5', 'hdf5', 'hdf5'),
+            ('HYPRE', 'hypre', 'hypre'),
+            ('MUMPS', 'mumps', 'mumps'),
+            ('UMFPACK', 'suite-sparse', 'suite-sparse'),
+            ('SuperLU', 'superlu', 'superlu'),
+            ('SuperLUDist', 'superlu-dist', 'superlu-dist'),
+            ('X11', 'x11', 'libx11'),
+        ]
+        if spec.satisfies('@13.0.2:'):
+            tpl_variant_map.append(('STRUMPACK', 'strumpack', 'strumpack'))
+
+        for tpl_name, var_name, spec_name in tpl_variant_map:
+            define_tpl(tpl_name, spec_name, spec.variants[var_name].value)
+
+        # Enable these TPLs based on whether they're in our spec; prefer to
+        # require this way so that packages/features disable availability
+        tpl_dep_map = [
+            ('BLAS', 'blas'),
+            ('CGNS', 'cgns'),
+            ('LAPACK', 'lapack'),
+            ('Matio', 'matio'),
+            ('METIS', 'metis'),
+            ('Netcdf', 'netcdf-c'),
+            ('SCALAPACK', 'scalapack'),
+            ('Zlib', 'zlib'),
+        ]
+        if spec.satisfies('@12.12.1:'):
+            tpl_dep_map.append(('Pnetcdf', 'parallel-netcdf'))
+        if spec.satisfies('@13:'):
+            tpl_dep_map.append(('HWLOC', 'hwloc'))
+
+        for tpl_name, dep_name in tpl_dep_map:
+            define_tpl(tpl_name, dep_name, dep_name in spec)
+
+        # MPI settings
+        options.append(define_tpl_enable('MPI'))
+        if '+mpi' in spec:
+            # Force Trilinos to use the MPI wrappers instead of raw compilers
+            # to propagate library link flags for linkers that require fully
+            # resolved symbols in shared libs (such as macOS and some newer
+            # Ubuntu)
+            options.extend([
+                define('CMAKE_C_COMPILER', spec['mpi'].mpicc),
+                define('CMAKE_CXX_COMPILER', spec['mpi'].mpicxx),
+                define('CMAKE_Fortran_COMPILER', spec['mpi'].mpifc),
+                define('MPI_BASE_DIR', spec['mpi'].prefix),
+            ])
+
+        # ParMETIS dependencies have to be transitive explicitly
+        have_parmetis = 'parmetis' in spec
+        options.append(define_tpl_enable('ParMETIS', have_parmetis))
+        if have_parmetis:
+            options.extend([
+                define('ParMETIS_LIBRARY_DIRS', [
+                    spec['parmetis'].prefix.lib, spec['metis'].prefix.lib
+                ]),
+                define('ParMETIS_LIBRARY_NAMES', ['parmetis', 'metis']),
+                define('TPL_ParMETIS_INCLUDE_DIRS',
+                       spec['parmetis'].headers.directories +
+                       spec['metis'].headers.directories),
+            ])
+
+        if spec.satisfies('^superlu-dist@4.0:'):
+            options.extend([
+                define('HAVE_SUPERLUDIST_LUSTRUCTINIT_2ARG', True),
+            ])
+
+        if spec.satisfies('^parallel-netcdf'):
+            options.extend([
+                define('TPL_Netcdf_Enables_Netcdf4', True),
+                define('TPL_Netcdf_PARALLEL', True),
+                define('PNetCDF_ROOT', spec['parallel-netcdf'].prefix),
+            ])
+
+        # ################# Explicit template instantiation #################
+
+        complex_s = spec.variants['complex'].value
+        float_s = spec.variants['float'].value
+
+        options.extend([
+            define('Teuchos_ENABLE_COMPLEX', complex_s),
+            define('Teuchos_ENABLE_FLOAT', float_s),
+        ])
+
+        if '+tpetra +explicit_template_instantiation' in spec:
+            options.append(define_from_variant('Tpetra_INST_OPENMP', 'openmp'))
+            options.extend([
+                define('Tpetra_INST_DOUBLE', True),
+                define('Tpetra_INST_COMPLEX_DOUBLE', complex_s),
+                define('Tpetra_INST_COMPLEX_FLOAT', float_s and complex_s),
+                define('Tpetra_INST_FLOAT', float_s),
+                define('Tpetra_INST_SERIAL', True),
+            ])
+
+            gotype = spec.variants['gotype'].value
+            if gotype == 'all':
+                # default in older Trilinos versions to enable multiple GOs
+                options.extend([
+                    define('Tpetra_INST_INT_INT', True),
+                    define('Tpetra_INST_INT_LONG', True),
+                    define('Tpetra_INST_INT_LONG_LONG', True),
+                ])
+            else:
+                options.extend([
+                    define('Tpetra_INST_INT_INT', gotype == 'int'),
+                    define('Tpetra_INST_INT_LONG', gotype == 'long'),
+                    define('Tpetra_INST_INT_LONG_LONG', gotype == 'long_long'),
+                ])
+
+        # ################# Kokkos ######################
+
+        if '+kokkos' in spec:
+            arch = Kokkos.get_microarch(spec.target)
+            if arch:
+                options.append(define("Kokkos_ARCH_" + arch.upper(), True))
+
+            define_kok_enable = _make_definer("Kokkos_ENABLE_")
+            options.extend([
+                define_kok_enable('CUDA'),
+                define_kok_enable('OPENMP' if spec.version >= Version('13')
+                                  else 'OpenMP'),
+            ])
+            if '+cuda' in spec:
+                options.extend([
+                    define_kok_enable('CUDA_UVM', True),
+                    define_kok_enable('CUDA_LAMBDA', True),
+                    define_kok_enable('CUDA_RELOCATABLE_DEVICE_CODE', 'cuda_rdc')
+                ])
+                arch_map = Kokkos.spack_cuda_arch_map
+                options.extend(
+                    define("Kokkos_ARCH_" + arch_map[arch].upper(), True)
+                    for arch in spec.variants['cuda_arch'].value
+                )
+
+        # ################# System-specific ######################
+
+        # Fortran lib (assumes clang is built with gfortran!)
+        if ('+fortran' in spec
+                and spec.compiler.name in ['gcc', 'clang', 'apple-clang']):
+            fc = Executable(spec['mpi'].mpifc) if (
+                '+mpi' in spec) else Executable(spack_fc)
+            libgfortran = fc('--print-file-name',
+                             'libgfortran.' + dso_suffix,
+                             output=str).strip()
+            # if libgfortran is equal to "libgfortran.<dso_suffix>" then
+            # print-file-name failed, use static library instead
+            if libgfortran == 'libgfortran.' + dso_suffix:
+                libgfortran = fc('--print-file-name',
+                                 'libgfortran.a',
+                                 output=str).strip()
+            # -L<libdir> -lgfortran required for OSX
+            # https://github.com/spack/spack/pull/25823#issuecomment-917231118
+            options.append(
+                define('Trilinos_EXTRA_LINK_FLAGS',
+                       '-L%s/ -lgfortran' % os.path.dirname(libgfortran)))
+
+        if sys.platform == 'darwin' and macos_version() >= Version('10.12'):
+            # use @rpath on Sierra due to limit of dynamic loader
+            options.append(define('CMAKE_MACOSX_RPATH', True))
+        else:
+            options.append(define('CMAKE_INSTALL_NAME_DIR', self.prefix.lib))
+
+        return options
+
+    @run_after('install')
+    def filter_python(self):
+        # When trilinos is built with Python, libpytrilinos is included
+        # through cmake configure files. Namely, Trilinos_LIBRARIES in
+        # TrilinosConfig.cmake contains pytrilinos. This leads to a
+        # run-time error: Symbol not found: _PyBool_Type and prevents
+        # Trilinos to be used in any C++ code, which links executable
+        # against the libraries listed in Trilinos_LIBRARIES.  See
+        # https://github.com/trilinos/Trilinos/issues/569 and
+        # https://github.com/trilinos/Trilinos/issues/866
+        # A workaround is to remove PyTrilinos from the COMPONENTS_LIST
+        # and to remove -lpytrilonos from Makefile.export.Trilinos
+        if '+python' in self.spec:
+            filter_file(r'(SET\(COMPONENTS_LIST.*)(PyTrilinos;)(.*)',
+                        (r'\1\3'),
+                        '%s/cmake/Trilinos/TrilinosConfig.cmake' %
+                        self.prefix.lib)
+            filter_file(r'-lpytrilinos', '',
+                        '%s/Makefile.export.Trilinos' %
+                        self.prefix.include)
+
+    def setup_run_environment(self, env):
+        if '+exodus' in self.spec:
+            env.prepend_path('PYTHONPATH', self.prefix.lib)
+
+        if '+cuda' in self.spec:
+            # currently Trilinos doesn't perform the memory fence so
+            # it relies on blocking CUDA kernel launch.
+            env.set('CUDA_LAUNCH_BLOCKING', '1')
diff --git a/lib/spack/spack/test/package_sanity.py b/lib/spack/spack/test/package_sanity.py
index 0ae3313c8a..b57c853e0e 100644
--- a/lib/spack/spack/test/package_sanity.py
+++ b/lib/spack/spack/test/package_sanity.py
@@ -3,6 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 """This test does sanity checks on Spack's builtin package database."""
+import ast
 import os.path
 import pickle
 import re
@@ -58,18 +59,30 @@ def test_packages_are_pickleable():
 
 
 def test_packages_are_unparseable():
-    failed_to_unparse = list()
+    """Ensure that all packages can unparse and that unparsed code is valid Python."""
+    failed_to_unparse = []
+    failed_to_compile = []
+
     for name in spack.repo.all_package_names():
         try:
-            ph.canonical_source(name, filter_multimethods=False)
+            source = ph.canonical_source(name, filter_multimethods=False)
         except Exception:
             failed_to_unparse.append(name)
 
+        try:
+            compile(source, "internal", "exec", ast.PyCF_ONLY_AST)
+        except Exception:
+            failed_to_compile.append(name)
+
     if failed_to_unparse:
         tty.msg('The following packages failed to unparse: ' +
                 ', '.join(failed_to_unparse))
+        assert False
 
-    assert len(failed_to_unparse) == 0
+    if failed_to_compile:
+        tty.msg('The following unparsed packages failed to compile: ' +
+                ', '.join(failed_to_compile))
+        assert False
 
 
 def test_repo_getpkg_names_and_classes():
diff --git a/lib/spack/spack/test/util/package_hash.py b/lib/spack/spack/test/util/package_hash.py
index d410817303..db8ac1f5be 100644
--- a/lib/spack/spack/test/util/package_hash.py
+++ b/lib/spack/spack/test/util/package_hash.py
@@ -156,32 +156,28 @@ class ManyDocstrings:
         return "TWELVE"
 '''
 
+many_strings_no_docstrings = """\
+var = 'THREE'
 
-def test_remove_docstrings():
-    tree = ast.parse(many_strings)
-    tree = ph.RemoveDocstrings().visit(tree)
+class ManyDocstrings:
+    x = 'SEVEN'
 
-    unparsed = unparse(tree)
+    def method1():
+        print('NINE')
+        for i in range(10):
+            print(i)
 
-    # make sure the methods are preserved
-    assert "method1" in unparsed
-    assert "method2" in unparsed
+    def method2():
+        return 'TWELVE'
+"""
 
-    # all of these are unassigned and should be removed
-    assert "ONE" not in unparsed
-    assert "TWO" not in unparsed
-    assert "FOUR" not in unparsed
-    assert "FIVE" not in unparsed
-    assert "SIX" not in unparsed
-    assert "EIGHT" not in unparsed
-    assert "TEN" not in unparsed
-    assert "ELEVEN" not in unparsed
 
-    # these are used in legitimate expressions
-    assert "THREE" in unparsed
-    assert "SEVEN" in unparsed
-    assert "NINE" in unparsed
-    assert "TWELVE" in unparsed
+def test_remove_docstrings():
+    tree = ast.parse(many_strings)
+    tree = ph.RemoveDocstrings().visit(tree)
+
+    unparsed = unparse(tree, py_ver_consistent=True)
+    assert unparsed == many_strings_no_docstrings
 
 
 many_directives = """\
@@ -199,31 +195,137 @@ class HasManyDirectives:
 ))
 
 
-def test_remove_directives():
+def test_remove_all_directives():
     """Ensure all directives are removed from packages before hashing."""
+    for name in spack.directives.directive_names:
+        assert name in many_directives
+
     tree = ast.parse(many_directives)
     spec = Spec("has-many-directives")
     tree = ph.RemoveDirectives(spec).visit(tree)
-    unparsed = unparse(tree)
+    unparsed = unparse(tree, py_ver_consistent=True)
 
     for name in spack.directives.directive_names:
         assert name not in unparsed
 
 
+many_attributes = """\
+class HasManyMetadataAttributes:
+    homepage = "https://example.com"
+    url = "https://example.com/foo.tar.gz"
+    git = "https://example.com/foo/bar.git"
+
+    maintainers = ["alice", "bob"]
+    tags = ["foo", "bar", "baz"]
+
+    depends_on("foo")
+    conflicts("foo")
+"""
+
+
+many_attributes_canonical = """\
+class HasManyMetadataAttributes:
+    pass
+"""
+
+
+def test_remove_spack_attributes():
+    tree = ast.parse(many_attributes)
+    spec = Spec("has-many-metadata-attributes")
+    tree = ph.RemoveDirectives(spec).visit(tree)
+    unparsed = unparse(tree, py_ver_consistent=True)
+
+    assert unparsed == many_attributes_canonical
+
+
+complex_package_logic = """\
+class ComplexPackageLogic:
+    for variant in ["+foo", "+bar", "+baz"]:
+        conflicts("quux" + variant)
+
+    for variant in ["+foo", "+bar", "+baz"]:
+        # logic in the loop prevents our dumb analyzer from having it removed. This
+        # is uncommon so we don't (yet?) implement logic to detect that spec is unused.
+        print("oops can't remove this.")
+        conflicts("quux" + variant)
+
+        # Hard to make a while loop that makes sense, so ignore the infinite loop here.
+        # Likely nobody uses while instead of for, but we test it just in case.
+        while x <= 10:
+            depends_on("garply@%d.0" % x)
+
+    # all of these should go away, as they only contain directives
+    with when("@10.0"):
+        depends_on("foo")
+        with when("+bar"):
+            depends_on("bar")
+            with when("+baz"):
+                depends_on("baz")
+
+    # this whole statement should disappear
+    if sys.platform == "linux":
+        conflicts("baz@9.0")
+
+    # the else block here should disappear
+    if sys.platform == "linux":
+        print("foo")
+    else:
+        conflicts("foo@9.0")
+
+    # both blocks of this statement should disappear
+    if sys.platform == "darwin":
+        conflicts("baz@10.0")
+    else:
+        conflicts("bar@10.0")
+
+    # This one is complicated as the body goes away but the else block doesn't.
+    # Again, this could be optimized, but we're just testing removal logic here.
+    if sys.platform() == "darwin":
+        conflicts("baz@10.0")
+    else:
+        print("oops can't remove this.")
+        conflicts("bar@10.0")
+"""
+
+
+complex_package_logic_filtered = """\
+class ComplexPackageLogic:
+    for variant in ['+foo', '+bar', '+baz']:
+        print("oops can't remove this.")
+    if sys.platform == 'linux':
+        print('foo')
+    if sys.platform() == 'darwin':
+        pass
+    else:
+        print("oops can't remove this.")
+"""
+
+
+def test_remove_complex_package_logic_filtered():
+    tree = ast.parse(complex_package_logic)
+    spec = Spec("has-many-metadata-attributes")
+    tree = ph.RemoveDirectives(spec).visit(tree)
+    unparsed = unparse(tree, py_ver_consistent=True)
+
+    assert unparsed == complex_package_logic_filtered
+
+
 @pytest.mark.parametrize("package_spec,expected_hash", [
-    ("amdfftw",      "nfrk76xyu6wxs4xb4nyichm3om3kb7yp"),
+    ("amdfftw",      "tivb752zddjgvfkogfs7cnnvp5olj6co"),
     ("grads",        "rrlmwml3f2frdnqavmro3ias66h5b2ce"),
-    ("llvm",         "ngact4ds3xwgsbn5bruxpfs6f4u4juba"),
+    ("llvm",         "g3hoqf4rhprd3da7byp5nzco6tcwliiy"),
     # has @when("@4.1.0") and raw unicode literals
-    ("mfem",         "65xryd5zxarwzqlh2pojq7ykohpod4xz"),
-    ("mfem@4.0.0",   "65xryd5zxarwzqlh2pojq7ykohpod4xz"),
-    ("mfem@4.1.0",   "2j655nix3oe57iwvs2mlgx2mresk7czl"),
+    ("mfem",         "tiiv7uq7v2xtv24vdij5ptcv76dpazrw"),
+    ("mfem@4.0.0",   "tiiv7uq7v2xtv24vdij5ptcv76dpazrw"),
+    ("mfem@4.1.0",   "gxastq64to74qt4he4knpyjfdhh5auel"),
     # has @when("@1.5.0:")
-    ("py-torch",     "lnwmqk4wadtlsc2badrt7foid5tl5vaw"),
-    ("py-torch@1.0", "lnwmqk4wadtlsc2badrt7foid5tl5vaw"),
-    ("py-torch@1.6", "5nwndnknxdfs5or5nrl4pecvw46xc5i2"),
+    ("py-torch",     "qs7djgqn7dy7r3ps4g7hv2pjvjk4qkhd"),
+    ("py-torch@1.0", "qs7djgqn7dy7r3ps4g7hv2pjvjk4qkhd"),
+    ("py-torch@1.6", "p4ine4hc6f2ik2f2wyuwieslqbozll5w"),
     # has a print with multiple arguments
-    ("legion",       "ba4tleyb3g5mdhhsje6t6jyitqj3yfpz"),
+    ("legion",       "zdpawm4avw3fllxcutvmqb5c3bj5twqt"),
+    # has nested `with when()` blocks and loops
+    ("trilinos",     "vqrgscjrla4hi7bllink7v6v6dwxgc2p"),
 ])
 def test_package_hash_consistency(package_spec, expected_hash):
     """Ensure that that package hash is consistent python version to version.
diff --git a/lib/spack/spack/util/package_hash.py b/lib/spack/spack/util/package_hash.py
index d2b9055579..e6ae29b067 100644
--- a/lib/spack/spack/util/package_hash.py
+++ b/lib/spack/spack/util/package_hash.py
@@ -34,51 +34,106 @@ class RemoveDocstrings(ast.NodeTransformer):
         self.generic_visit(node)
         return node
 
-    def visit_FunctionDef(self, node):  # noqa
+    def visit_FunctionDef(self, node):
         return self.remove_docstring(node)
 
-    def visit_ClassDef(self, node):  # noqa
+    def visit_ClassDef(self, node):
         return self.remove_docstring(node)
 
-    def visit_Module(self, node):  # noqa
+    def visit_Module(self, node):
         return self.remove_docstring(node)
 
 
 class RemoveDirectives(ast.NodeTransformer):
-    """Remove Spack directives from a package AST."""
+    """Remove Spack directives from a package AST.
+
+    This removes Spack directives (e.g., ``depends_on``, ``conflicts``, etc.) and
+    metadata attributes (e.g., ``tags``, ``homepage``, ``url``) in a top-level class
+    definition within a ``package.py``, but it does not modify nested classes or
+    functions.
+
+    If removing directives causes a ``for``, ``with``, or ``while`` statement to have an
+    empty body, we remove the entire statement. Similarly, If removing directives causes
+    an ``if`` statement to have an empty body or ``else`` block, we'll remove the block
+    (or replace the body with ``pass`` if there is an ``else`` block but no body).
+
+    """
+
     def __init__(self, spec):
+        # list of URL attributes and metadata attributes
+        # these will be removed from packages.
+        self.metadata_attrs = [s.url_attr for s in spack.fetch_strategy.all_strategies]
+        self.metadata_attrs += spack.package.Package.metadata_attrs
+
         self.spec = spec
+        self.in_classdef = False  # used to avoid nested classdefs
+
+    def visit_Expr(self, node):
+        # Directives are represented in the AST as named function call expressions (as
+        # opposed to function calls through a variable callback). We remove them.
+        #
+        # Note that changes to directives (e.g., a preferred version change or a hash
+        # chnage on an archive) are already represented in the spec *outside* the
+        # package hash.
+        return None if (
+            node.value and isinstance(node.value, ast.Call) and
+            isinstance(node.value.func, ast.Name) and
+            node.value.func.id in spack.directives.directive_names
+        ) else node
+
+    def visit_Assign(self, node):
+        # Remove assignments to metadata attributes, b/c they don't affect the build.
+        return None if (
+            node.targets and isinstance(node.targets[0], ast.Name) and
+            node.targets[0].id in self.metadata_attrs
+        ) else node
+
+    def visit_With(self, node):
+        self.generic_visit(node)            # visit children
+        return node if node.body else None  # remove with statement if it has no body
+
+    def visit_For(self, node):
+        self.generic_visit(node)            # visit children
+        return node if node.body else None  # remove loop if it has no body
+
+    def visit_While(self, node):
+        self.generic_visit(node)            # visit children
+        return node if node.body else None  # remove loop if it has no body
+
+    def visit_If(self, node):
+        self.generic_visit(node)
+
+        # an empty orelse is ignored by unparsing, but an empty body with a full orelse
+        # ends up unparsing as a syntax error, so we replace the empty body into `pass`.
+        if not node.body:
+            if node.orelse:
+                node.body = [ast.Pass()]
+            else:
+                return None
+
+        # if the node has a body, it's valid python code with or without an orelse
+        return node
+
+    def visit_FunctionDef(self, node):
+        # do not descend into function definitions
+        return node
+
+    def visit_ClassDef(self, node):
+        # packages are always top-level, and we do not descend
+        # into nested class defs and their attributes
+        if self.in_classdef:
+            return node
+
+        # guard against recrusive class definitions
+        self.in_classdef = True
+        self.generic_visit(node)
+        self.in_classdef = False
+
+        # replace class definition with `pass` if it's empty (e.g., packages that only
+        # have directives b/c they subclass a build system class)
+        if not node.body:
+            node.body = [ast.Pass()]
 
-    def is_directive(self, node):
-        """Check to determine if the node is a valid directive
-
-        Directives are assumed to be represented in the AST as a named function
-        call expression.  This means that they will NOT be represented by a
-        named function call within a function call expression (e.g., as
-        callbacks are sometimes represented).
-
-        Args:
-            node (ast.AST): the AST node being checked
-
-        Returns:
-            bool: ``True`` if the node represents a known directive,
-                ``False`` otherwise
-        """
-        return (isinstance(node, ast.Expr) and
-                node.value and isinstance(node.value, ast.Call) and
-                isinstance(node.value.func, ast.Name) and
-                node.value.func.id in spack.directives.directive_names)
-
-    def is_spack_attr(self, node):
-        return (isinstance(node, ast.Assign) and
-                node.targets and isinstance(node.targets[0], ast.Name) and
-                node.targets[0].id in spack.package.Package.metadata_attrs)
-
-    def visit_ClassDef(self, node):  # noqa
-        if node.name == spack.util.naming.mod_to_class(self.spec.name):
-            node.body = [
-                c for c in node.body
-                if (not self.is_directive(c) and not self.is_spack_attr(c))]
         return node
 
 
@@ -89,7 +144,7 @@ class TagMultiMethods(ast.NodeVisitor):
         # map from function name to (implementation, condition_list) tuples
         self.methods = {}
 
-    def visit_FunctionDef(self, func):  # noqa
+    def visit_FunctionDef(self, func):
         conditions = []
         for dec in func.decorator_list:
             if isinstance(dec, ast.Call) and dec.func.id == 'when':
@@ -207,7 +262,7 @@ class ResolveMultiMethods(ast.NodeTransformer):
         # if nothing was picked, the last definition wins.
         return result
 
-    def visit_FunctionDef(self, func):  # noqa
+    def visit_FunctionDef(self, func):
         # if the function def wasn't visited on the first traversal there is a problem
         assert func.name in self.methods, "Inconsistent package traversal!"
 
@@ -277,7 +332,7 @@ def package_ast(spec, filter_multimethods=True, source=None):
     # create an AST
     root = ast.parse(source)
 
-    # remove docstrings and directives from the package AST
+    # remove docstrings, comments, and directives from the package AST
     root = RemoveDocstrings().visit(root)
     root = RemoveDirectives(spec).visit(root)