Skip to content
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

Topic/hcoll config #796

Merged
merged 4 commits into from
Aug 13, 2015
Merged

Conversation

ggouaillardet
Copy link
Contributor

No description provided.

@ggouaillardet
Copy link
Contributor Author

@miked-mellanox could you please review this ?
this was initially discussed on the ompi user mailing list at http://www.open-mpi.org/community/lists/users/2015/08/27418.php

@@ -25,20 +25,17 @@ AC_DEFUN([OMPI_CHECK_FCA],[
OPAL_CHECK_WITHDIR([fca], [$with_fca], [lib/libfca.so])

AS_IF([test "$with_fca" != "no"],
[AS_IF([test ! -z "$with_fca" && test "$with_fca" != "yes"],
[ompi_check_fca_libs=fca
AS_IF([test ! -z "$with_fca" && test "$with_fca" != "yes"],
[ompi_check_fca_dir=$with_fca
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$with_fca might equal no at this point (if the user specified --without-fca).

@ggouaillardet
Copy link
Contributor Author

@jsquyres got it, will do

@ggouaillardet ggouaillardet force-pushed the topic/hcoll_config branch 2 times, most recently from d976245 to 4fbf431 Compare August 12, 2015 05:04
@ggouaillardet
Copy link
Contributor Author

@jsquyres i updated this PR and made 3 for the ompi-release branches

@bureddy
Copy link
Member

bureddy commented Aug 12, 2015

👍

@@ -25,41 +25,37 @@ AC_DEFUN([OMPI_CHECK_FCA],[
OPAL_CHECK_WITHDIR([fca], [$with_fca], [lib/libfca.so])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help string is inaccurate -- it says it looks for libraries in DIR, but it really looks in DIR/lib (and headers in DIR/include). Can you fix that up as part of this PR, too? (sorry!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am doing that right now
i also fixed the same inaccurate string in hcoll, mxm (and knem too)

@jsquyres
Copy link
Member

I can't test this, but I think it looks ok...? (other than the inaccurate help message)

@bureddy did you test this patch?

@bureddy
Copy link
Member

bureddy commented Aug 12, 2015

@jsquyres , Yes, I tested v1.8 patch with options --with-hcoll, --with-hcoll=yes, --with-hcoll=no, --with-hcoll=< hcoll_install >

@jsquyres
Copy link
Member

Cool. Can you test v1.10 and v2.x, too? Feel free the thumbs-up each of the PRs after you test them. Thanks!

 * do not add -I/.../include/hcoll -I /.../include/hcoll/api to CPPFLAGS
 * allow configure --with-hcoll
 * search hcoll libs in both DIR/lib and DIR/lib64
 * fix the description of the --with-hcoll option
 * do not add -I/.../include/fca -I /.../include/fca_core to CPPFLAGS
 * allow configure --with-fca
 * search fca libs in both DIR/lib and DIR/lib64
 * fix the description of the --with-fca option
 * search mxm libs in both DIR/lib and DIR/lib64
 * fix the description of the --with-mxm option
@ggouaillardet ggouaillardet force-pushed the topic/hcoll_config branch 2 times, most recently from 7383298 to 288e13b Compare August 13, 2015 05:22
@ggouaillardet
Copy link
Contributor Author

i fixed the messages and did some cleanup,
it took more time than expected, but all should be good now

@jsquyres
Copy link
Member

One last comment: didn't a user bring this to our attention on the mailing list? If so, it would be good to cite that user's name in the commit message (as a way of saying "thank you!"). E.g., "Thanks to Joe Smith for bringing this issue to our attention."

ggouaillardet added a commit that referenced this pull request Aug 13, 2015
configury: fix hcoll, fca and mxm detection and revamp yalla Makefile.am
Thanks to David Shrader and Ake Sandgren for bringing this issue to our attention
@ggouaillardet ggouaillardet merged commit 6118236 into open-mpi:master Aug 13, 2015
jsquyres pushed a commit to jsquyres/ompi that referenced this pull request Aug 23, 2016
Silence libevent reentrant warning when using openib except for debug builds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants