build: Custom libmpi name option in configure #2059

jjhursey · 2016-09-06T20:59:45Z

Add a configure time option to rename libmpi(_FOO).*
- --with-libmpi-name=STRING defaults STRING to mpi (so libmpi(_FOO).* is not changed)

jjhursey · 2016-09-06T21:07:34Z

@jsquyres This touches the build system, so I need someone with build system expertise to look over this to make sure I didn't miss anything obvious.

I'm doing some more testing now, but I think I caught all of the places that needed to change. Notice we are just focusing on libmpi.so not any of the other libmpi_ libraries like libmpi_mpifh.so.

An example:

shell$ ./autogen.pl --libmpi-name wookie_mpi
...
shell$  grep ompi_lib_prefix config/autogen_found_items.m4 
m4_define([ompi_lib_prefix], [wookie_mpi])
shell$ grep OMPI_LIB_PREFIX= configure
    OMPI_LIB_PREFIX=wookie_mpi
shell$ # build and install
...
shell$ cd install-dir
shell$ find . -name "libmpi.*"
shell$ find . -name "libwookie_mpi.*"
./lib/libwookie_mpi.so.0.0.0
./lib/libwookie_mpi.so.0
./lib/libwookie_mpi.so
./lib/libwookie_mpi.la

jjhursey · 2016-09-08T15:15:05Z

