Fix DIR, DIR/include search for --with-pmix #4683

pkovacs · 2018-01-08T06:58:38Z

This PR allows ompi to find external pmix headers that were installed to a DIR/include/pmix subdirectory.

When packages contain multiple header files, it is common and desirable to be able to place the headers in a project-named subdirectory. Doing so with external pmix, e.g. installing headers to /usr/include/pmix, renders the headers unusable to ompi.

To address the issue, an optional new argument (action-if-failed) was added to OPAL_CHECK_WITHDIR so that, instead of halting the configure immediately, the caller can, if desired, capture the failure and try a different directory. New code was added to opal_check_pmi.m4 to do just that: first try DIR/include and then try DIR/include/pmix.

./configure --with-pmix=/usr --with-libevent=/usr ...

checking --with-external-pmix value... not found (/usr/include)
checking --with-external-pmix value... sanity check ok (/usr/include/pmix)

ompiteam-bot · 2018-01-08T06:58:40Z

Can one of the admins verify this patch?

rhc54 · 2018-01-08T14:32:51Z

@jsquyres Any thoughts on this?

rhc54 · 2018-01-08T14:32:58Z

ok to test

ggouaillardet · 2018-01-09T09:35:18Z

config/opal_check_pmi.m4

@@ -358,9 +363,9 @@ AC_DEFUN([OPAL_CHECK_PMIX],[
           LDFLAGS=$opal_external_pmix_save_LDFLAGS
           LIBS=$opal_external_pmix_save_LIBS

+           opal_external_pmix_CPPFLAGS="-I$pmix_ext_install_include_dir"


we do not want -I/usr/include in CPPFLAGS

@ggouaillardet I can filter that case, sure. When the prefix is /usr, that var might be /usr/include or /usr/include/pmix.

ggouaillardet

Does PMIx install its headers in /usr/include/pmix by default ?

FWIW, we handle SLURM (that might install its headers in /usr/include/slurm) a bit differently, and i think we should be consistent in how we handle these exceptions.

pkovacs · 2018-01-09T14:25:28Z

@ggouaillardet PMIx installs its multiple headers to /usr/include by default. Goal here was to support the possibility that distros might want to install to a subdirectory corresponding to a package name., e.g. /usr/include/pmix via:

# pmix configure
./configure --includedir=/usr/include/pmix ...

The above provides better include organization. I'm not changing any default behavior in any case, just supporting the above possibility. I'll make the change you suggested above, filtering /usr/include.

rhc54 · 2018-01-09T14:30:38Z

I'm wondering if this is simply exposing a small problem in the pmix configure logic itself. We already have the ability to specify the include location separate from the lib directory. It sounds, though, like we aren't first testing the exact path given to us before appending "include" or other subdirectories.

If that is true, then we should probably just fix that problem.

pkovacs · 2018-01-09T14:44:09Z

@rhc54 If you're suggesting that pmix should install headers to %{prefix}/include/pmix by default I'm quite happy with that, although ompi would still not be able to find them without this PR.

jsquyres · 2018-01-09T15:06:04Z

FWIW, I think the way that we have handled this before is to do something like:

$ export FOO_DIRr=/path/to/foo
$ ./configure \
    --with-FOO=$FOO_DIR/include \
    --with-FOO-libdir=$FOO_DIR/lib ...

I.e., specify the includedir as the --with-FOO clause and then specify the libdir via --with-FOO-libdir. We debated this back in the beginning (when creating the --with-FOO[-libdir] options) and decided on this approach instead of:

User specifies --with-FOO, or
User specifies --with-FOO-incdir and --with-FOO-libdir

The rationale was that checking for the all the corner cases and error cases for the above scenario was a royal pain, and just created yet another CLI option (i.e., the --with-FOO-incdir option).

Is using --with-pmix=/usr/include/pmix --with-pmix-libdir=/usr/lib not sufficient?

EDIT AFTER THE FACT This is wrong. This comment should be ignored. ☹️

rhc54 · 2018-01-09T15:07:32Z

I fear you misunderstood me - let me clarify. We are happy with how/where PMIx installs. Per @jsquyres, we do have the two options. Is the problem simply that the configure logic isn't checking the --with-pmix=path-to-include-files argument before trying it with "include" appended?

artpol84 · 2018-01-09T15:09:14Z

bot:mellanox:retest

ggouaillardet · 2018-01-09T15:16:14Z

@jsquyres if that works, that would be best for me !
i bet -L/usr/lib would end up in the LDFLAGS, but this is a bug that can be easily fixed.

@pkovacs SLURM is known to install its header in /usr/include/slurm, so it makes sense for Open MPI to try that automatically. If PMIx is not known to install its headers in /usr/include/pmix, i do not see any rationale on why Open MPI would search there automatically. This is only my view, so let's say this will become the de facto standard in a near future. In this case, i'd rather have SLURM and PMIx use the same logic (and i have no preference) in order to keep the code consistent and easier to maintain.

pkovacs · 2018-01-09T15:18:44Z

Below is exactly the problem and my motivation here.

Is using --with-pmix=/usr/include/pmix --with-pmix-libdir=/usr/lib not sufficient?

Does not work given the putative example of pmix headers installed to /usr/include/pmix.

ompi silently appends /include, looks in /usr/include/pmix/include and throws up its hands.

rhc54 · 2018-01-09T15:20:53Z

Understood - as I said, that is a bug in the OMPI configure logic. It is supposed to first check in the exact given path, and only if not found should it append "include"

pkovacs · 2018-01-09T15:25:48Z

OK, now we're on the same page!

My code needs to be revised. Instead of attempting DIR/include and then DIR/include/pmix, I should search DIR then DIR/include to be consistent with the rest of the universe.

Got it! Gimme a bit to correct the code here.

ggouaillardet · 2018-01-09T15:30:44Z

Note OPAL_CHECK_PACKAGE is not used here, so that could explain the "missing logic"

jsquyres · 2018-01-09T15:33:43Z

@ggouaillardet Excellent point that I totally missed. Is there a reason OPAL_CHECK_PACKAGE is not used here?

ggouaillardet · 2018-01-09T15:36:48Z

Could this be because of the many PMIx dependencies that make linking a dummy test program non trivial ?

rhc54 · 2018-01-09T15:38:07Z

Errr...."many dependencies"? I'm only aware of libevent

pkovacs · 2018-01-09T15:46:24Z

There's extra code in the check to discern the pmix version (1x, 2x, 3x) from the headers. Presumably
OPAL_CHECK_PACKAGE can be rolled in front of that section.

artpol84 · 2018-01-10T02:27:37Z

bot:mellanox:retest

artpol84 · 2018-01-10T04:02:57Z

looks like leftover in /dev/shm after some failed runs has caused this failure.

artpol84 · 2018-01-10T04:03:05Z

bot:mellanox:retest

artpol84 · 2018-01-10T04:46:17Z

@jladd-mlnx @vspetrov looks like disabling hcoll helps.
Please let me know when the issue is fixed and we can turn it on again.
I also suggest to have some test running with hcoll disabled so it will be easier to identify issues in the future.

Signed-off-by: Philip Kovacs <pkdevel@yahoo.com>

pkovacs · 2018-01-10T22:32:56Z

Changes pushed. The OPAL_CHECK_PMIX code is simplified by using OPAL_CHECK_PACKAGE. The check for pmix_version.h now uses the more robust AC_CHECK_HEADER with the CPPFLAGS returned from check package, instead of the manual ls command.

Note that OPAL_CHECK_PACKAGE did not actually search DIR then DIR/include when using --with-foo=<dir> (to my surprise). I added a clause there to perform the missing DIR check.

rhc54 · 2018-01-10T23:14:58Z

I'm not seeing the test for DIR/include - can you point it out to me?

pkovacs · 2018-01-10T23:32:55Z

The DIR/include check was already there and is now on lines 62-69 under my addition to opal_check_package.m4.

rhc54 · 2018-01-11T00:24:43Z

Got it - thanks!

alex-mikheev · 2018-01-16T09:39:15Z

config/opal_check_package.m4

+          [AS_IF([test "$dir_prefix" != ""],
+                 [$1_CPPFLAGS="$$1_CPPFLAGS -I$dir_prefix"
+                  CPPFLAGS="$CPPFLAGS -I$dir_prefix"])
+          AC_CHECK_HEADERS([$2], [opal_check_package_header_happy="yes"], [], [$6])


@pkovacs @jsquyres @rhc54 @ggouaillardet @yosefe @jladd-mlnx This check is wrong when the package headers are installed in /usr/include but configure was called with the specific package dir. The check will pickup h file from the /usr/include and add -I$dir_prefix to the CPPFLAGS which is wrong.

OK, so we can add /usr/include and /usr/local/include filters to the to code block above that (lines 44, 45) which will handle the --with-foo=/usr/include and --with-foo='/usr/include/local cases without adding unnecessary CPPFLAGS

AS_IF([test "$dir_prefix" = "/usr" || \ test "$dir_prefix" = "/usr/local || \ test "$dir_prefix" = "/usr/include || \ test "$dir_prefix" = "/usr/local/include"],

Added #4722 to correct the concern above.

pkovacs mentioned this pull request Jan 8, 2018

PMIx packaging #4072

Closed

ggouaillardet reviewed Jan 9, 2018

View reviewed changes

jsquyres mentioned this pull request Jan 9, 2018

CI/Jenkins failures: timeouts #4686

Closed

Fix DIR, DIR/include search for --with-pmix

2e3f401

Signed-off-by: Philip Kovacs <pkdevel@yahoo.com>

pkovacs changed the title ~~Add DIR/include/pmix to search path for external --with-pmix~~ Fix DIR, DIR/include search for --with-pmix Jan 10, 2018

rhc54 merged commit 3c78b85 into open-mpi:master Jan 11, 2018

pkovacs deleted the master-pmix-dirs branch January 11, 2018 04:21

alex-mikheev reviewed Jan 16, 2018

View reviewed changes

jsquyres mentioned this pull request Jan 17, 2018

opal_check_package: filter /usr[/local]/include from CPPFLAGS #4722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DIR, DIR/include search for --with-pmix #4683

Fix DIR, DIR/include search for --with-pmix #4683

pkovacs commented Jan 8, 2018

ompiteam-bot commented Jan 8, 2018

rhc54 commented Jan 8, 2018

rhc54 commented Jan 8, 2018

ggouaillardet Jan 9, 2018

pkovacs Jan 9, 2018

ggouaillardet left a comment

pkovacs commented Jan 9, 2018 •

edited

Loading

rhc54 commented Jan 9, 2018

pkovacs commented Jan 9, 2018

jsquyres commented Jan 9, 2018 •

edited

Loading

rhc54 commented Jan 9, 2018

artpol84 commented Jan 9, 2018

ggouaillardet commented Jan 9, 2018

pkovacs commented Jan 9, 2018

rhc54 commented Jan 9, 2018

pkovacs commented Jan 9, 2018

ggouaillardet commented Jan 9, 2018

jsquyres commented Jan 9, 2018

ggouaillardet commented Jan 9, 2018

rhc54 commented Jan 9, 2018

pkovacs commented Jan 9, 2018

artpol84 commented Jan 10, 2018

artpol84 commented Jan 10, 2018

artpol84 commented Jan 10, 2018

artpol84 commented Jan 10, 2018

pkovacs commented Jan 10, 2018 •

edited

Loading

rhc54 commented Jan 10, 2018

pkovacs commented Jan 10, 2018 •

edited

Loading

rhc54 commented Jan 11, 2018

alex-mikheev Jan 16, 2018

pkovacs Jan 16, 2018 •

edited

Loading

pkovacs Jan 16, 2018

Fix DIR, DIR/include search for --with-pmix #4683

Fix DIR, DIR/include search for --with-pmix #4683

Conversation

pkovacs commented Jan 8, 2018

ompiteam-bot commented Jan 8, 2018

rhc54 commented Jan 8, 2018

rhc54 commented Jan 8, 2018

ggouaillardet Jan 9, 2018

Choose a reason for hiding this comment

pkovacs Jan 9, 2018

Choose a reason for hiding this comment

ggouaillardet left a comment

Choose a reason for hiding this comment

pkovacs commented Jan 9, 2018 • edited Loading

rhc54 commented Jan 9, 2018

pkovacs commented Jan 9, 2018

jsquyres commented Jan 9, 2018 • edited Loading

rhc54 commented Jan 9, 2018

artpol84 commented Jan 9, 2018

ggouaillardet commented Jan 9, 2018

pkovacs commented Jan 9, 2018

rhc54 commented Jan 9, 2018

pkovacs commented Jan 9, 2018

ggouaillardet commented Jan 9, 2018

jsquyres commented Jan 9, 2018

ggouaillardet commented Jan 9, 2018

rhc54 commented Jan 9, 2018

pkovacs commented Jan 9, 2018

artpol84 commented Jan 10, 2018

artpol84 commented Jan 10, 2018

artpol84 commented Jan 10, 2018

artpol84 commented Jan 10, 2018

pkovacs commented Jan 10, 2018 • edited Loading

rhc54 commented Jan 10, 2018

pkovacs commented Jan 10, 2018 • edited Loading

rhc54 commented Jan 11, 2018

alex-mikheev Jan 16, 2018

Choose a reason for hiding this comment

pkovacs Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

pkovacs Jan 16, 2018

Choose a reason for hiding this comment

pkovacs commented Jan 9, 2018 •

edited

Loading

jsquyres commented Jan 9, 2018 •

edited

Loading

pkovacs commented Jan 10, 2018 •

edited

Loading

pkovacs commented Jan 10, 2018 •

edited

Loading

pkovacs Jan 16, 2018 •

edited

Loading