Bug fixes for package netcdf-cxx4 on macOS, align netcdf-cxx4 with netcdf-fortran by climbfuji · Pull Request #29246 · spack/spack

climbfuji · 2022-02-28T15:21:49Z

This PR fixes a problem building netcdf-cxx4 (all the way up to the latest public release 4.3.1) on macOS with its semi case-sensitive file systems. An additional bug fix is required to get the correct library dependency on netcdf-c into the netcdf-cxx4 build on macOS, which uses libtool.

The PR also aligns the netcdf-cxx4 package config with the (better maintained) netcdf-fortran package config.

Fixes #29245
Fixes #26559

This PR was tested on macOS Monterey with [email protected] (and [email protected]), as well as on a Cray (NOAA-GFDL Gaea) with [email protected].

…e-sensitive filesystems; build package netcdf-cxx4 consistently with netcdf-fortran

skosukhin · 2022-02-28T16:40:22Z

var/spack/repos/builtin/packages/netcdf-cxx4/package.py

    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')


I support this renaming. Thank you!

var/spack/repos/builtin/packages/netcdf-cxx4/package.py

skosukhin · 2022-02-28T16:43:36Z

var/spack/repos/builtin/packages/netcdf-cxx4/package.py

+            # can 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]


The variable config_flags is set but seems not to be used anywhere.

This is copied from the netcdf-fortran package as well, but I missed returning the flags. Since you said it's not needed, I'll remove it here and in netcdf-fortran. Yes, I noticed that there are no .la files and I was wondering why netcdf-fortran had this code.

skosukhin · 2022-02-28T16:44:30Z

var/spack/repos/builtin/packages/netcdf-cxx4/package.py

+            # On macOS, we also need to add this to ldflags
+            if self.spec.satisfies("platform=darwin"):
+                flags = [self.spec['netcdf-c'].libs.search_flags, 
+                         self.spec['netcdf-c'].libs.link_flags] + flags


I am curious to learn why you think link_flags should be added. As far as I know, the configure script appends -lnetcdf automatically.

Definitely needed on macOS (in this or another way). I built netcdf-c with spack and netcdf-fortran as-is in the current spack repo, but netcdf-cxx4 did not have the -L.../netcdf-c/lib -lnetcdf in the linker line.

skosukhin · 2022-02-28T16:45:29Z

var/spack/repos/builtin/packages/netcdf-cxx4/package.py

+        if '+mpi' in netcdf_c_spec or '+parallel-netcdf' in netcdf_c_spec:
+            config_args.append('CC=%s' % self.spec['mpi'].mpicc)
+            config_args.append('FC=%s' % self.spec['mpi'].mpifc)
+            config_args.append('F77=%s' % self.spec['mpi'].mpif77)


I doubt that setting FC and F77 is needed for this package. I would add setting CXX and check whether CC is needed. In any case, thanks for trying to make this consistent.

Of course, good point. I'll remove Fortran and check if we still need to set CC.

skosukhin · 2022-02-28T16:56:38Z

var/spack/repos/builtin/packages/netcdf-cxx4/package.py

+            # Unidata has fixed this in their development track:
+            #   https://github.com/Unidata/netcdf-cxx4/commit/41c0233cb964a3ee1d4e5db5448cd28d617925fb
+            os.rename('VERSION', 'config.VERSION')
+        super(NetcdfCxx4, self).build(spec, prefix)


could you, please, check whether this can be replaced with (also for version 4.3.0):

patch('https://github.com/Unidata/netcdf-cxx4/commit/e7cc5bab02cf089dc79616456a0a951fee979fe9.patch', sha256='ba466b887c7ad2710e6d394046bc0742a2f4043d98bd21d5b08f33dc9d7f392d', when='@:4.3.1 platform=darwin')

I have used Unidata/netcdf-cxx4@e7cc5ba because that's the commit in the master upstream.

Ok, I got this - will update the sha hash in my next test:

==> Error: ChecksumError: sha256 checksum failed for /Users/heinzell/work/jedi-stack/spack-stack-new-joint/spack-stack-develop-20220215/cache/build_stage/spack-stage-u032oec1/e7cc5bab02cf089dc79616456a0a951fee979fe9.patch Expected ba466b887c7ad2710e6d394046bc0742a2f4043d98bd21d5b08f33dc9d7f392d but got 4ddf6db9dc0c5f754cb3d68b1dbef8c385cf499f6e5df8fbccae3749336ba84a

My bad, should be 4ddf6db9dc0c5f754cb3d68b1dbef8c385cf499f6e5df8fbccae3749336ba84a indeed.

You might need to run spack clean -m after the update and before the attempt to build.

Sure, I have learned (the hard way) that making spack forget isn't that easy ... The patch works just fine.

skosukhin · 2022-02-28T17:23:29Z

I have just realized that I contradict myself regarding the MPI wrappers. See #17051, which also explains why we need to add -lnetcdf (not only on MacOs).

Not that I really mind building with the MPI wrappers (btw, the comment in this PR refers to the Fortran-related documentation, is there one for C++?) but I normally try to avoid overlinking and would like to learn why exactly we should use the wrappers.

