Bug 2437

Summary: openmpi invalid suffix on literal
Product: ns-3 Reporter: Tom Henderson <tomh>
Component: mpiAssignee: George Riley <riley>
Status: REOPENED ---    
Severity: normal CC: joshpelkey, ns-bugs, pdbarnes
Priority: P5    
Version: unspecified   
Hardware: All   
OS: All   

Description Tom Henderson 2016-06-11 17:21:53 EDT
An upstream problem with OpenMPI (https://www.open-mpi.org/community/lists/users/2013/11/22912.php) leads to compilation error when MPI is enabled for ns-3, due to recent cutover to use -std=c++11:

C++11 requires a space between literal and string macro [-Werror=literal-suffix]

This can be worked around by disabling the warning when MPI is enabled (with no effect in the typical case that MPI is disabled):

+                    conf.env.append_value('CXXFLAGS', '-Wno-literal-suffix')
Comment 1 Tom Henderson 2016-06-11 17:26:00 EDT
pushed workaround in changeset 12157:ffd079a4b2e4
Comment 2 Peter Barnes 2016-07-13 21:11:12 EDT
For the record, this appears to have been fixed in upstream OpenMPI-1.7 (feature), released Apr 2, 2013, and OpenMPI-1.8 (stable) Mar 31, 2014.

https://github.com/open-mpi/ompi-release/blob/v1.7/opal/include/opal/opal_portable_platform.h

OpenMPI-1.10 is current, released Aug 2015.

Instead of the patch pushed in r12157:ffd079a4b2e4 which disables a warning for all versions, should we instead require OpenMPI >= 1.7?

From the shell: 

$ ompi_info | grep "Open MPI:" | cut -d ':' -f 2 | cut -d '.' -f 1-2
 1.10

gets me the major.minor version number.  I need help Pythonizing this into src/mpi/wscript. The conditionals around detection of OpenMPI then MPICH will have to be refactored a bit too.
Comment 3 Tom Henderson 2016-07-14 09:58:21 EDT
(In reply to Peter Barnes from comment #2)
> For the record, this appears to have been fixed in upstream OpenMPI-1.7
> (feature), released Apr 2, 2013, and OpenMPI-1.8 (stable) Mar 31, 2014.
> 
> https://github.com/open-mpi/ompi-release/blob/v1.7/opal/include/opal/
> opal_portable_platform.h
> 
> OpenMPI-1.10 is current, released Aug 2015.
> 
> Instead of the patch pushed in r12157:ffd079a4b2e4 which disables a warning
> for all versions, should we instead require OpenMPI >= 1.7?
> 
> From the shell: 
> 
> $ ompi_info | grep "Open MPI:" | cut -d ':' -f 2 | cut -d '.' -f 1-2
>  1.10
> 
> gets me the major.minor version number.  I need help Pythonizing this into
> src/mpi/wscript. The conditionals around detection of OpenMPI then MPICH
> will have to be refactored a bit too.

I believe that r12157 is not sufficient anyway since a user reported to me that clang does not support -Wno-literal-suffix.  

My only reservation for the version detection such as above is whether enough systems have OpenMPI >= 1.7 (for instance, Ubuntu 14.04 is still on 1.6), and whether ompi_info is typically installed (on Fedora, it is not provided by openmpi or openmpi-devel; I didn't even find it in openmpi-debuginfo)
Comment 4 Peter Barnes 2016-08-05 19:17:24 EDT
The first OpenMPI version which lists ompi_info in the docs is v1.3.4
https://www.open-mpi.org/doc/v1.3/

We're not above telling folks to update truly obsolete packages :)
https://groups.google.com/d/msg/ns-3-users/4ZFbFhDP-wM/E1VCE2GLCwAJ

This may qualify.  If you're sophisticated enough to use MPI, you ought to be sophisticated enough to use your package manager.

In any case, I can confirm that the patch doesn't work with 
Apple LLVM version 7.0.2 (clang-700.1.81)
Comment 5 Tom Henderson 2016-08-05 19:33:06 EDT
(In reply to Peter Barnes from comment #4)
> The first OpenMPI version which lists ompi_info in the docs is v1.3.4
> https://www.open-mpi.org/doc/v1.3/
> 
> We're not above telling folks to update truly obsolete packages :)
> https://groups.google.com/d/msg/ns-3-users/4ZFbFhDP-wM/E1VCE2GLCwAJ

I agree and I don't have a problem to tell people to upgrade MPI.  I have no problem reverting my workaround patch.  The question I have is how to gracefully handle the configure version checking when ompi_info is not installed.  I couldn't find a package in Fedora providing it.

If we find ompi_info is provided by the packages, we can list this as a prerequisite.  Or, is there another way to version check?
Comment 6 Peter Barnes 2016-08-05 20:14:13 EDT
It looks like this stanza in mpi.h was introduced in released version 1.1

/* Major, minor, and release version of Open MPI */
#define OMPI_MAJOR_VERSION 1
#define OMPI_MINOR_VERSION 10
#define OMPI_RELEASE_VERSION 3

A little conf.check_nonfatal(fragment=... and were there.
Comment 7 Tom Henderson 2016-08-28 11:35:57 EDT
I reverted the original patch and agree that we should pursue a conf.check-based solution.