Skip to content
This repository was archived by the owner on Sep 30, 2022. It is now read-only.

Conversation

artpol84
Copy link
Contributor

ompi commit: open-mpi/ompi@3ff16e7
ompi pull request: open-mpi/ompi#240

@jsquyres
Copy link
Member

Artem --

I'm struggling to remember why the initial -I. was there. In my comment on your master PR (open-mpi/ompi#240 (comment)), I was only referring to your new second -I, not the first/original -I..

Can you check to see exactly which gcc -E ... command is emitted, and see if changing -I. to -I$srcdir is necessary?

FWIW: the libltdl.h file should be in the source tree (vs. the build tree), so -I$srcdir might actually be correct. But I'm not remembering exactly what that -I. was for -- i.e., was it actually for libltdl.h, or was it for something else? And if it was actually for libltdl.h, is it just plain wrong, and your now -I$srcdir/... is the correct one? I.e., should the original -I. (now -I$srcdir) just be deleted?

@artpol84
Copy link
Contributor Author

That is a good point, Jeff.

Looking closely to the configuration system I can see that we deal with following conftest.c template:

CPPFLAGS="-I$srcdir/ -I$srcdir/opal/libltdl/"
# Must specifically mention $srcdir here for VPATH builds
# (this file is in the src tree).
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h.  */
#include <$srcdir/opal/libltdl/ltdl.h>

_ACEOF

That will turn into

//... < lots of defines > ....
#define OMPI_MPI_CONTRIBS "libompitrace, vt"
#define OPAL_C_HAVE_VISIBILITY 1
#define HAVE_DLFCN_H 1
#define LT_OBJDIR ".libs/"
/* end confdefs.h.  */

#include < ./opal/libltdl/ltdl.h > 

For local config and to

...
#include < /some/vpath/path/opal/libltdl/ltdl.h >

for VPATH.

So we don't actually need "-I." or "-I$srcdir" at all because conftest.c always contains correct path relative to the current configuration directory.
I updated my pull branch by changing the commit of interest.

@artpol84
Copy link
Contributor Author

I guess I need to go back to ompi and apply this patch there? Additionall pull request?

@jsquyres
Copy link
Member

Wait, let's figure this out before doing anything...

If we don't need the -I, then why did adding it fix your problem?

@artpol84
Copy link
Contributor Author

Sure,
I didn't add -I. I proposed to add -I$srcdir/opal/libltdl/. And now I think that we can also remove "-I."

@jsquyres
Copy link
Member

I know you didn't add -I. -- I just said -I, meaning the -I$srcdir... one. :-)

I was responding to your prior comment:

So we don't actually need "-I." or "-I$srcdir" at all ...

So if we need neither -I, then why did your adding -I$srcdir... fix the problem for you?

@artpol84
Copy link
Contributor Author

Because in conftest.c we include ltdl.h using its full path. And ltdl.h includes few others:

#include <libltdl/lt_system.h>
#include <libltdl/lt_error.h>

They are located in opal/libltdl/libltdl and are reffered by relative paths and inaccessible for gcc without corresponding -I$srcdir/opal/libltdl/.

P.S. Check the latest patch to clarify my current version.

@jsquyres
Copy link
Member

Ok, got it -- we do need the -I$srcdir.... Got it.

Yes, please push the final change back to master (i.e., delete the -I.), put the master hash here (per the requirement that we have to have all the relevant commits/functionality on master before bringing to v1.8, and then we'll be good to go.

@artpol84
Copy link
Contributor Author

Done: open-mpi/ompi@ce7102c

rhc54 pushed a commit that referenced this pull request Oct 21, 2014
Fix building system: correct libltdl check for advise capability.
@rhc54 rhc54 merged commit 427f2d5 into open-mpi:v1.8 Oct 21, 2014
mike-dubman added a commit to mike-dubman/ompi-release that referenced this pull request Jan 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants