easybuilders / easybuild-easyconfigs

A collection of easyconfig files that describe which software to build using which build options with EasyBuild.
https://easybuild.io
GNU General Public License v2.0
380 stars 703 forks source link

OpenFOAM-4.x-cleanup.patch doesn't set WM_CC (etc) #5183

Open JackPerdue opened 7 years ago

JackPerdue commented 7 years ago

Changes like this break OpenFOAM 4 for users since $CC isn't set by any compiler module:

e.g.

$ ml purge
$ ml OpenFOAM/4.1-intel-2017A-Python-2.7.12
$ source $FOAM_BASH
$ echo $WM_CC

nothing.... WM_CC isn't set because CC wasn't set when the user sourced $FOAM_BASH.

If of course works during building becase EB sets CC. But EB does not put CC in any compiler modules. This basically means users can't use wmake.

The proper solution (besides adding CC/CXX/FC to the compiler modules) it to replace

export WM_CC=$(CC)

with:

export WM_CC=gcc or export WM_CC=icc

Same for other compiler vars.

boegel commented 7 years ago

Hmm, this is supposed to be fixed with https://github.com/easybuilders/easybuild-easyblocks/pull/692... (see also https://github.com/easybuilders/easybuild-easyblocks/blob/master/easybuild/easyblocks/o/openfoam.py#L168)

Which EasyBuild version are you using here @JackPerdue? Are you using the stock OpenFOAM easyblock, or a customised one?

JackPerdue commented 7 years ago

Currently 3.4.0 stock block, though I see my last my last build of FOAM was on 3.3.0 in late June. I'll try again with 3.4.0.

boegel commented 7 years ago

I don't think that will make a difference, the PR that is supposed to fix this has been included ever since EasyBuild v2.4.0.

I just took a quick look, it seems like the compiler commands are still hardcoded:

$ diff -u OpenFOAM/4.1-intel-2017a/OpenFOAM-4.1/wmake/rules/linuxIcc/c++.orig.eb OpenFOAM/4.1-intel-2017a/OpenFOAM-4.1/wmake/rules/linuxIcc/c++
--- OpenFOAM/4.1-intel-2017a/OpenFOAM-4.1/wmake/rules/linuxIcc/c++.orig.eb      2016-10-16 16:11:45.000000000 +0200
+++ OpenFOAM/4.1-intel-2017a/OpenFOAM-4.1/wmake/rules/linuxIcc/c++      2017-09-26 02:02:43.826343000 +0200
@@ -6,7 +6,7 @@
 # Suppress some warnings for flex++ and CGAL
 c++LESSWARN = -diag-disable 1224,2026,2305

-CC          = icpc -std=c++0x -fp-trap=common -fp-model precise
+CC          = mpiicpc -cxx="icpc"

 include $(DEFAULT_RULES)/c++$(WM_COMPILE_OPTION)

Are you seeing something different in those wmake/rules files?

boegel commented 7 years ago

@JackPerdue ping?

JackPerdue commented 6 years ago

I'm still convinced it isn't correct..... I haven't rebuilt using 3.5 but the below is just wrong....

