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

configury: look for PMI header in DIR provided by --with-pmi=DIR #4864

Merged
merged 1 commit into from
Feb 26, 2018

Conversation

ggouaillardet
Copy link
Contributor

Signed-off-by: Gilles Gouaillardet gilles@rist.or.jp

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>
Copy link
Contributor

@rhc54 rhc54 left a comment

Choose a reason for hiding this comment

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

I looked it over when I saw it come by earlier, and it all looks fine. However, one thing did occur to me. This fixes the -L/usr/lib problem for the user who specified --with-pmi, but it does nothing for anyone who specifies some other external package.

So my concern is: are we just kicking the problem down the road to the next .m4, and users are going to still get LDFLAGS thrown into the build?

@ggouaillardet
Copy link
Contributor Author

I think this is a PMI specific issue caused by the snippet below, and not a generic one.

AS_IF([test ! -z "$with_pmi" && test "$with_pmi" != "yes"],
                 [check_pmi_install_dir=$with_pmi],
                 [check_pmi_install_dir=/usr])

The same pattern might be present in other parts of the code, but that has not been reported (yet).
I'd rather fix this issue that was reported via the ML, complete the OPAL_CHECK_PACKAGE revamp, and then track this kind of issue (if any left)

@rhc54
Copy link
Contributor

rhc54 commented Feb 26, 2018

Agreed - I approved it because I wasn't suggesting we fix everything here. I only wanted to note that we probably do need to consider solving the entire issue, and shouldn't lose track of that problem

@rhc54 rhc54 merged commit 5c59876 into open-mpi:master Feb 26, 2018
@ggouaillardet
Copy link
Contributor Author

thanks, I updated #4860, #4861 and #4862

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants