Fix `--test` behavior for gromacs package. (#35674)

For `spack install --test=all gromacs`
* remove the `test` target from the `check()` call and just use
  the `check` target, in accordance with usual GROMACS test protocol
* build the test binaries explicitly during the build phase

Additional minor updates are necessary. This change
updates the package structure to the newer format with a
separate Builder class so we can override `check()`.
However, note that additional modernization should be
undertaken with care.

1 files changed, 47 insertions, 3 deletions

diff --git a/var/spack/repos/builtin/packages/gromacs/package.py b/var/spack/repos/builtin/packages/gromacs/package.py
index 8a28c76687..d6b04d15d4 100644
--- a/var/spack/repos/builtin/packages/gromacs/package.py
+++ b/var/spack/repos/builtin/packages/gromacs/package.py
@@ -5,6 +5,8 @@
 
 import os
 
+import llnl.util.filesystem as fs
+
 from spack.package import *
 
 
@@ -335,12 +337,54 @@ class Gromacs(CMakePackage):
                     r"-gencode;arch=compute_20,code=sm_21;?", "", "cmake/gmxManageNvccConfig.cmake"
                 )
 
+
+class CMakeBuilder(spack.build_systems.cmake.CMakeBuilder):
+    @run_after("build")
+    def build_test_binaries(self):
+        """Build the test binaries.
+
+        GROMACS usually excludes tests from the default build target, but building
+        the tests during spack's ``check`` phase takes a long time while producing
+        no visible output, even with ``--verbose``.
+
+        Here, we make sure the test binaries are built during the build phase
+        (as would normally be expected when configured with BUILD_TESTING)
+        when the ``--test`` flag is used.
+
+        Note: the GMX_DEVELOPER_BUILD option disables the EXCLUDE_FROM_ALL on the
+        test binaries, but the option incurs additional side effects that may
+        not be intended with ``--test``.
+        """
+        if self.pkg.run_tests:
+            with fs.working_dir(self.build_directory):
+                make("tests")
+
+    def check(self):
+        """Run the ``check`` target (skipping the ``test`` target).
+
+        Override the standard CMakeBuilder behavior. GROMACS has both `test`
+        and `check` targets, but we are only interested in the latter.
+        """
+        with fs.working_dir(self.build_directory):
+            if self.generator == "Unix Makefiles":
+                make("check")
+            elif self.generator == "Ninja":
+                ninja("check")
+
     def cmake_args(self):
         options = []
+        # Warning: Use `define_from_variant()` with caution.
+        # GROMACS may use unexpected conventions for CMake variable values.
+        # For example: variables that accept boolean values like "OFF"
+        # may actually be STRING type, and undefined variables may trigger
+        # different defaults for dependent options than explicitly defined variables.
+        # `-DGMX_VAR=OFF` may not have the same meaning as `-DGMX_VAR=`.
+        # In other words, the mapping between package variants and the
+        # GMX CMake variables is often non-trivial.
 
         if "+mpi" in self.spec:
             options.append("-DGMX_MPI:BOOL=ON")
-            if self.version < Version("2020"):
+            if self.pkg.version < Version("2020"):
                 # Ensures gmxapi builds properly
                 options.extend(
                     [
@@ -349,7 +393,7 @@ class Gromacs(CMakePackage):
                         "-DCMAKE_Fortran_COMPILER=%s" % self.spec["mpi"].mpifc,
                     ]
                 )
-            elif self.version == Version("2021"):
+            elif self.pkg.version == Version("2021"):
                 # Work around https://gitlab.com/gromacs/gromacs/-/issues/3896
                 # Ensures gmxapi builds properly
                 options.extend(
@@ -395,7 +439,7 @@ class Gromacs(CMakePackage):
         else:
             options.append("-DGMX_HWLOC:BOOL=OFF")
 
-        if self.version >= Version("2021"):
+        if self.pkg.version >= Version("2021"):
             if "+cuda" in self.spec:
                 options.append("-DGMX_GPU:STRING=CUDA")
             elif "+opencl" in self.spec: