Improving robustness in opal_check_package header checking.

[avilcheslopez]

This is to make the header checking in the opal_check_package macro a little bit more robust (specifically in "_OPAL_CHECK_PACKAGE_HEADER"). Basically, in addition to looking in /include, it also tries looking in the provided path directly.

[rhc54]

@avilcheslopez looks like this broke the hcoll m4 - not sure why, but I wonder if we lost one of the optional sub-directories with the change?

[avilcheslopez]

@rhc54: :o! Will look into this...

[avilcheslopez]

@rhc54: I made some changes and the issue has been resolved now :)

[rhc54]

@jsquyres @goodell I think this looks okay, but would appreciate your review

[goodell]

Adding the fourth argument to AC_CHECK_HEADERS makes this line disagree with the AC_VERBOSE message immediately above, and is certainly wrong if CPPFLAGS is not modified. I don't think this part of the change is good.

[avilcheslopez]

Oh, I see. Sure, I can fix this.

[goodell]

@avilcheslopez would you explain a bit about why you wanted this change? The initial comment in the PR doesn't explain the failure case you were hitting. I.e., what were you building with which arguments that was causing a problem? That information should be present in both the PR and the commit message, otherwise this change appears to be gratuitous. The primary user of this macro is OPAL_CHECK_PACKAGE and this change doesn't make sense for that usage unless you have some library with headers in a funny path but the library in some standard path.

The addition of a third stanza of AC_CHECK_HEADERS makes me feel like this should almost be a loop and/or use a utility macro to stamp out those stanzas instead to prevent us from making copy/paste errors. I don't feel strongly here, but this sort of repetition is a common way for us to make hard-to-review mistakes.

Also, the two commits here should be squashed together before this PR is pulled (assuming we do end up actually pulling it).

[jsquyres]

I agree with @goodell -- can you cite the failure you're trying to fix? It would be unusual for someone to specify --with-foo=/path/to/foo/include, or even --with-foo=/path/to/foo, and have foo.h be in that top-level directory.

[avilcheslopez]

@jsquyres, @goodell: actually, yes, that is exactly the issue we're having: one of our components depends on a package that installs the headers in a funny path while the libraries are installed in the standard path. So we thought making the _OPAL_CHECK_PACKAGE_HEADER macro more robust to check in besides /include would be helpful. If you find this solution acceptable, I can definitely go ahead and squash the commits and improve the commit message as you suggested. However, if there is a better approach to dealing with this, I'm definitely open to that as well.

[jsquyres]

What package is doing this funny installation? Can that package be fixed? It would certainly be more social to have a dependent package install itself like the rest of the world does rather than force us to put in weird workarounds. :-)

[avilcheslopez]

It's postgres :o. So unfortunately, it can't be fixed. However, it's not entirely inconsistent with standard practices, it's just inconsistent across distributions and versions. This is the funny behavior:

• On some distributions, it installs everything under /usr/pgsql-<version> (so headers go into /usr/pgsql-<version>/include and libs go into /usr/pgsql-<version>/libs). So in this case, supplying the prefix /usr/pgsql-<version> works perfectly.
• On some distributions it installs headers under /usr/include/pgsql or /usr/include/postgres, while installing libraries in /usr/lib. In this case, in theory the prefix is /usr and applications should #include <pgsql/libpq-fe.h> or #include <postrgres/libpq-fe.h>. But with this inconsistent behavior, we have to figure out what to #include in our programs.

Perhaps there already is a method for handling these situations and I haven't looked into it.

[jsquyres]

@avilcheslopez Ew -- it has multiple different #include paths? Bad Postgres! Bad, bad Postgres!

I think what you want to do is check into what happens when you #include <pg_dir/FOO> -- how does that file include other PG header files? Does it:

1. #include <BAR>
2. #include <pgsql/BAR>
3. #include <postgres/BAR>

Or is that answer different on different distros?

It is significantly easier if the answer is the same on all distros (which would likely be option number 1). Because then your code should include PG header files the same way that they do -- i.e., in your component .c files, follow exactly the same convention that they do for including header files, and then just have a -I that allows that convention.

You can figure which -I to use with a few test -d statements in your configure.m4, and then pass that value in via the extra_includes argument to OPAL_CHECK_PACKAGE. No changes should be necessary to OPAL_CHECK_PACKAGE.

If each distro changes the convention of how you include Postgres header files, then a) the Postgres authors and packagers should be flogged, and b) what we need to do here in OMPI gets significantly more complex. Let's hope that's not the case.

[avilcheslopez]

@jsquyres: I looked at the Postgres header files, and they include other Postgres header files like this: #include "other-pg-file.h". This makes sense as they're header files that belong to the same library and they're always going to be next to each other (wherever they're installed on different distros).

Ideally, I would like to simply do one of the following: #include <libpq-fe.h> or #include <pgsql/libpq-fe.h>. And whatever that is, it should work consistently across distros by simply passing the correct prefix in the with-postgres configure flag. But unfortunately, because of their inconsistency, I can't do that. What we're doing at the moment is #include <libpq-fe.h>, and on distros where headers get installed in /usr/include/<pgsql | postgres>, we pass that as the "prefix" (even though it's not really a prefix) and the correct -I flag gets generated (with this patch we're proposing, of course).

Now, I could add some extra logic in my component's configure.m4 to figure out what's going on and based on that pass the appropriate parameters to OPAL_CHECK_PACKAGE. For example, in the case of the headers being installed in /usr/include/pgsql, I would call OPAL_CHECK_PACKAGE with /usr as the directory prefix and pgsql/libpq-fe.h as the header file to look for. However, OPAL_CHECK_PACKAGE will set the -I flag in <prefix>_CPPFLAGS (opal_db_postgres_CPPFLAGS in my case) to a path that won't work for my component since it's doing #include <libpq-fe.h>. My configure.m4 file would have to additionally "correct" the opal_db_postgres_CPPFLAGS variable before AC_SUBST()'ing it. So after all that extra work, there wouldn't be much value in calling OPAL_CHECK_PACKAGE.

I agree that changing OPAL_CHECK_PACKAGE to accommodate for this issue is not the correct solution (especially since we're abusing the meaning of "directory prefix"). I considered this patch simply because regardless of all these issues, it seemed to add some robustness to the macro (i.e. it's not really "tailored" to our particular issue with postgres, but it enables our "workaround"). I don't know if we could maybe pass another flag besides with-postgres to provide a hint regarding where to look for header files. Alternatively, I don't know if there's a way to add some logic in my program to #include <libpq-fe.h> with the correct path.

I don't know... Any ideas? I'm okay with adding more logic to my component's configure.m4 file.

[goodell]

I'm leaning towards you just fixing this up in your component's configure.m4 file.

Another option is that we could update the OPAL_CHECK_PACKAGE macro (or a copy of it) to add configure arguments like: --with-PACKAGE-includes=DIR and --with-PACKAGE-libdir. I think there's some (at least partial) precedent for this in OMPI, and there's definitely precedent for this in other packages.

[avilcheslopez]

Yeah, I like the idea of the additional "--with" options. In the meantime, I'll close this request and work on a fix within our configure.m4 file.