rebased and ready for some review (I'd like another set of eyes on this before pushing to master). @jsquyres If you get a chance can you take a look at this PR. No big rush on this, just when you get a chance.

jsquyres · 2016-09-21T01:29:22Z

@jjhursey Any reason not to also do libmpi_FOO and friends? It does look a little weird to do libjosh_mpi, but then have libmpi_mpifh (vs. libjosh_mpi_mpifh).

I mention this because from a sysadmin perspective, it's pretty obvious when you can ls -l libmpi* to see all the MPI libraries.

jsquyres · 2016-09-21T01:33:49Z

autogen.pl

@@ -1096,6 +1098,7 @@ sub in_tarball {
  --no-ompi | -no-ompi          Do not build the Open MPI layer
  --no-orte | -no-orte          Do not build the ORTE layer
  --no-oshmem | -no-oshmem      Do not build the OSHMEM layer
+  --libmpi-name | -libmpi-name  Override the default lib name of: lib$libmpi_name_arg


Just curious: why is this at autogen time (vs. configure time)?

I thought it was a little odd to set it during configure time since as part of a release we want this value fixed so users don't change it. However, I could probably be convinced otherwise. Just seemed like a little bit of protection.

I guess my default position on this is: we only put stuff in autogen.pl that can't go in configure.ac. Otherwise, you could make an argument that just about any configure CLI option should also (or only) be available in autogen.pl.

Indeed, we already have some branding-specific configure CLI options. For example, how is the library name different than --with-package-string=STRING or --with-ident-string=STRING?

jsquyres · 2016-09-21T01:40:21Z

configure.ac

@@ -278,6 +279,8 @@ fi
 OPAL_SET_LIB_PREFIX([])
 m4_ifdef([project_orte],
          [ORTE_SET_LIB_PREFIX([])])
+m4_ifdef([project_ompi],
+          [OMPI_SET_LIB_PREFIX([ompi_lib_prefix])])


I'm a little confused -- I see that we have the mechanisms here to add prefixes for the OPAL and ORTE libraries, but those don't appear to be used anywhere (i.e., we just pass in blank strings, so the prefixes are always empty -- there's no way to not have them be empty).

I'm therefore trying to remember why this functionality is here...? 😕

Per another email thread, those were added for a different external project (ORCM, I believe). Where they wanted to prefix libopen-pal.so to something like liborcm_open-pal.so. I just piggybacked on that mechanism to achieve this patch.

If there is no reason to have the OPAL_LIB_PREFIX and ORTE_LIB_PREFIX then we can probably remove them. But if there is a chance that other projects might want to do such renaming then we might as well leave them in. They are not hurting anything.

I agree -- no real reason to remove them.

jsquyres · 2016-09-21T01:41:43Z

ompi/Makefile.am

-lib_LTLIBRARIES = libmpi.la
-libmpi_la_SOURCES =
-libmpi_la_LIBADD = \
+lib_LTLIBRARIES = lib@[email protected]


It's a little odd to call this value a "prefix", when, in fact, it's the entire library name. I.e., it's:

lib@[email protected]

not

lib@[email protected]

The latter would follow suit for what was done with the other libraries. I.e., you could set the prefix to be josh_, and then it would be libjosh_mpi (etc.).

Is there a reason you want it to be the entire library name, rather than a prefix?

We were hoping to use this mechanism to rename libmpi to libmpi_ibm - so really a postfix. When preparing the patch I made it a bit more broad to allow both prefix and postfix by replacing the whole name. However, the variable OMPI_LIB_PREFIX is misleading so adopting OMPI_LIBMPI_NAME or OMPI_LIBRARY_NAME is probably better.

I can make that change once we settle on what the variable should be called.

Ah, you want a suffix.

Ok, in classic "Jeff wants a long name" form, I'd like OMPI_MPI_LIBRARY_NAME. I'd settle for OMPI_MPI_LIB_NAME, too. I think the extra MPI is important in there because, as a project, OMPI has lots of libraries. In this case, you're specifically talking about the MPI library.

But then again, we have several MPI libraries... So it would be good to see this for all the MPI libraries -- Fortran and C++ too. E.g.:

lib$(OMPI_MPI_LIB_NAME).la

lib$(OMPI_MPI_LIB_NAME)_mpifh.la

...etc.

No objection to doing the Fortran names also. I think longer term the consistency would be desirable. Short term (for our 2.0 based release), we're only interested in the libmpi_suffix, and if the community default of empty string is applied, this change should be an effective no-op for the Open MPI v2.x branch.

rhc54 · 2016-09-21T14:37:50Z

To fully maintain jeff's reputation, i think the variable needs to be OPEN_MESSAGE_PASSING_LIBRARY_FOR_FORTRAN_AS_WELL_AS_C_AND_C++_AND_PYTHON_LIBRARY_NAME

jjhursey · 2016-09-21T15:02:39Z

In reply to @jsquyres comment:

Any reason not to also do libmpi_FOO and friends? It does look a little weird to do libjosh_mpi, but then have libmpi_mpifh (vs. libjosh_mpi_mpifh).

The truth is that we do not need it right now. However, I could envision a time where that might change. For the near term we only want to change libmpi, in the future that might change to include all libmpi_FOO - so we can extend the patch to include that. However, I would need the ability to adjust one and not the other for a while.

How about this logic as a proposal:

# These two variable set either via autogen or configure
# Name of libmpi
#  -- Default 'mpi' (as in libmpi)
OMPI_LIBMPI_NAME=
# Name of libmpi_FOO
#  -- Default 'mpi' (as in libmpi_mpifh)
OMPI_LIBMPI_FOO_NAME=

# Setting OMPI_LIBMPI_NAME will also set OMPI_LIBMPI_FOO_NAME, if not already specified
if( OMPI_LIBMPI_NAME != "" && OMPI_LIBMPI_FOO_NAME == "") {
   OMPI_LIBMPI_FOO_NAME=OMPI_LIBMPI_NAME
}

jjhursey · 2016-09-21T15:21:54Z

I spoke with @jsquyres on the phone a little bit about this, and I think we came up with a workaround for the OMPI_LIBMPI_NAME vs OMPI_LIBMPI_FOO_NAME comment that would not push that distinction to the community code.

So, per the review, I'm going to make the following changes:

Move the prefix option from autogen time to configure time
Create a single OMPI_LIBMPI_NAME variable that will update libmpi and libmpi_FOO
- Something like lib@OMPI_LIBMPI_NAME@ and lib@OMPI_LIBMPI_NAME@mpifh
- Update all of the libmpi_FOO places.

jjhursey · 2016-09-27T14:43:12Z

@jsquyres I've updated the commit per the review. I think I addressed everything.

Example usage:

shell$ ./configure OMPI_LIBMPI_NAME=wookie
...
shell$ find . -name "libmpi*"
shell$ find . -name "libwookie*"
./lib/libwookie.so.0.0.0
./lib/libwookie.so.0
./lib/libwookie.so
./lib/libwookie.la
./lib/libwookie_mpifh.so.0.0.0
./lib/libwookie_mpifh.so.0
./lib/libwookie_mpifh.so
./lib/libwookie_mpifh.la
./lib/libwookie_usempi.so.0.0.0
./lib/libwookie_usempi.so.0
./lib/libwookie_usempi.so
./lib/libwookie_usempi.la
shell$

gpaulsen · 2016-09-27T17:00:58Z

libwookie will be THE new official Open MPI library name right? It follows the Star Wars [tm] naming conventions of the components. Plus MPI IS quite Hairy in places.

jsquyres · 2016-09-27T18:13:06Z

Per discussion on the webex this morning, a --with option would likely be better (e.g., to be similar to the already-existing --with-ident-string=STRING). Hopefully, this is a small / trivial change to make. It would probably be good to document this switch in README, too (perhaps even putting an admonishment that users are not expected to use this option).

jjhursey · 2016-09-27T18:51:44Z

Latest commit changes the mechanism to a configure option: --with-libmpi-name=STRING
Also updated the README

jjhursey · 2016-09-27T21:40:17Z

Rebased - preparing for merge. Also fixed one corner case seen in diff below (case --with-libmpi-name with no =):

diff --git a/config/opal_set_lib_prefix.m4 b/config/opal_set_lib_prefix.m4
index 46ee4f73..69b65930 100644
--- a/config/opal_set_lib_prefix.m4
+++ b/config/opal_set_lib_prefix.m4
@@ -47,7 +47,7 @@ AC_DEFUN([OMPI_SET_LIB_NAME],[
     AC_ARG_WITH([libmpi-name],
         [AC_HELP_STRING([--with-libmpi-name=STRING],
                 ["Replace \"libmpi(_FOO)\" with \"libSTRING(_FOO)\" (default=mpi)"])])
-    if test "$with_libmpi_name" = "" || test "$with_libmpi_name" = "no"; then
+    if test "$with_libmpi_name" = "" || test "$with_libmpi_name" = "no" || test "$with_libmpi_name" = "yes"; t
         with_libmpi_name="mpi"
     fi

jsquyres · 2016-09-27T21:49:03Z

@jjhursey Do you want to error if someone specifies --with-mpi-name (without a string) or --without-mpi-name?

jjhursey · 2016-09-28T15:10:25Z

I followed the logic used by --with-ident-string
https://github.com/open-mpi/ompi/blob/master/config/opal_configure_options.m4#L421-L427

Which, looking at that logic, would have the same problem I noted a couple comments ago where if the user supplies --with-ident-string (instead of --with-ident-string=) then with_ident_string will be set to yes.

In any case, having a error in the following case is probably good.

--without-libmpi-name ($with_libmpi_name = "no")

These cases would be the 'default', so no warning if the user specified it or not.

--with-libmpi-name ($with_libmpi_name = "yes")
--with-libmpi-name= ($with_libmpi_name = "") Default value set if no flag specified by the user.

* Add a configure time option to rename libmpi(_FOO).* - `--with-libmpi-name=STRING` * This commit only impacts the installed libraries. Internal, temporary libraries have not been renamed to limit the scope of the patch to only what is needed. For example: ```shell shell$ ./configure --with-libmpi-name=wookie ... shell$ find . -name "libmpi*" shell$ find . -name "libwookie*" ./lib/libwookie.so.0.0.0 ./lib/libwookie.so.0 ./lib/libwookie.so ./lib/libwookie.la ./lib/libwookie_mpifh.so.0.0.0 ./lib/libwookie_mpifh.so.0 ./lib/libwookie_mpifh.so ./lib/libwookie_mpifh.la ./lib/libwookie_usempi.so.0.0.0 ./lib/libwookie_usempi.so.0 ./lib/libwookie_usempi.so ./lib/libwookie_usempi.la shell$ ```

jjhursey · 2016-09-28T17:10:20Z

Pushed a rebased version with the suggested change to make --without-libmpi-name and error:

shell$ ./configure --without-libmpi-name
...
checking if want custom libmpi(_FOO) name... Error
configure: WARNING: Invalid to specify --without-libmpi-name
configure: error: Cannot continue

@jsquyres I think this is good to go. Any other comments?

jsquyres · 2016-09-30T15:19:57Z

Sorry for chiming in late: 👍

jjhursey · 2016-09-30T15:49:00Z

In preparing the PRs I noticed a bug in the resolution of the libmpi name for the fortran wrapper compiler. PR to fix that coming shortly.

jjhursey added enhancement ⚠️ WIP-DNM! labels Sep 6, 2016

jjhursey added this to the v2.1.0 milestone Sep 6, 2016

jjhursey assigned jsquyres and jjhursey Sep 6, 2016

jjhursey force-pushed the topic/custom-libmpi branch from b33dcce to efd0933 Compare September 8, 2016 14:38

jjhursey removed the ⚠️ WIP-DNM! label Sep 8, 2016

jsquyres requested changes Sep 21, 2016

View reviewed changes

jsquyres added the pushed-back label Sep 26, 2016

jjhursey force-pushed the topic/custom-libmpi branch from cd5abe2 to 19c3af7 Compare September 27, 2016 14:38

jjhursey changed the title ~~build: Custom libmpi name option in autogen.pl~~ build: Custom libmpi name option in configure Sep 27, 2016

jjhursey force-pushed the topic/custom-libmpi branch from 19c3af7 to 0cd2a20 Compare September 27, 2016 18:48

jsquyres approved these changes Sep 27, 2016

View reviewed changes

jjhursey force-pushed the topic/custom-libmpi branch from b99ba59 to 5ee6573 Compare September 27, 2016 21:38

jjhursey removed the pushed-back label Sep 27, 2016

jjhursey force-pushed the topic/custom-libmpi branch from 5ee6573 to 6e1a285 Compare September 28, 2016 17:07

jjhursey merged commit f6f24a4 into open-mpi:master Sep 30, 2016

jjhursey deleted the topic/custom-libmpi branch September 30, 2016 03:14

This was referenced Sep 30, 2016

v2.0.2: build: Custom libmpi(_FOO) name option in configure #2138

Closed

v2.1.0: build: Custom libmpi(_FOO) name option in configure #2139

Merged

jjhursey mentioned this pull request Sep 30, 2016

build: Custom libmpi_FOO name fix for wrapper compilers #2142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Custom libmpi name option in configure #2059

build: Custom libmpi name option in configure #2059

jjhursey commented Sep 6, 2016 •

edited

Loading

jjhursey commented Sep 6, 2016

jjhursey commented Sep 8, 2016

jsquyres commented Sep 21, 2016

jsquyres Sep 21, 2016

jjhursey Sep 21, 2016

jsquyres Sep 21, 2016

jsquyres Sep 21, 2016

jjhursey Sep 21, 2016

jsquyres Sep 21, 2016

jsquyres Sep 21, 2016 •

edited

Loading

jjhursey Sep 21, 2016

jsquyres Sep 21, 2016

gpaulsen Sep 21, 2016

rhc54 commented Sep 21, 2016

jjhursey commented Sep 21, 2016

jjhursey commented Sep 21, 2016

jjhursey commented Sep 27, 2016

gpaulsen commented Sep 27, 2016

jsquyres commented Sep 27, 2016

jjhursey commented Sep 27, 2016

jjhursey commented Sep 27, 2016

jsquyres commented Sep 27, 2016

jjhursey commented Sep 28, 2016 •

edited

Loading

jjhursey commented Sep 28, 2016

jsquyres commented Sep 30, 2016

jjhursey commented Sep 30, 2016

build: Custom libmpi name option in configure #2059

build: Custom libmpi name option in configure #2059

Conversation

jjhursey commented Sep 6, 2016 • edited Loading

jjhursey commented Sep 6, 2016

jjhursey commented Sep 8, 2016

jsquyres commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsquyres Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhc54 commented Sep 21, 2016

jjhursey commented Sep 21, 2016

jjhursey commented Sep 21, 2016

jjhursey commented Sep 27, 2016

gpaulsen commented Sep 27, 2016

jsquyres commented Sep 27, 2016

jjhursey commented Sep 27, 2016

jjhursey commented Sep 27, 2016

jsquyres commented Sep 27, 2016

jjhursey commented Sep 28, 2016 • edited Loading

jjhursey commented Sep 28, 2016

jsquyres commented Sep 30, 2016

jjhursey commented Sep 30, 2016

jjhursey commented Sep 6, 2016 •

edited

Loading

jsquyres Sep 21, 2016 •

edited

Loading

jjhursey commented Sep 28, 2016 •

edited

Loading