Add checks for libevent.so conflict with LSF #7698

jjhursey · 2020-05-05T18:27:30Z

LSF ships a libevent.so that is no related to the libevent.so
shipped with Libevent.
Add some checks to the configure logic to detect scenarios where this
conflict can be detected, and provide the user with a descriptive
warning message.
- When detected by event/external this is just a warning since
  the internal component may be able to be used instead.
  - This happens when the user supplies the LSF path via the
    LDFLAGS envar instead of via --with-lsf-libdir.
- When detected by a LSF component and LSF was explicitly requested
  then this becomes an error. Otherwise, it will just print the warning
  and that component will fail to build.

* This should have been `LDFLAGS` not `LIBS`. Either works, but `LDFLAGS` is more correct. We should also include `CPPFLAGS` just in case the header is important to the check. Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>

jjhursey · 2020-05-05T18:29:12Z

Note: This is filed against v4.0.x first because the configury change spans ORTE and OMPI. We will be able to pick a part of this patch back to OMPI master, and the other part to PRRTE master (thus two commits).

For the first round of review, it was easier to keep them together in a single commit.

jjhursey · 2020-05-06T14:42:57Z

I'm keeping this as a draft for the moment. I'm looking at a libevent library linking advancement to tack onto this ticket.

jsquyres · 2020-05-06T14:51:48Z

opal/mca/event/external/configure.m4

+                                AC_MSG_WARN([on the system, but the linker is picking up the wrong version.])
+                                AC_MSG_WARN([])
+                                AC_MSG_WARN([Configure will continue and attempt to use the 'internal' libevent])
+                                AC_MSG_WARN([instead of the 'external' libevent.])


This part of the message isn't true if the user specified --with-event=external

Good point. I'll generalize the text. If they did specify --with-libevent=external then this message will print, but it will error out at the bottom of the configure check.

Pushed change. Take a look at the new wording.

I'll look at the new wording. You could also just selectively emit this paragraph.

opal/mca/event/external/configure.m4

config/orte_check_lsf.m4

jjhursey · 2020-05-06T16:08:35Z

I just re-pushed with the changes that Jeff suggested.

libevent vs libevent_core

I added 1 new commit that moves from using -levent to -levent_core for the core libevent functionality. I found some notes here that discuss the legacy nature of -levent. -levent_core has the functionality that we need. -levent has all of core plus a few things we don't need.

libevent_core.so exists as far back as Libevent 2.0.5 which is the oldest supported Libevent that Open MPI supports.

I kept that particular change out as a separate commit to isolate it. We may need to change the internal component, but I wanted to hold off on that given the changes to libevent happening in @bwbarrett 's branch. But I'd like Brian to comment on the libevent_core part of this change before deciding if we should include it or not. We might decide not to do it for v4, but to move to it for v5.

config/orte_check_lsf.m4

gpaulsen · 2020-05-08T16:42:16Z

It'd be nice if the non-orte commits were cherry-picked from an equivalent master PR.

jjhursey · 2020-05-09T06:07:58Z

@gpaulsen I wanted to get eyes on it all together before breaking it apart between OMPI and PRRTE for master. I'll prepare the master versions next week. For now, I think this is ready for review.

Note that the last commit might be too much for v4 (changing from -levent to -levent_core). So the v4 RMs can make a call on that, and we can drop that last commit. The prior commits will work independently of it.

config/orte_check_lsf.m4

* LSF ships a `libevent.so` that is no related to the `libevent.so` shipped with Libevent. * Add some checks to the configure logic to detect scenarios where this conflict can be detected, and provide the user with a descriptive warning message. - When detected by `event/external` this is just a warning since the internal component may be able to be used instead. - This happens when the user supplies the LSF path via the `LDFLAGS` envar instead of via `--with-lsf-libdir`. - When detected by a LSF component and LSF was explicitly requested then this becomes an error. Otherwise it will just print the warning and that component will fail to build. Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>

* `libevent_core.so` contains the core functionality that we depend upon - `libevent.so` library has been identified as the legacy target. - `libevent_core.so` exists as far back as Libevent 2.0.5 (oldest supported by OMPI) * `libevent_pthreads.so` can work with either `-levent` or `-levent_core` Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>

jjhursey · 2020-05-11T14:50:34Z

I'm not sure why the "Pull Request Build Checker" is stuck. It looks like the sub-jobs passed, but the main job status is still flashing. It's been stuck like that for 1 day 19 hr ago so far :(

gpaulsen · 2020-05-12T22:27:27Z

We discussed on today's call, and we cam to consensus that this change WOULD be appropriate for a v4.0.x release (changing dependency from libevent to libevent_core is okay, as it doesn't break any compatibility guarantees).

jjhursey · 2020-05-15T19:37:47Z

Note: master PRs have now been merged in their various repositories.

ompiteam-bot · 2020-05-15T19:37:50Z

Can one of the admins verify this patch?

rhc54 · 2020-05-15T20:33:40Z

There is a problem with this patch - it causes spurious warnings to be emitted if there is a system-level libevent (a good one) and you point at a custom one. Please don't merge this until that gets fixed!

jjhursey · 2020-05-15T21:25:16Z

The issue that @rhc54 mentioned is described here: openpmix/prrte#560
I'll take a look.

Signed-off-by: Joshua Hursey <jhursey@us.ibm.com> (cherry picked from commit 05e095a)

* Want to make sure that the result from `wc` is trimmed of spaces, so the `0` check returns properly * Add a few more comments, and fix wording in the warning message. Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>

jjhursey · 2020-05-18T19:14:45Z

I just pushed to commits that should address Ralph's concerns.

Related PRs:

Open MPI master (cherry-picked here) PR A slightly stronger check for LSF's libevent #7749
PRRTE master (Ported version of the second commit in this PR) Fix LSF configure check for libevent conflict openpmix/prrte#571

gpaulsen · 2020-05-18T20:42:31Z

@bwbarrett This looks like the (transient?) gcc6 issue: https://jenkins.open-mpi.org/jenkins/job/open-mpi.build.compilers/4220/Compiler=gcc6/console

gpaulsen · 2020-05-18T23:09:47Z

bot:retest

event/external: Fix typo in LDFLAGS vs LIBS var before check

22d8fa1

* This should have been `LDFLAGS` not `LIBS`. Either works, but `LDFLAGS` is more correct. We should also include `CPPFLAGS` just in case the header is important to the check. Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>

jsquyres reviewed May 6, 2020

View reviewed changes

jjhursey force-pushed the v4-fix-lsf-libevent branch from d51a67c to 88498c6 Compare May 6, 2020 16:02

jjhursey commented May 6, 2020

View reviewed changes

config/orte_check_lsf.m4 Outdated Show resolved Hide resolved

jjhursey commented May 6, 2020

View reviewed changes

config/orte_check_lsf.m4 Outdated Show resolved Hide resolved

jjhursey force-pushed the v4-fix-lsf-libevent branch from 88498c6 to 5f3d3f5 Compare May 9, 2020 06:04

jjhursey requested review from jsquyres, bwbarrett and gpaulsen May 9, 2020 06:08

jjhursey marked this pull request as ready for review May 9, 2020 06:08

jjhursey commented May 9, 2020

View reviewed changes

config/orte_check_lsf.m4 Outdated Show resolved Hide resolved

jjhursey added 2 commits May 9, 2020 14:55

jjhursey force-pushed the v4-fix-lsf-libevent branch from 5f3d3f5 to 886f41f Compare May 9, 2020 18:55

gpaulsen approved these changes May 11, 2020

View reviewed changes

jjhursey mentioned this pull request May 13, 2020

Move to libevent_core and add checks for libevent.so conflict with LSF #7734

Merged

rhc54 added the ⚠️ WIP-DNM! label May 15, 2020

jjhursey added 2 commits May 18, 2020 15:10

A slightly stronger check for LSF's libevent

7dba35f

Signed-off-by: Joshua Hursey <jhursey@us.ibm.com> (cherry picked from commit 05e095a)

Fix LSF configure check for libevent conflict

76500e6

* Want to make sure that the result from `wc` is trimmed of spaces, so the `0` check returns properly * Add a few more comments, and fix wording in the warning message. Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>

jjhursey removed the ⚠️ WIP-DNM! label May 19, 2020

hppritcha merged commit 63cc3da into open-mpi:v4.0.x May 19, 2020

hppritcha added the NEWS label May 19, 2020

jjhursey deleted the v4-fix-lsf-libevent branch May 19, 2020 15:36

ax3l mentioned this pull request Jan 4, 2021

macOS: levent_core missing for OpenMPI 4.0.5 #8332

Closed

Add checks for libevent.so conflict with LSF #7698

Add checks for libevent.so conflict with LSF #7698

Uh oh!

Conversation

jjhursey commented May 5, 2020

Uh oh!

jjhursey commented May 5, 2020

Uh oh!

jjhursey commented May 6, 2020

Uh oh!

jsquyres May 6, 2020

Choose a reason for hiding this comment

Uh oh!

jjhursey May 6, 2020

Choose a reason for hiding this comment

Uh oh!

jjhursey May 6, 2020

Choose a reason for hiding this comment

Uh oh!

jsquyres May 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjhursey commented May 6, 2020

libevent vs libevent_core

Uh oh!

Uh oh!

Uh oh!

gpaulsen commented May 8, 2020

Uh oh!

jjhursey commented May 9, 2020

Uh oh!

Uh oh!

jjhursey commented May 11, 2020

Uh oh!

gpaulsen commented May 12, 2020

Uh oh!

jjhursey commented May 15, 2020

Uh oh!

ompiteam-bot commented May 15, 2020

Uh oh!

rhc54 commented May 15, 2020

Uh oh!

jjhursey commented May 15, 2020

Uh oh!

jjhursey commented May 18, 2020

Uh oh!

gpaulsen commented May 18, 2020

Uh oh!

gpaulsen commented May 18, 2020

Uh oh!

Uh oh!