JackPerdue commented 6 years ago
$ for x in `grep ^OpenFOAM/ ./module.avail.ada | grep -v '/$'` ; do ml purge ; ml $x ; unset WM_CC ; source $EBROOTOPENFOAM/OpenFOAM-$EBVERSIONOPENFOAM/etc/bashrc ; echo "$x - WM_CC=$WM_CC" ; done
OpenFOAM/2.1.1-intel-2015B-CC_test - WM_CC=gcc
OpenFOAM/2.1.1-intel-2015B - WM_CC=gcc
OpenFOAM/2.2.2-intel-2016a - WM_CC=gcc
OpenFOAM/2.3.1-foss-2015a - WM_CC=gcc
OpenFOAM/2.3.1-intel-2015B-r2 - WM_CC=gcc
OpenFOAM/2.3.1-intel-2015B - WM_CC=gcc
OpenFOAM/2.3.1-intel-2017A - WM_CC=gcc
OpenFOAM/2.4.0-intel-2015B - WM_CC=gcc
OpenFOAM/2.4.0-intel-2017A-new - WM_CC=gcc
OpenFOAM/2.4.0-intel-2017A - WM_CC=gcc
OpenFOAM/3.0.0-intel-2015B-osX11-Python-2.7.10 - WM_CC=gcc
OpenFOAM/3.0.0-intel-2015B - WM_CC=gcc
OpenFOAM/3.0.1-intel-2016b - WM_CC=
OpenFOAM/4.0-intel-2016b - WM_CC=
-bash: [: too many arguments
OpenFOAM/4.0-intel-2017A-Python-2.7.12-no-ParaView - WM_CC=
OpenFOAM/4.0-intel-2017A-Python-2.7.12 - WM_CC=
OpenFOAM/4.1-intel-2017A-Python-2.7.12-test - WM_CC=
OpenFOAM/4.1-intel-2017A-Python-2.7.12 - WM_CC=
JackPerdue commented 6 years ago

OK... using EB 3.5.1 and the provided OF5 easyconfig (with minor changes):

[j-perdue@terra2 2017A-all]$ ml purge
[j-perdue@terra2 2017A-all]$ ml OpenFOAM/5.0-intel-2017A
[j-perdue@terra2 2017A-all]$ . $FOAM_BASH
[j-perdue@terra2 2017A-all]$ env | grep WM_C
WM_COMPILER_TYPE=system
WM_CXXFLAGS=
WM_CFLAGS=
WM_COMPILER_LIB_ARCH=64
WM_CXX=
WM_COMPILER=Icc
WM_CC=
WM_COMPILE_OPTION=Opt

That is, it is still broken and essentially means our FOAM users can't build additional OF tools since they usually require WM_CC and WM_CXX. Again, the problem is that the patch puts in WM_CC=$(CC) but there are no modules that set CC. Sure, you could probably build OF tools in EB since it sets CC but outside of EB CC is NOT set.

Personally, I've always thought you should set CC/CXX in the icc module but that has never been the case. So this is still broken and we are basically having to build OF by hand to placate our users.

JackPerdue commented 6 years ago

OpenFOAM-5.0-intel-2017A.eb.txt

JackPerdue commented 6 years ago

Note that I consider this much more of a blocker for 3.5.2 than any TensorFlow issues. It really should be fixed ASAP.

boegel commented 6 years ago

@JackPerdue Sorry for letting this drag on so long...

I just took another look at this, and I understand the problem now. We patch etc/config.sh/settings to make sure that OpenFOAM itself is compiled with the right compiler, rather than being hardcoded to gcc. We do this by picking up $CC, which is defined by EasyBuild in the build environment.

As you mention this is a problem when users build their own tools & solvers on top of OpenFOAM though, since $CC is typically not defined in their environment when they source $FOAM_BASH.

I'm not quite sure how to fix this though...

One obvious way would be to let the OpenFOAM modules define $CC, $CXX, $CFLAGS, $CXXFLAGS and $LDFLAGS, but that's also a bit problematic since those are not specific to OpenFOAM at all. So I wonder if there's a better solution for this.

Maybe @olesenm has a suggestion here?

We have quite a bit of OpenFOAM users ourselves, who also build their own tools, and we haven't had any complaints ever since we fixed our OpenFOAM installations with https://github.com/easybuilders/easybuild-easyblocks/pull/692.

Do you have any more details on how your users are running into this exactly @JackPerdue?

olesenm commented 6 years ago

Instead of patching settings.sh, it is also possible to drop everything special you need into an alternative compilation option and generate appropriate content for that. For example, with a standard WM_COMPILER=Icc setting, you could define WM_COMPILE_OPTION=Eb and a corresponding additional wmake rule.

wmake/rules/linux64Icc/c++Eb
# EasyBuild settings for Icc
CC = mpiicpc -cxx=icpc -std=c++11 ....
c++DBUG =
c++OPT  = -O3

Since the compile option is included later, you can effective overwrite CC, c++WARN, c++LESSWARN as well as defining c++DBUG, c++OPT.

This leaves the regular OpenFOAM files intact, but still gives room for your own EasyBuild definitions.

JackPerdue commented 6 years ago

OK.... for now I'm going to try this (untested but looks right):

prebuildopts = """
    sed -e "s/WM_CC=\$CC/WM_CC=$CC/" -i.eb.cc etc/config.sh/settings
    sed -e "s/WM_CXX=\$CXX/WM_CXX=$CXX/" -i.eb.cxx etc/config.sh/settings
    sed -e "s/WM_CFLAGS=\$CFLAGS/WM_CFLAGS=$CFLAGS/" -i.eb.cflags etc/config.sh/settings
    sed -e "s/WM_CXXFLAGS=\$CXXFLAGS/WM_CXXFlAGS=$CXXFLAGS/" -i.eb.cxxflags etc/config.sh/settings
    sed -e "s:WM_LDFLAGS=\$LDFLAGS:WM_LDFLAGS=$LDFLAGS:" -i.eb.ldflags etc/config.sh/settings
""" 

which gives me this diff for intel/2017b (after the patch currently included):

$ diff settings.orig settings
64,68c64,68
<             export WM_CC='gcc'
<             export WM_CXX='g++'
<             export WM_CFLAGS='-m64 -fPIC'
<             export WM_CXXFLAGS='-m64 -fPIC -std=c++0x'
<             export WM_LDFLAGS='-m64'
---
>             export WM_CC=icc
>             export WM_CXX=icpc
>             export WM_CFLAGS=-O2 -xHost -ftz -fp-speculation=safe -fp-model source -std=c++11 -no-prec-div
>             export WM_CXXFlAGS=-O2 -xHost -ftz -fp-speculation=safe -fp-model source -std=c++11 -no-prec-div
>             export WM_LDFLAGS=-L/sw/eb/sw/icc/2017.4.196-GCC-6.4.0-2.28/lib/intel64 -L/sw/eb/sw/imkl/2017.3.196-iimpi-2017b/lib -L/sw/eb/sw/imkl/2017.3.196-iimpi-2017b/mkl/lib/intel64 -L/sw/eb/sw/imkl/2017.3.196-iimpi-2017b/lib -L/sw/eb/sw/libreadline/7.0-GCCcore-6.4.0/lib -L/sw/eb/sw/ncurses/6.1-GCCcore-6.4.0/lib -L/sw/eb/sw/METIS/5.1.0-GCCcore-6.4.0/lib -L/sw/eb/sw/Bison/3.0.4-GCCcore-6.4.0/lib -L/sw/eb/sw/flex/2.6.4-GCCcore-6.4.0/lib

@boegel and like-minded individuals might want to put that in the easyblock maybe.

In either case, I think this addresses what I (my users) are trying to fix.

JackPerdue commented 6 years ago

(backslashes missing in the sed commands)....

sed -e "s/WM_CXXFLAGS=\$CXXFLAGS/WM_CXXFlAGS=$CXXFLAGS/" -i.eb.cxxflags etc/config.sh/settings

JackPerdue commented 6 years ago

still missing... anyway... the first $ for CXXFLAGS should be backslashed

JackPerdue commented 6 years ago

This seems to work...

OpenFOAM-5.0-intel-2017b.eb.txt