ompi_wrapper_script: fix $extra_ldflags #2250

ggouaillardet · 2016-10-19T01:17:23Z

use @OMPI_PKG_CONFIG_LDFLAGS@ instead of @OMPI_WRAPPER_EXTRA_LDFLAGS@
so @{libdir} is substitued with ${libdir}

Thanks Manesh Nanavalla for the report

(back-ported from commit cb76d93)

use @OMPI_PKG_CONFIG_LDFLAGS@ instead of @OMPI_WRAPPER_EXTRA_LDFLAGS@ so @{libdir} is substitued with ${libdir} Thanks Manesh Nanavalla for the report (back-ported from commit open-mpi/ompi@cb76d93)

ggouaillardet · 2016-10-19T05:27:50Z

:bot:mellanox:retest

jsquyres

While this may be correct, it feels misleading.

I.e., when future Jeff/future Gilles looks at this code, we'll wonder "Hey, why is $extra_ldflags set to the PKG version of LDFLAGS while all other variables are set with the EXTRA_WRAPPER version? Even more curious, the wrapper-data files all use EXTRA_WRAPPER... is this use of the PKG version a bug?"

Meaning: I see why you did what you did. And it fixes the problem. But it feels like a band-aid that will be confusing in the future, rather than making a slightly bigger but more comprehensive / easier-to-read / easier-to-maintain solution. Given that the wrapper compilers are already pretty complicated, I think it would be a good idea to have a more expressive solution to this issue.

ggouaillardet · 2016-10-20T04:23:44Z

Are you suggesting the explanation from the commit message should be added as a comment in the code ?

jsquyres · 2016-10-24T20:58:30Z

@ggouaillardet I think that's a minimum. But -- without thinking this through to be specific -- it feels like there should be a better solution that decouples the "extra" vs. "pkg" flags so that we don't have two similar-but-slightly-different sets of flags floating around...? (I would be wrong, though)

jsquyres · 2016-10-25T18:10:38Z

Please add a Signed-off-by line to this PR's commit.

hppritcha · 2016-12-07T23:35:35Z

moving to 2.0.3

jsquyres · 2017-01-24T19:58:22Z

@ggouaillardet Any progress?

jsquyres · 2017-02-20T18:08:34Z

@ggouaillardet Ping

ggouaillardet · 2017-02-21T06:26:00Z

@jsquyres i do not feel very inspired on how to move forward with this.

what about the patch below ?
if it works for you, i will commit it into master and append it to this PR

diff --git a/config/opal_setup_wrappers.m4 b/config/opal_setup_wrappers.m4
index 6c33008..4f1da0b 100644
--- a/config/opal_setup_wrappers.m4
+++ b/config/opal_setup_wrappers.m4
@@ -12,7 +12,7 @@ dnl Copyright (c) 2004-2005 The Regents of the University of California.
 dnl                         All rights reserved.
 dnl Copyright (c) 2006-2010 Oracle and/or its affiliates.  All rights reserved.
 dnl Copyright (c) 2009-2016 Cisco Systems, Inc.  All rights reserved.
-dnl Copyright (c) 2015-2016 Research Organization for Information Science
+dnl Copyright (c) 2015-2017 Research Organization for Information Science
 dnl                         and Technology (RIST). All rights reserved.
 dnl Copyright (c) 2016      IBM Corporation.  All rights reserved.
 dnl $COPYRIGHT$
@@ -310,9 +310,9 @@ dnl Avoid some repetitive code below
 dnl
 AC_DEFUN([_OPAL_SETUP_WRAPPER_FINAL_PKGCONFIG],[
     AC_MSG_CHECKING([for $1 pkg-config LDFLAGS])
-    $1_PKG_CONFIG_LDFLAGS=`echo "$$1_WRAPPER_EXTRA_LDFLAGS" | sed -e 's/@{libdir}/\${libdir}/g'`
-    AC_SUBST([$1_PKG_CONFIG_LDFLAGS])
-    AC_MSG_RESULT([$$1_PKG_CONFIG_LDFLAGS])
+    $1_WRAPPER_EXTRA_LDFLAGS_NO_AT=`echo "$$1_WRAPPER_EXTRA_LDFLAGS" | sed -e 's/@{libdir}/\${libdir}/g'`
+    AC_SUBST([$1_WRAPPER_EXTRA_LDFLAGS_NO_AT])
+    AC_MSG_RESULT([$$1_WRAPPER_EXTRA_LDFLAGS_NO_AT])
 ])
 
 
