Generalize netcdf build systems

[pwolfram]

This PR improves Makefiles and existing code for more robust compilation.

[pwolfram]

cc @mgduda, thanks for all the help last time. This PR is in response to challenges @josephzhang8 had getting these tools to compile.

A companion PR should provide a way to build netcdf in a consistent manner although https://github.com/pwolfram/homebrew-mpas and https://github.com/pwolfram/mpas-conda-recipes/tree/parallel_netcdf suggest some ways to do this as tested on edison (thanks @xylar and @mark-petersen for hints on how to do this). A complete netcdf / parallel-netcdf / pio implementation is still in the works because there are challenges with environment settings on edison that impede a clean build via either homebrew or conda.

[xylar]

This looks very helpful. I'd like to make sure I can build with it (on my laptop, mustang, wolf and edison) as well before it gets merged.

[xylar]

--flibs should be --libs here. I can't get this to build on Edison without this change because there is not a version of NetCDF there that includes both Fortran and C++ libraries.

[pwolfram]

Sounds good @xylar and thanks for catching this typo. By the way, if you build with either https://github.com/pwolfram/homebrew-mpas or https://github.com/pwolfram/mpas-conda-recipes/tree/parallel_netcdf that version should have both the C++ and Fortran bindings to netcdf. As far as I can tell only the netcdf library and its bindings are correctly installed.

[pwolfram]

FYI @xylar, this is fixed but github hasn't updated the comment for whatever reason...

[xylar]

@pwolfram, Thanks for the tips on building the libraries. I appreciate the work you've put into those and I'll give them a try at NERSC as soon as I have the time.

[xylar]

@pwolfram, I only tested the mesh_generation_tools, since these are what I use most frequently. I can build them with the new Makefile on my laptop (Ubuntu with g++) and with both g++ and icpc on wolf and mustang.

I had difficulty on Edison because the NetCDF libraries I usually use (cray-netcdf-hdf5parallel/4.4.0 and cray-parallel-netcdf/1.7.0) don't include the c++ libraries. Instead I had to first change --flibs to --libs in the Makefile (see comment above) and then:

module purge
module load PrgEnv-gnu # which complain about a lot of modules it can't find
module load netcdf/4.4.0
make

This produces the executables but may be impractical for use in MPAS runs if the NetCDF libraries are dynamically linked, given that these are different libraries from those that can be used to build/run the model. Obviously this concern is outside the scope of this PR, I just wanted to mention what I had had to do to test the PR.

[xylar]

@pwolfram, I realized that things are probably also not configured correctly on wolf or mustang. If I do:

$ which nc-config
/usr/bin/nc-config
$ nc-config --libs
-L/usr/projects/climate/SHARED_CLIMATE/anaconda_envs/default-2.7/lib -lnetcdf

This is definitely not the correct version of NetCDF to be using. I think the problem is that, when loading the following NetCDF modules on IC machines

module use /usr/projects/climate/SHARED_CLIMATE/modulefiles/all/
...
module load netcdf/4.4.0

we are not adding the NetCDF bin directory to the path.

This really needs to be fixed on IC before you merge this PR so we make sure no one's tools suddenly stop working.

[xylar]

@pwolfram, okay, I've made sure the bin directory gets added to the path when NetCDF libraries get loaded on IC machines. This takes care of the issue so as soon as you have fixed the --flibs-->--libs issue, I'm fine with this getting merged.

Whoever reviews this should go through all the tools and make sure 1) they build and 2) they actually work. That's more than I have time to take on at the moment but if you don't find anyone willing to review it, check back with me later.

[milenaveneziani]

@pwolfram: I can build and run the mask_creator tool, if that helps. I could do it on edison, wolf, and mustang (and maybe OLCF?).

[xylar]

@milenaveneziani, to clarify, you're offering to try that? Rather than that you've already done it?

[pwolfram]

Thanks for the comments @xylar, the --flibs-->--libs issue is fixed.

[akturner]

Quick question, do these changes prevent $NETCDF from being used as the netcdf library location? If so is there a way to override the new behaviour and still have $NETCDF the netcdf library location?

[milenaveneziani]

@xylar: sorry, after re-reading my comment I realize it's confusing.. Yes, I was offering to try that. I already ran the mask_creator on wolf, just by using the gnu libraries (not the setup of this PR).

[xylar]

@akturner and @pwolfram, maybe the best way to deal with this would be to first look for nc-config in $NETCDF/bin and then look in the user's path. I agree that this would be more intuitive and convenient in my opinion. I understand that there's not a good way to know how to configure the libraries without knowing where the correct nc-config lives, but there seems to be a tendency for the wrong nc-config to be in the path.

@pwolfram, how feasible would that be?

[pwolfram]

@akturner, thanks for catching this. After talking with you in person, we decided that we should check if $NETCDF exists, and if it does, we use the current makefile system. If it doesn't, we sort out dependencies via use of nc-config. Changes to rectify this will be forthcoming.

@xylar, the goal here is to not break the current system and I think this approach should work-- do you agree?

[xylar]

@pwolfram, I think you should still look for nc-config and revert to the current system only if none can be found by searching first in $NETCDF/bin and then in $PATH.

[pwolfram]

@xylar, your approach to ensure that the nc-config used is from $NETCDF seems better. @akturner, would you be ok with this change instead of the approach we talked about?

[xylar]

@pwolfram, let me amend the steps I would suggest:

• If $NETCDF is set and $NETCDF/bin/nc-config exists, use it
• If $NETCDF is set but $NETCDF/bin/nc-config does not exist, use existing flags (even if nc-config exists in the path somewhere)
• If $NETCDF is not set and nc-config exists in the path, use it
• If $NETCDF is not set and nc-config does not exists in the path, use existing flags

Would that work?

[pwolfram]

@xylar and @akturner, does commit b7facea above work for you? It has the logic we have all agreed on and if this is acceptable I can extend this approach to the rest of the MPAS-Tools Makefiles for this PR.

[xylar]

@pwolfram, the logic seems right. It should be tested, though, and I don't have time just now. I'll put it on my list.

[pwolfram]

Thanks @xylar. I tested this with my custom-built versions of netcdf with and without the $NETCDF specification approach. The last approach

If $NETCDF is not set and nc-config does not exists in the path, use existing flags

should fail I think because there is no information to build against netcdf. We may want to issue a warning for this-- would you agree?

[xylar]

@pwolfram, I agree that case should probably fail. It would presumably fail with the current Makefile, right?

[pwolfram]

@xylar, yes.

[mark-petersen]

@pwolfram Thank you for working on this. Compiling on many machines with various compilers and libraries is not fun, so we should all pitch it to ease the pain.

I did a quick test of grid_gen/mesh_conversion_tools on the following platforms:

LANL IC, wolf gnu

wf-fe1.lanl.gov> module purge
wf-fe1.lanl.gov> module use /usr/projects/climate/SHARED_CLIMATE/modulefiles/all/
wf-fe1.lanl.gov> module load gcc/4.8.2 openmpi/1.6.5 netcdf/4.4.0 parallel-netcdf/1.5.0 pio/1.7.2

wf-fe1.lanl.gov> make
... 
/usr/bin/ld: warning: libnetcdf.so.6, needed by /usr/lib/../lib64/libnetcdf_c++.so, may conflict with libnetcdf.so.7

I get that warning, but all compiles and runs OK. I tested here:

/lustre/scratch3/turquoise/mpeterse/runs/t36u/ocean/global_ocean/QU_240km/performance_test/init_step1

LANL IC, wolf intel
change Makefile from gnu to intel, then:

module purge
module use /usr/projects/climate/SHARED_CLIMATE/modulefiles/all/; 
module load intel/15.0.5 openmpi/1.6.5 netcdf/4.4.0 parallel-netcdf/1.5.0 pio/1.7.2
make

and tested as before. Everything worked fine.

[mark-petersen]

NERSC edison, cray compiler
I tried this on edison and was not able to get it to work. I tried the same netcdf libraries we use for MPAS, see:
https://gist.github.com/douglasjacobsen/ce893e2ccfb9be810fd4
Here are my notes for reference:

edison09> pwd
/global/u2/m/mpeterse/repos/MPAS-Tools/pr_generalize_makefile/grid_gen/mesh_conversion_tools
edison09> module purge
edison09> module load cray-netcdf/4.3.3.1 cray-parallel-netcdf/1.6.1
edison09> set NETCDF=${NETCDF_DIR}
edison09> set PNETCDF=${PARALLEL_NETCDF_DIR}

in Makefile, use cray compile command

 CC=cc
 CFLAGS= -O3 -std=c++0x -openmp -lstdc++
 DFLAGS= -g -std=c++0x -D_DEBUG -openmp -lstdc++

It does not find this netcdf header file:

edison09> make
cc mpas_mask_creator.cpp netcdf_utils.cpp jsoncpp.cpp -O3 -std=c++0x -openmp -lstdc++ -o MpasMaskCreator.x -I. -I/opt/cray/netcdf/4.3.3.1/GNU/5.1/include -I/opt/cray/hdf5/1.8.14/GNU/5.1/include -L/opt/cray/netcdf/4.3.3.1/GNU/5.1/lib -lnetcdf -lnetcdf_c++
In file included from mpas_mask_creator.cpp:19:
netcdf_utils.h:2:23: error: netcdfcpp.h: No such file or directory

I know I probably don't need PNETCDF and the compile flags might not be right for cray. Still, I spent 20 minutes on it and couldn't figure out how to compile, so this points out the difficulty of working on all these machines.

[mark-petersen]

Thanks, @xylar I see now. I wonder if we could just ask NERSC support to rebuild NETCDF with c++ support. It sounds like a reasonable request, and there is no time pressure.

[xylar]

@mark-petersen, that might be a really good idea. At the very least they could tell us what they would recommend. Maybe submit a ticket about it?

[mark-petersen]

Well, with a few emails to NERSC consultants, we now have a netcdf library with c++ support. I am able to build and run on edison as follows:

cd to
/global/u2/m/mpeterse/repos/MPAS-Tools/pr_generalize_makefile/grid_gen/mesh_conversion_tools
edison09> module load cray-netcdf
edison09> module load netcdf-cxx
edison09> module list
Currently Loaded Modulefiles:
  1) modules/3.2.10.4                       8) cray-libsci/16.07.1                   15) dvs/2.5_0.9.0-1.0502.1958.2.55.ari    22) cray-mpich/7.4.1
  2) nsg/1.2.0                              9) udreg/2.3.2-1.0502.9889.2.20.ari      16) alps/5.2.3-2.0502.9295.14.14.ari      23) slurm/edison
  3) eswrap/1.1.0-1.020200.1130.0          10) ugni/6.0-1.0502.10245.9.9.ari         17) rca/1.0.0-2.0502.57212.2.56.ari       24) darshan/2.3.1
  4) switch/1.0-1.0502.57058.1.58.ari      11) pmi/5.0.10-1.0000.11050.0.0.ari       18) atp/2.0.2                             25) cray-netcdf/4.4.0
  5) craype-network-aries                  12) dmapp/7.0.1-1.0502.10246.8.47.ari     19) PrgEnv-intel/5.2.56                   26) netcdf-cxx/4.2
  6) craype/2.5.5                          13) gni-headers/4.0-1.0502.10317.9.2.ari  20) craype-ivybridge
  7) intel/15.0.1.133                      14) xpmem/0.1-2.0502.57015.1.15.ari       21) cray-shmem/7.4.1

edison09> make
CC mpas_mask_creator.cpp netcdf_utils.cpp jsoncpp.cpp -O3 -std=c++0x -openmp -lstdc++ -o MpasMaskCreator.x -I. -I/usr/common/graphics/netcdf-cxx/4.2/include -L/usr/common/graphics/netcdf-cxx/4.2/lib -lnetcdf_c++ -lnetcdf -lstdc++ -lnetcdff
/opt/cray/hdf5/1.8.16/INTEL/15.0/lib/libhdf5.a(H5PL.o): In function `H5PL_load':
H5PL.c:(.text+0x5c2): warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
CC mpas_mesh_converter.cpp netcdf_utils.cpp -O3 -std=c++0x -openmp -lstdc++ -o MpasMeshConverter.x -I/usr/common/graphics/netcdf-cxx/4.2/include -L/usr/common/graphics/netcdf-cxx/4.2/lib -lnetcdf_c++ -lnetcdf -lstdc++ -lnetcdff
/opt/cray/hdf5/1.8.16/INTEL/15.0/lib/libhdf5.a(H5PL.o): In function `H5PL_load':
H5PL.c:(.text+0x5c2): warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
CC mpas_cell_culler.cpp netcdf_utils.cpp -O3 -std=c++0x -openmp -lstdc++ -o MpasCellCuller.x  -I/usr/common/graphics/netcdf-cxx/4.2/include -L/usr/common/graphics/netcdf-cxx/4.2/lib -lnetcdf_c++ -lnetcdf -lstdc++ -lnetcdff
/opt/cray/hdf5/1.8.16/INTEL/15.0/lib/libhdf5.a(H5PL.o): In function `H5PL_load':
H5PL.c:(.text+0x5c2): warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
edison09> ls -lh |grep Mpas
-rwxr-x--- 1 mpeterse mpeterse  18M Sep 22 06:17 MpasCellCuller.x*
-rwxr-x--- 1 mpeterse mpeterse  19M Sep 22 06:17 MpasMeshConverter.x*
-rwxr-x--- 1 mpeterse mpeterse  19M Sep 22 06:17 MpasMaskCreator.x*
edison09> git diff
edison09> git diff | cat
diff --git a/grid_gen/mesh_conversion_tools/Makefile b/grid_gen/mesh_conversion_tools/Makefile
index a134e8d..b48a0bf 100644
--- a/grid_gen/mesh_conversion_tools/Makefile
+++ b/grid_gen/mesh_conversion_tools/Makefile
@@ -1,14 +1,16 @@
 #Assumes ${NETCDF} is defined to the root of the netcdf library

 # gfortran
-CC=g++
-CFLAGS= -O3 -std=c++0x -fopenmp -lstdc++
-DFLAGS= -g -std=c++0x -D_DEBUG -fopenmp -lstdc++
+#CC=g++
+#CFLAGS= -O3 -std=c++0x -fopenmp -lstdc++
+#DFLAGS= -g -std=c++0x -D_DEBUG -fopenmp -lstdc++

 # intel
-# CC=icpc
-# CFLAGS= -O3 -std=c++0x -openmp -lstdc++
-# DFLAGS= -g -std=c++0x -D_DEBUG -openmp -lstdc++
+ CC=CC
+ CFLAGS= -O3 -std=c++0x -openmp -lstdc++
+ DFLAGS= -g -std=c++0x -D_DEBUG -openmp -lstdc++
+
+NETCDF=/usr/common/graphics/netcdf-cxx/4.2

A few notes:

1. compiler name is CC
2. I had to add NETCDF=/usr/common/graphics/netcdf-cxx/4.2 in the make file. For some reason it did not take it from the shell.
3. Note that a bunch of default modules are already loaded when you log in. Some of those are required.

This is a good test to generalize the build system, if we can simply type "module load XXX" and "make"

[xylar]

@pwolfram, it might be a good idea to try to assign someone to review this PR. Otherwise it's not likely to move forward. Not that I'm volunteering....

[milenaveneziani]

As an FYI: I tried this on Edison and was able to compile the mask-creator tools using the workflow (and notes) that @mark-petersen indicated above.

[pwolfram]

@xylar and @milenaveneziani, this has been a very controversial PR and to be honest, I'm not sure it is finished yet. Some good progress is made but there is likely more to do before it is ready to merge. The risk is introduction of bugs into the build systems if we move too quickly on this merge. As such, it is less clear if this will be a true added value merge if we prematurely merge. Could either of you take a look at this to see if it breaks anything for you? Also, it would be good if we can get @mgduda to test that this does not break his code for his applications. Once we know more fully that this doesn't cause problems I would be more comfortable about the merge. We don't want to break existing workflows. For example, I know the first cut of this broke @akturner's workflow, and we should have his sign-off before merging.

[milenaveneziani]

@pwolfram: I am really not sure what to test..
I know I used the branch on this PR plus Mark's suggestions on edison, and I could build the mesh-creator tool. After that I ran the ACME diagnostics and I didn't have a problem.
But I don't know if other things would break.
If you have specific suggestions of what to test let me know, although I wouldn't be able to do intensive testing at the moment.

[pwolfram]

Thanks @milenaveneziani, this is what makes this PR tricky. I'm guessing one way to approach this is to do the testing we can and fix things later if they get broken but I would prefer to use a better strategy if it exists. The minimal test is essentially that you can build without changing the makefile on every system you use for development.

[xylar]

@pwolfram, I think the problem is that these various tools get used with quite varying frequency, by different users, and at different stages in our workflow. It might be best to apply these changes to a single set of tools (e.g. the mask creator, cell culler and mesh converter) and use that as the test bed. Other tools could follow, one at a time, so it would be clearer to the users of each tool what to test and when. It's unrealistic to expect everyone involved in all these tools to test this particular branch, even though it probably made sense to implement all these similar changes in one branch.

[pwolfram]

@xylar, this is a great idea. If we need to split this out sooner than later @xylar and @milenaveneziani, which key parts need to follow this update for sub-PRs in the short term?

[xylar]

@pwolfram, in my opinion, the mesh conversion tools would be the first priority.

[pwolfram]

@xylar, @milenaveneziani, @mark-petersen and @akturner, I'm happy to return to this PR sometime soon. Do you all still agree that the best approach here is to split this into several smaller PRs, e.g., beginning with the mesh conversion tools?

[xylar]

@pwolfram, yes, I still think it needs to be done one tool at a time because it's next to impossible to find everyone involved with every tool and have them test them all at once. I agree that mesh conversion tools should be a priority.

[pwolfram]

#158 is the mesh conversion section of this PR.

[xylar]

@pwolfram, the next subset of this PR that you might want to peel off would potentially be periodic_quad and periodic_hex. Those are tools I've used for test cases.

[mgduda]

The changes to the global_scvt Makefile break the usability of this code for anyone who doesn't have gcc-4.8:

/tmp/MPAS-Tools/grid_gen/global_scvt>make
( cd src; make FC="gfortran" FFLAGS="-ffree-form -O3 -fopenmp -ffree-line-length-none -I/sysdisk1/duda/Thursday_test/local/include" F77FLAGS="-ffixed-form -O3 -fopenmp -fsecond-underscore" CPPFLAGS="-DRKIND=8 -D_RKIND=_8" PROMOTION="-fdefault-real-8" LDFLAGS="-O3 -fopenmp -L/sysdisk1/duda/Thursday_test/local/lib -lnetcdff -L/sysdisk1/duda/Thursday_test/local/lib -lnetcdf -lnetcdf -lhdf5_hl -lhdf5 -lz" )
make[1]: Entering directory `/dev/shm/MPAS-Tools/grid_gen/global_scvt/src'
gcc-4.8 -C -P -traditional-cpp -DRKIND=8 -D_RKIND=_8 STRIPACK.f > STRIPACK.for
/bin/sh: gcc-4.8: command not found

[mgduda]

The changes to grid_rotate also break builds when the netCDF library wasn't compiled with C++ support:

gfortran grid_rotate.f90 -o grid_rotate -m64 -O2 -ffree-line-length-none -ffree-form -Wall -I/sysdisk1/duda/Thursday_test/local/include -I/sysdisk1/duda/Thursday_test/local/include -L/sysdisk1/duda/Thursday_test/local/lib -lnetcdf -lnetcdf_c++ -lnetcdff
grid_rotate.f90:538:25:

          sphere_angle =  2.0 * asin(max(min(sin_angle,1.0_RKIND),-1.0_RKIND))
                         1
Warning: Possible change of value in conversion from REAL(8) to REAL(4) at (1) [-Wconversion]
grid_rotate.f90:540:24:

          sphere_angle = -2.0 * asin(max(min(sin_angle,1.0_RKIND),-1.0_RKIND))
                        1
Warning: Possible change of value in conversion from REAL(8) to REAL(4) at (1) [-Wconversion]
grid_rotate.f90:369:25:

       degreesToRadians = degAngle * 2 * pii / 360 
                         1
Warning: Possible change of value in conversion from REAL(8) to REAL(4) at (1) [-Wconversion]
/bin/ld: cannot find -lnetcdf_c++
collect2: error: ld returned 1 exit status
make: *** [grid_rotate] Error 1

[xylar]

@pwolfram, can we close the PR and (if you're so inclined) open PRs for individual tools one by one? I don't think this PR is going to happen as it is.

[mgduda]

I would agree that we should probably close this PR and create PRs to update individual tools. As I noted in earlier comments, the changes as proposed seem to break certain tools or generally, builds on certain platforms.

What we could do is to discuss a general strategy to be used for detecting the location of the netCDF library and the necessary include paths. We could then adopt this strategy on a per-tool basis.

@xylar and @pwolfram would there be any interest to discuss this via e-mail, a GitHub Issue, or on a future developers's telecon?

[xylar]

@mgduda, I don't really have time to add this to my plate. I'd be willing to review PRs related to specific tools if someone else has time to implement these changes. @pwolfram?

[pwolfram]

@mgduda, I'm not clear on whether there is enough broad-spectrum need / interest here. The risk of breaking someones' workflow appear to outweigh potential benefits, hence its "zombie" state... do you see a clear path toward integration / cleanup?

[mgduda]

I think if there were some agreement on the "best" way to detect and use netCDF libraries in a Makefile, we could adopt that method in individual tools. Likely, whoever was the largest user of a particular tool -- or whoever had available time -- could handle this on a per-tool basis.

However, I think we've all got quite a few items on our to-do lists, so I'm fine with simply closing this PR.

[xylar]

I'm going to go ahead and make the executive decision to close this PR.