netcdf-cxx4 Package: updates to build on Mac OS (#29246)

Bug fixes for package netcdf-cxx4 so that it builds on macOS semi
case-sensitive filesystems; this includes additional changes to build
netcdf-cxx4 consistently with netcdf-fortran.

* netcdf-fortran: remove unused config_flags
* netcdf-fortran: avoid building without the optimization flags
* netcdf-cxx4: do not enforce autoreconf. This was a rudiment from the
  times when the package was fetched with git, which broke timestamp
  order of the automatically generated Autoconf files.
* netcdf-cxx4: inject PIC flags for C++ when '+pic'
* netcdf-cxx4: inject C/CXXFLAGS via the wrapper
* netcdf-cxx4: fix the underlinking problem for platforms other than darwin
  (add netcdf-c libs netcdf-cxx4 ldlibs flags)
* netcdf-cxx4: remove redundant extension of CPPFLAGS
* netcdf-cxx4: only need to use MPI compiler wrapper when building C
  (vs both C and C++)
* netcdf-cxx4: remove variant 'static'
  This makes it consistent with other packages from the NetCDF
  constellation: always build the static libraries and additionally
  build the shared ones when '+shared'.
* netcdf-cxx4: do not configure --with/--without-pic.
  This makes it consistent with other packages from the NetCDF
  constellation: build the shared libraries with the PIC flag and
  the static ones without it (the default for Autotools) when
  '~pic', and build the static libraries with PIC when '+pic' (to
  make them injectable into other shared libraries).
* netcdf-cxx4: run the tests serially
* netcdf-cxx4: build the plugins only when the tests are run

Co-authored-by: Sergey Kosukhin <sergey.kosukhin@mpimet.mpg.de>

2 files changed, 103 insertions, 46 deletions

diff --git a/var/spack/repos/builtin/packages/netcdf-cxx4/package.py b/var/spack/repos/builtin/packages/netcdf-cxx4/package.py
index fc475be5ca..012ab9ace2 100644
--- a/var/spack/repos/builtin/packages/netcdf-cxx4/package.py
+++ b/var/spack/repos/builtin/packages/netcdf-cxx4/package.py
@@ -3,6 +3,8 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import os
+
 from spack import *
 
 
@@ -19,61 +21,122 @@ class NetcdfCxx4(AutotoolsPackage):
     version('4.3.1', sha256='6a1189a181eed043b5859e15d5c080c30d0e107406fbb212c8fb9814e90f3445')
     version('4.3.0', sha256='e34fbc6aba243ec82c23e9ee99db2430555ada849c54c1f3ab081b0ddd0f5f30')
 
-    # Usually the configure automatically inserts the pic flags, but we can
-    # force its usage with this variant.
-    variant('static', default=True, description='Enable building static libraries')
     variant('shared', default=True, description='Enable shared library')
     variant('pic', default=True, description='Produce position-independent code (for shared libs)')
-    variant('doxygen', default=True, description='Enable doxygen docs')
+    variant('doc', default=False, description='Enable doxygen docs')
 
     depends_on('netcdf-c')
 
-    depends_on('automake', type='build')
-    depends_on('autoconf', type='build')
-    depends_on('libtool', type='build')
-    depends_on('m4', type='build')
-    depends_on('doxygen', when='+doxygen', type='build')
-
-    conflicts('~shared', when='~static')
-
-    force_autoreconf = True
+    depends_on('doxygen', when='+doc', type='build')
 
     def flag_handler(self, name, flags):
         if name == 'cflags' and '+pic' in self.spec:
             flags.append(self.compiler.cc_pic_flag)
-        elif name == 'cppflags':
-            flags.append('-I' + self.spec['netcdf-c'].prefix.include)
-
-        return (None, None, flags)
+        if name == 'cxxflags' and '+pic' in self.spec:
+            flags.append(self.compiler.cxx_pic_flag)
+        elif name == 'ldlibs':
+            # Address the underlinking problem reported in
+            # https://github.com/Unidata/netcdf-cxx4/issues/86, which also
+            # results into a linking error on macOS:
+            flags.append(self.spec['netcdf-c'].libs.link_flags)
+
+        # Note that cflags and cxxflags should be added by the compiler wrapper
+        # and not on the command line to avoid overriding the default
+        # compilation flags set by the configure script:
+        return flags, None, None
 
     @property
     def libs(self):
-        shared = True
-        return find_libraries(
-            'libnetcdf_c++4', root=self.prefix, shared=shared, recursive=True
-        )
+        libraries = ['libnetcdf_c++4']
 
-    def configure_args(self):
-        config_args = []
+        query_parameters = self.spec.last_query.extra_parameters
 
-        if '+static' in self.spec:
-            config_args.append('--enable-static')
+        if 'shared' in query_parameters:
+            shared = True
+        elif 'static' in query_parameters:
+            shared = False
         else:
-            config_args.append('--disable-static')
+            shared = '+shared' in self.spec
 
-        if '+shared' in self.spec:
-            config_args.append('--enable-shared')
-        else:
-            config_args.append('--disable-shared')
+        libs = find_libraries(
+            libraries, root=self.prefix, shared=shared, recursive=True
+        )
 
-        if '+pic' in self.spec:
-            config_args.append('--with-pic')
-        else:
-            config_args.append('--without-pic')
+        if libs:
+            return libs
+
+        msg = 'Unable to recursively locate {0} {1} libraries in {2}'
+        raise spack.error.NoLibrariesError(
+            msg.format('shared' if shared else 'static',
+                       self.spec.name,
+                       self.spec.prefix))
+
+    @when('@4.3.1:+shared')
+    @on_package_attributes(run_tests=True)
+    def patch(self):
+        # We enable the filter tests only when the tests are requested by the
+        # user. This, however, has a side effect: an extra file 'libh5bzip2.so'
+        # gets installed (note that the file has .so extension even on darwin).
+        # It's unclear whether that is intended but given the description of the
+        # configure option --disable-filter-testing (Do not run filter test and
+        # example; requires shared libraries and netCDF-4), we currently assume
+        # that the file is not really meant for the installation. To make all
+        # installations consistent and independent of whether the shared
+        # libraries or the tests are requested, we prevent installation of
+        # 'libh5bzip2.so':
+        filter_file(r'(^\s*)lib(_LTLIBRARIES\s*)(=\s*libh5bzip2\.la\s*$)',
+                    r'\1noinst\2+\3', join_path(self.stage.source_path,
+                                                'plugins', 'Makefile.in'))
+
+    def configure_args(self):
+        config_args = self.enable_or_disable('shared')
 
-        if '+doxygen' in self.spec:
+        if '+doc' in self.spec:
             config_args.append('--enable-doxygen')
         else:
             config_args.append('--disable-doxygen')
 
+        if self.spec.satisfies('@4.3.1:'):
+            if self.run_tests and '+shared' in self.spec:
+                config_args.append('--enable-filter-testing')
+                if self.spec.satisfies('^hdf5+mpi'):
+                    # The package itself does not need the MPI libraries but
+                    # includes <hdf5.h> in the filter test C code, which
+                    # requires <mpi.h> when HDF5 is built with the MPI support.
+                    # Using the MPI wrapper introduces overlinking to MPI
+                    # libraries and we would prefer not to use it but it is the
+                    # only reliable way to provide the compiler with the correct
+                    # path to <mpi.h>. For example, <mpi.h> of a MacPorts-built
+                    # MPICH might reside in /opt/local/include/mpich-gcc10,
+                    # which Spack does not know about and cannot inject with its
+                    # compiler wrapper.
+                    config_args.append('CC={0}'.format(self.spec['mpi'].mpicc))
+            else:
+                config_args.append('--disable-filter-testing')
+
         return config_args
+
+    @run_after('configure')
+    def rename_version(self):
+        # See https://github.com/Unidata/netcdf-cxx4/issues/109
+        # The issue is fixed upstream:
+        #   https://github.com/Unidata/netcdf-cxx4/commit/e7cc5bab02cf089dc79616456a0a951fee979fe9
+        # We do not apply the upstream patch because we want to avoid running
+        # autoreconf and introduce additional dependencies. We do not generate a
+        # patch for the configure script because the patched string contains the
+        # version and we would need a patch file for each supported version of
+        # the library. We do not implement the patching with filter_file in the
+        # patch method because writing a robust regexp seems to be more
+        # difficult that simply renaming the file if exists. It also looks like
+        # we can simply remove the file since it is not used anywhere.
+        if not self.spec.satisfies('@:4.3.1 platform=darwin'):
+            return
+
+        with working_dir(self.build_directory):
+            fname = 'VERSION'
+            if os.path.exists(fname):
+                os.rename(fname, '{0}.txt'.format(fname))
+
+    def check(self):
+        with working_dir(self.build_directory):
+            make('check', parallel=False)
diff --git a/var/spack/repos/builtin/packages/netcdf-fortran/package.py b/var/spack/repos/builtin/packages/netcdf-fortran/package.py
index d3843ccd0b..c79c8af46a 100644
--- a/var/spack/repos/builtin/packages/netcdf-fortran/package.py
+++ b/var/spack/repos/builtin/packages/netcdf-fortran/package.py
@@ -62,8 +62,6 @@ class NetcdfFortran(AutotoolsPackage):
     patch('no_parallel_build.patch', when='@4.5.2')
 
     def flag_handler(self, name, flags):
-        config_flags = None
-
         if name == 'cflags':
             if '+pic' in self.spec:
                 flags.append(self.compiler.cc_pic_flag)
@@ -81,15 +79,11 @@ class NetcdfFortran(AutotoolsPackage):
                 # The following flag forces the compiler to produce module
                 # files with lowercase names.
                 flags.append('-ef')
-        elif name == 'ldflags':
-            # We need to specify LDFLAGS to get correct dependency_libs
-            # in libnetcdff.la, so packages that use libtool for linking
-            # could correctly link to all the dependencies even when the
-            # building takes place outside of Spack environment, i.e.
-            # without Spack's compiler wrappers.
-            config_flags = [self.spec['netcdf-c'].libs.search_flags]
-
-        return flags, None, config_flags
+
+        # Note that cflags and fflags should be added by the compiler wrapper
+        # and not on the command line to avoid overriding the default
+        # compilation flags set by the configure script:
+        return flags, None, None
 
     @property
     def libs(self):