@@ -328,9 +328,9 @@ AC_DEFUN([OPAL_SETUP_WRAPPER_FINAL],[
           [AC_MSG_WARN([RPATH support requested but not available])
            AC_MSG_ERROR([Cannot continue])])
 
-    # Note that we have to setup <package>_PKG_CONFIG_LDFLAGS for the
+    # Note that we have to setup <package>_WRAPPER_EXTRA_LDFLAGS_NO_AT for the
     # pkg-config files to parallel the
-    # <package>_WRAPPER_EXTRA_LDFLAGS.  This is because pkg-config
+    # <package>_WRAPPER_EXTRA_LDFLAGS.  This is because pkg-config nor the OMPI wrapper script
     # will not understand the @{libdir} notation in
     # *_WRAPPER_EXTRA_LDFLAGS; we have to translate it to ${libdir}.
 
diff --git a/ompi/tools/wrappers/ompi-c.pc.in b/ompi/tools/wrappers/ompi-c.pc.in
index be29ad7..c4397b1 100644
--- a/ompi/tools/wrappers/ompi-c.pc.in
+++ b/ompi/tools/wrappers/ompi-c.pc.in
@@ -16,7 +16,7 @@ pkgincludedir=@opalincludedir@
 # static linking (they're pulled in by libopen-rte.so's implicit
 # dependencies), so only list these in Libs.private.
 #
-Libs: -L${libdir} @OMPI_PKG_CONFIG_LDFLAGS@ -l@OMPI_LIBMPI_NAME@
+Libs: -L${libdir} @OMPI_WRAPPER_EXTRA_LDFLAGS_NO_AT@ -l@OMPI_LIBMPI_NAME@
 Libs.private: -lopen-rte -lopen-pal @OMPI_WRAPPER_EXTRA_LIBS@
 #
 Cflags: -I${includedir} @OMPI_WRAPPER_EXTRA_CPPFLAGS@ @OMPI_WRAPPER_EXTRA_CFLAGS@
diff --git a/ompi/tools/wrappers/ompi-cxx.pc.in b/ompi/tools/wrappers/ompi-cxx.pc.in
index 4b19366..1698bc0 100644
--- a/ompi/tools/wrappers/ompi-cxx.pc.in
+++ b/ompi/tools/wrappers/ompi-cxx.pc.in
@@ -16,7 +16,7 @@ pkgincludedir=@opalincludedir@
 # static linking (they're pulled in by libopen-rte.so's implicit
 # dependencies), so only list these in Libs.private.
 #
-Libs: -L${libdir} @OMPI_PKG_CONFIG_LDFLAGS@ @OMPI_WRAPPER_CXX_LIB@ -l@OMPI_LIBMPI_NAME@
+Libs: -L${libdir} @OMPI_WRAPPER_EXTRA_LDFLAGS_NO_AT@ @OMPI_WRAPPER_CXX_LIB@ -l@OMPI_LIBMPI_NAME@
 Libs.private: -lopen-rte -lopen-pal @OMPI_WRAPPER_EXTRA_LIBS@
 #
 Cflags: -I${includedir} @OMPI_WRAPPER_EXTRA_CPPFLAGS@ @OMPI_WRAPPER_EXTRA_CXXFLAGS@
diff --git a/ompi/tools/wrappers/ompi-fort.pc.in b/ompi/tools/wrappers/ompi-fort.pc.in
index 5635870..d75b283 100644
--- a/ompi/tools/wrappers/ompi-fort.pc.in
+++ b/ompi/tools/wrappers/ompi-fort.pc.in
@@ -16,6 +16,6 @@ pkgincludedir=@opalincludedir@
 # static linking (they're pulled in by libopen-rte.so's implicit
 # dependencies), so only list these in Libs.private.
 #
-Libs: -L${libdir} @OMPI_PKG_CONFIG_LDFLAGS@ @OMPI_FORTRAN_USEMPIF08_LIB@ @OMPI_FORTRAN_USEMPI_LIB@ -l@OMPI_LIBMPI_NAME@_mpifh -l@OMPI_LIBMPI_NAME@
+Libs: -L${libdir} @OMPI_WRAPPER_EXTRA_LDFLAGS_NO_AT@ @OMPI_FORTRAN_USEMPIF08_LIB@ @OMPI_FORTRAN_USEMPI_LIB@ -l@OMPI_LIBMPI_NAME@_mpifh -l@OMPI_LIBMPI_NAME@
 Libs.private: -lopen-rte -lopen-pal @OMPI_WRAPPER_EXTRA_LIBS@
 Cflags: -I${includedir} @OMPI_WRAPPER_EXTRA_CPPFLAGS@ @OMPI_WRAPPER_EXTRA_FCFLAGS@
diff --git a/ompi/tools/wrappers/ompi.pc.in b/ompi/tools/wrappers/ompi.pc.in
index c696186..47387eb 100644
--- a/ompi/tools/wrappers/ompi.pc.in
+++ b/ompi/tools/wrappers/ompi.pc.in
@@ -16,7 +16,7 @@ pkgincludedir=@opalincludedir@
 # static linking (they're pulled in by libopen-rte.so's implicit
 # dependencies), so only list these in Libs.private.
 #
-Libs: -L${libdir} @OMPI_PKG_CONFIG_LDFLAGS@ -l@OMPI_LIBMPI_NAME@
+Libs: -L${libdir} @OMPI_WRAPPER_EXTRA_LDFLAGS_NO_AT@ -l@OMPI_LIBMPI_NAME@
 Libs.private: @OMPI_WRAPPER_EXTRA_LIBS@
 #
 Cflags: -I${includedir} @OMPI_WRAPPER_EXTRA_CPPFLAGS@ @OMPI_WRAPPER_EXTRA_CFLAGS@
diff --git a/ompi/tools/wrappers/ompi_wrapper_script.in b/ompi/tools/wrappers/ompi_wrapper_script.in
index 0822a58..2a4155a 100644
--- a/ompi/tools/wrappers/ompi_wrapper_script.in
+++ b/ompi/tools/wrappers/ompi_wrapper_script.in
@@ -8,7 +8,7 @@
 # Copyright (c) 2010      Oracle and/or its affiliates.  All rights reserved.
 # Copyright (c) 2013      Sandia National Laboratories.  All rights reserved.
 # Copyright (c) 2016      IBM Corporation.  All rights reserved.
-# Copyright (c) 2016      Research Organization for Information Science
+# Copyright (c) 2016-2017 Research Organization for Information Science
 #                         and Technology (RIST). All rights reserved.
 # $COPYRIGHT$
 #
@@ -43,7 +43,7 @@ my $extra_cxxflags = "@OMPI_WRAPPER_EXTRA_CXXFLAGS@";
 my $extra_cxxflags_prefix = "@OMPI_WRAPPER_EXTRA_CXXFLAGS_PREFIX@";
 my $extra_fcflags = "@OMPI_WRAPPER_EXTRA_FCFLAGS@";
 my $extra_fcflags_prefix = "@OMPI_WRAPPER_EXTRA_FCFLAGS_PREFIX@";
-my $extra_ldflags = "@OMPI_PKG_CONFIG_LDFLAGS@";
+my $extra_ldflags = "@OMPI_WRAPPER_EXTRA_LDFLAGS_NO_AT@";
 my $extra_libs = "@OMPI_WRAPPER_EXTRA_LIBS@";
 my $cxx_lib = "@OMPI_WRAPPER_CXX_LIB@";
 my $fc_module_flag = "@OMPI_FC_MODULE_FLAG@";
diff --git a/opal/tools/wrappers/opal.pc.in b/opal/tools/wrappers/opal.pc.in
index ed1b3ad..fcdd191 100644
--- a/opal/tools/wrappers/opal.pc.in
+++ b/opal/tools/wrappers/opal.pc.in
@@ -15,7 +15,7 @@ pkgincludedir=@opalincludedir@
 # (they're pulled in via libopen-pal.so's implicit dependencies), so
 # list them in Libs.private.
 #
-Libs: -L${libdir} @OPAL_PKG_CONFIG_LDFLAGS@ -lopen-pal
+Libs: -L${libdir} @OPAL_WRAPPER_EXTRA_LDFLAGS_NO_AT@ -lopen-pal
 Libs.private: @OPAL_WRAPPER_EXTRA_LIBS@
 #
 # It is safe to hard-wire the -I before the EXTRA_INCLUDES because we
diff --git a/orte/tools/wrappers/orte.pc.in b/orte/tools/wrappers/orte.pc.in
index 2bbd31b..76042e8 100644
--- a/orte/tools/wrappers/orte.pc.in
+++ b/orte/tools/wrappers/orte.pc.in
@@ -15,7 +15,7 @@ pkgincludedir=@opalincludedir@
 # static linking (they're pulled in by libopen-rte.so's implicit
 # dependencies), so only list these in Libs.private.
 #
-Libs: -L${libdir} @ORTE_PKG_CONFIG_LDFLAGS@ -l@ORTE_LIB_PREFIX@open-rte
+Libs: -L${libdir} @ORTE_WRAPPER_EXTRA_LDFLAGS_NO_AT@ -l@ORTE_LIB_PREFIX@open-rte
 Libs.private: -l@OPAL_LIB_PREFIX@open-pal @ORTE_WRAPPER_EXTRA_LIBS@
 #
 # It is safe to hard-wire the -I before the EXTRA_INCLUDES because we

hppritcha · 2017-10-24T20:06:54Z

@jsquyres should we close this?

ompi_wrapper_script: fix $extra_ldflags

ce7640a

use @OMPI_PKG_CONFIG_LDFLAGS@ instead of @OMPI_WRAPPER_EXTRA_LDFLAGS@ so @{libdir} is substitued with ${libdir} Thanks Manesh Nanavalla for the report (back-ported from commit open-mpi/ompi@cb76d93)

ggouaillardet added the bug label Oct 19, 2016

ggouaillardet added this to the v2.0.2 milestone Oct 19, 2016

ggouaillardet assigned jsquyres Oct 19, 2016

jsquyres requested changes Oct 19, 2016

View reviewed changes

jsquyres added the pushed-back label Oct 19, 2016

hppritcha modified the milestones: v2.0.3, v2.0.2 Dec 7, 2016

hppritcha modified the milestones: v2.0.3, v2.0.4 Jun 1, 2017

hppritcha modified the milestones: v2.0.4, v2.0.5 Nov 9, 2017

hppritcha closed this Dec 7, 2017

jsquyres mentioned this pull request Jan 9, 2018

ompi_wrapper_script: fix $extra_ldflags #2249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ompi_wrapper_script: fix $extra_ldflags #2250

ompi_wrapper_script: fix $extra_ldflags #2250

ggouaillardet commented Oct 19, 2016

ggouaillardet commented Oct 19, 2016

jsquyres left a comment

ggouaillardet commented Oct 20, 2016

jsquyres commented Oct 24, 2016

jsquyres commented Oct 25, 2016

hppritcha commented Dec 7, 2016

jsquyres commented Jan 24, 2017

jsquyres commented Feb 20, 2017

ggouaillardet commented Feb 21, 2017

hppritcha commented Oct 24, 2017

ompi_wrapper_script: fix $extra_ldflags #2250

ompi_wrapper_script: fix $extra_ldflags #2250

Conversation

ggouaillardet commented Oct 19, 2016

ggouaillardet commented Oct 19, 2016

jsquyres left a comment

Choose a reason for hiding this comment

ggouaillardet commented Oct 20, 2016

jsquyres commented Oct 24, 2016

jsquyres commented Oct 25, 2016

hppritcha commented Dec 7, 2016

jsquyres commented Jan 24, 2017

jsquyres commented Feb 20, 2017

ggouaillardet commented Feb 21, 2017

hppritcha commented Oct 24, 2017