-
Notifications
You must be signed in to change notification settings - Fork 932
configury: fix PMI detection #4860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
configury: fix PMI detection #4860
Conversation
and do not end up with -L/usr/lib[64] when PMI libraries are installed in the default location. Thanks Davide Vanzo for the report. Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp> (cherry picked from commit open-mpi/ompi@b86e0f0)
| [AC_MSG_RESULT([not found]) | ||
| AC_MSG_CHECKING([for $3.h in $1/include/slurm]) | ||
| AS_IF([test -f $1/include/slurm/$3.h], | ||
| AS_IF([test -n "$1"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urrr...what does test -n do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test -n string is equivalent to test ! -z string. In this case, $1 is empty when configure'd with --with-pmix or --with-pmix=yes, and not empty when configure'd with --with-pmix=DIR
config/opal_check_pmi.m4
Outdated
| AC_MSG_CHECKING([for $3.h in $1/include/slurm]) | ||
| AS_IF([test -f $1/include/slurm/$3.h], | ||
| AS_IF([test -n "$1"], | ||
| [AC_MSG_CHECKING([for $3.h in $1/include]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I note that we never just check to see if the header is present in where they told us to look - everything immediately puts an /include after it. I believe people have been complaining about that - shouldn't this check pattern start by checking where they told us it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, the goal of this PR is to stop ending up with -L/usr/lib[64] in the LDFLAGS, and I kept everything else unmodified.
That being said, this is a fair point and I will address this in a new commit and append it to this PR and its siblings for the other release branches.
Shall I stop searching in $1/include/slurm when $1 is not empty then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would continue that search as slurm does that strange thing, and users have come to expect it.
Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp> (cherry picked from commit open-mpi/ompi@83dd8cd)
and do not end up with -L/usr/lib[64] when PMI libraries
are installed in the default location.
Thanks Davide Vanzo for the report.
Signed-off-by: Gilles Gouaillardet gilles@rist.or.jp
(cherry picked from commit b86e0f0)