climbfuji · 2022-02-28T17:29:44Z

I have just realized that I contradict myself regarding the MPI wrappers. See #17051, which also explains why we need to add -lnetcdf (not only on MacOs).

Not that I really mind building with the MPI wrappers (btw, the comment in this PR refers to the Fortran-related documentation, is there one for C++?) but I normally try to avoid overlinking and would like to learn why exactly we should use the wrappers.

I can try building without the MPI wrappers. Maybe that problem is solved by linking against the netcdf-c library. I remember I got the missing MPI symbols first, so I put the MPI wrappers in place, before I ran into and fixed the netcdf-c linker problem.

skosukhin · 2022-02-28T17:34:37Z

@climbfuji thanks, please, keep in mind that the direct dependency on MPI might be unintended. For example, see #6632

climbfuji · 2022-02-28T18:00:38Z

@climbfuji thanks, please, keep in mind that the direct dependency on MPI might be unintended. For example, see #6632

Without any MPI wrapper, I get this:

     393    In file included from /Users/heinzell/work/jedi-stack/spack-stack-new-joint/spack-stack-develop-20220215/envs/jedi-fv3.netcdfcxxtest.macos/install/clang/13.0.0/hdf5-1.12.1-i23lyyj/include/hdf5.h:22:
  >> 394    /Users/heinzell/work/jedi-stack/spack-stack-new-joint/spack-stack-develop-20220215/envs/jedi-fv3.netcdfcxxtest.macos/install/clang/13.0.0/hdf5-1.12.1-i23lyyj/include/H5public.h:63:10: fatal error: 'mpi.h' file not found
     395    #include <mpi.h>
     396             ^~~~~~~

Will try with just the mpicxx wrapper next.

climbfuji · 2022-02-28T18:05:49Z

@climbfuji thanks, please, keep in mind that the direct dependency on MPI might be unintended. For example, see #6632

Without any MPI wrapper, I get this:

     393    In file included from /Users/heinzell/work/jedi-stack/spack-stack-new-joint/spack-stack-develop-20220215/envs/jedi-fv3.netcdfcxxtest.macos/install/clang/13.0.0/hdf5-1.12.1-i23lyyj/include/hdf5.h:22:
  >> 394    /Users/heinzell/work/jedi-stack/spack-stack-new-joint/spack-stack-develop-20220215/envs/jedi-fv3.netcdfcxxtest.macos/install/clang/13.0.0/hdf5-1.12.1-i23lyyj/include/H5public.h:63:10: fatal error: 'mpi.h' file not found
     395    #include <mpi.h>
     396             ^~~~~~~

Will try with just the mpicxx wrapper next.

Same problem with just the MPICXX wrapper, as expected.

…df-fortran/package.py

…ge.py

skosukhin · 2022-03-01T10:49:47Z

@climbfuji could you, please, tell me which MPI library you use? I find it strange that Spack compiler wrapper does not inject the required -I/path/to/mpi_h flag.

climbfuji · 2022-03-01T12:24:18Z

@climbfuji could you, please, tell me which MPI library you use? I find it strange that Spack compiler wrapper does not inject the required -I/path/to/mpi_h flag.

mpich-3.4.3 installed via homebrew (i.e. an external package for spack)

This was a rudiment from the times when the package was fetched with git, which broke timestamp order of the automatically generated Autoconf files.

…rwin

This makes it consistent with other packages from the NetCDF constellation: always build the static libraries and additionally build the shared ones when '+shared'.

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).

Bugfix/netcdf cxx4 macos

…netcdf_cxx4_macos

…ji/spack into bugfix/netcdf_cxx4_macos

climbfuji · 2022-03-08T18:55:24Z

@skosukhin @WardF I think this is ready to merge with your (Sergey's updates). Would you be able to review this? Thanks a lot in advance!

climbfuji · 2022-03-19T01:53:10Z

@skosukhin @WardF I have tested this to the bones on numerous systems (macOS, Linux, Cray) with several compilers (clang, gnu, intel) in the last ten days. Can we get this merged? Thanks a lot in in advance for your time.

climbfuji · 2022-03-25T21:40:22Z

@skosukhin @WardF I am sorry that I have to keep asking, but can we get this merged, please? It passed all the CI tests and works on all our platforms. I just updated the branch once again ... thanks very much!

skosukhin · 2022-03-28T13:02:33Z

@climbfuji none of us can actually do the merge. All I can do is write LGTM.
@haampie I've picked you almost randomly :) could you, please, merge this or poke someone else who can do it? Thanks!

climbfuji · 2022-03-28T13:59:37Z

@climbfuji none of us can actually do the merge. All I can do is write LGTM.
@haampie I've picked you almost randomly :) could you, please, merge this or poke someone else who can do it? Thanks!

Thanks, @skosukhin - appreciate your time.

climbfuji · 2022-04-12T01:09:20Z

Any update on this PR?

scheibelp

I had a couple questions

(I can merge this once those are resolved)

var/spack/repos/builtin/packages/netcdf-fortran/package.py

var/spack/repos/builtin/packages/netcdf-cxx4/package.py

scheibelp · 2022-04-14T17:10:53Z

Thanks!

climbfuji · 2022-04-14T17:18:14Z

Thanks indeed!

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 <[email protected]>

Bug fixes for package netcdf-cxx4 so that it builds on macOS semi cas…

c8e170b

…e-sensitive filesystems; build package netcdf-cxx4 consistently with netcdf-fortran

spackbot-app bot added dependencies new-variant update-package labels Feb 28, 2022

spackbot-app bot requested review from WardF and skosukhin February 28, 2022 15:21

skosukhin suggested changes Feb 28, 2022

View reviewed changes

Update netcdf-cxx4 and netcdf-fortran based on code review

6b1fbd1

spackbot-app bot added the patch label Feb 28, 2022

No need to import os anymore

e4ba713

climbfuji requested a review from skosukhin February 28, 2022 18:23

climbfuji added 2 commits February 28, 2022 14:35

Remove unused config_flags from var/spack/repos/builtin/packages/netc…

3613dc4

…df-fortran/package.py

Trim whitespace in var/spack/repos/builtin/packages/netcdf-cxx4/packa…

226fccc

…ge.py

climbfuji mentioned this pull request Feb 28, 2022

Update ESMF, netcdf-c, netcdf-fortran, netcdf-cxx4, libjpeg-turbo, py-numpy packages JCSDA/spack#16

Merged

skosukhin added 9 commits March 2, 2022 13:44

netcdf-fortran: avoid building without the optimization flags

19757f4

netcdf-cxx4: do not enforce autoreconf

67b7880

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'

080cb5c

netcdf-cxx4: inject C/CXXFLAGS via the wrapper

0ea6f13

netcdf-cxx4: fix the underlinking problem for platforms other than da…

f02227d

…rwin

netcdf-cxx4: remove redundant extention of CPPFLAGS

70c6ba4

netcdf-cxx4: fixup for the MPI-related problem

66d57c8

netcdf-cxx4: a bit more laconic syntax

535c412

netcdf-cxx4: add/update the comments

8ece49d

skosukhin added 5 commits March 2, 2022 16:47

netcdf-cxx4: remove variant 'static'

eb92355

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: run the tests serially

e9046e2

netcdf-cxx4: build the plugins only when the tests are run

b101675

netcdf-cxx4: update property 'libs'

bd2141c

skosukhin mentioned this pull request Mar 2, 2022

Bugfix/netcdf cxx4 macos climbfuji/spack#4

Merged

skosukhin and others added 7 commits March 4, 2022 16:35

eccodes: replace the patch with the post-configure method

6b5f475

eccodes: update comments

2831caf

Merge pull request #4 from skosukhin/bugfix/netcdf_cxx4_macos

1fec62a

Bugfix/netcdf cxx4 macos

Merge branch 'develop' into bugfix/netcdf_cxx4_macos

03d72fa

Merge branch 'develop' into bugfix/netcdf_cxx4_macos

ae781ba

Merge branch 'develop' of https://github.com/spack/spack into bugfix/…

4975248

…netcdf_cxx4_macos

Merge branch 'bugfix/netcdf_cxx4_macos' of https://github.com/climbfu…

0313202

…ji/spack into bugfix/netcdf_cxx4_macos

Merge branch 'develop' into bugfix/netcdf_cxx4_macos

9981003

scheibelp self-assigned this Apr 12, 2022

scheibelp reviewed Apr 12, 2022

View reviewed changes

var/spack/repos/builtin/packages/netcdf-fortran/package.py Show resolved Hide resolved

var/spack/repos/builtin/packages/netcdf-cxx4/package.py Show resolved Hide resolved

scheibelp approved these changes Apr 14, 2022

View reviewed changes

scheibelp merged commit bb70e6f into spack:develop Apr 14, 2022

climbfuji deleted the bugfix/netcdf_cxx4_macos branch April 14, 2022 17:18

Chrismarsh mentioned this pull request Feb 20, 2024

Migrate netcdf-cxx4 to use CMake and stop linking against system libnetcdf #42766

Merged

Conversation

climbfuji commented Feb 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skosukhin commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

climbfuji commented Feb 28, 2022

Uh oh!

skosukhin commented Feb 28, 2022

Uh oh!

climbfuji commented Feb 28, 2022

Uh oh!

climbfuji commented Feb 28, 2022

Uh oh!

skosukhin commented Mar 1, 2022

Uh oh!

climbfuji commented Mar 1, 2022

Uh oh!

climbfuji commented Mar 8, 2022

Uh oh!

climbfuji commented Mar 19, 2022

Uh oh!

climbfuji commented Mar 25, 2022

Uh oh!

skosukhin commented Mar 28, 2022

Uh oh!

climbfuji commented Mar 28, 2022

Uh oh!

climbfuji commented Apr 12, 2022

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

scheibelp commented Apr 14, 2022

Uh oh!

climbfuji commented Apr 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

skosukhin commented Feb 28, 2022 •

edited

Loading