configury/libevent: fix incorrect drop of OPAL_HAVE_WORKING_EVENTOPS

[karasevb]

Fixes PR #1687
The code that sets OPAL_HAVE_WORKING_EVENTOPS for internal libevent
was executed even if the external libevent component was configured.

As the result libevent progress wasn't called in opal_progress which
for example caused ring_c to hang when pml/ob1 was used.

[karasevb]

@jsquyres @ggouaillardet, please review.

[artpol84]

@jsquyres @hppritcha @jladd-mlnx I guess we want that in 2.0.1 so I'm setting it as the milestone. Fix me if need.

[ggouaillardet]

Will do tomorrow

At first glance, and being afk, I am a bit puzzledby the fix.
(E.g. an undefined variable is better than a variable defined with value "0")
could we simply use this variable incorrectly ?
E.g. #ifdef instead of #if ?
Should we test this variable and an other as well to handle external libevent ?
What if external libevent has a working eventops (or not) ?

[artpol84]

This variable will be set correctly in the corresponding external *.m4 file
https://github.com/open-mpi/ompi/blob/master/opal/mca/event/external/configure.m4#L158:L167

Right now it is set to 1 there and then dropped in the subsequently executed "internal" *.m4 file here:
https://github.com/open-mpi/ompi/blob/master/opal/mca/event/libevent2022/configure.m4#L195:L199

We shouldn't touch this variable in the "internal" component if "external" one was requested. We just want to configure the internal one to be able to do "distclean" later.

[ggouaillardet]

Got it, thanks !

So can we improve the fix by explaining this in the m4 (comment) and commit message ?

[artpol84]

Sure

четверг, 4 августа 2016 г. пользователь Gilles Gouaillardet написал:

Got it, thanks !

So can we improve the fix by explaining this in the m4 (comment) and
commit message ?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1937 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AHL5PjspHwQMcdgiRRtafMPT8PENBv71ks5qcdUxgaJpZM4Jckc3
.

---

Best regards, Artem Polyakov
(Mobile mail)

[jsquyres]

I'm not sure I understand the references to fixing PR #1687 -- did you mean to refer to a different issue?

Isn't this a simpler fix?

diff --git a/opal/mca/event/libevent2022/configure.m4 b/opal/mca/event/libevent2
index 5ace9dc..79a5589 100644
--- a/opal/mca/event/libevent2022/configure.m4
+++ b/opal/mca/event/libevent2022/configure.m4
@@ -195,8 +195,7 @@ AC_DEFUN([MCA_opal_event_libevent2022_CONFIG],[
     AS_IF([test "$libevent_happy" = "yes" && test -r $libevent_file],
           [OPAL_HAVE_WORKING_EVENTOPS=`grep HAVE_WORKING_EVENTOPS $libevent_fil
            $1],
-          [$2
-           OPAL_HAVE_WORKING_EVENTOPS=0])
+          [$2])

     OPAL_VAR_SCOPE_POP
 ])

[artpol84]

@jsquyres the reference to PR is correct. The hangs that we are observing was introduced along with it's merge.

Originally the code configuring internal event component was executed only if internal component was chosen (with function MCA_opal_event_libevent2022_DO_THE_CONFIG):
https://github.com/open-mpi/ompi/pull/1687/files#diff-d8b0ce0375e7a6130c38ddb1fa5df90fL102

But now it is executed always and skews the configuration.

[artpol84]

Regarding to the simplicity of the fix I guess that both of them are comparable but indentation in @karasevb version makes patch looks bigger.

Also in the proposed solution we making sure that we are setting OPAL_HAVE_WORKING_EVENTOPS ether to 0 or to 1.
I guess that undefined value may be ok as well, but we decided to keep the original design.

[jsquyres]

@artpol84 Ah, I see -- you're saying it fixes a problem introduced by PR #1687. From the wording, I thought it was trying to do the "auto-close a github issue when this is merged" kind of thing.

As for my suggestion: you're right. The original PR/patch keeps the always-set-to-0-or-1 behavior, which is Better than my suggestion. 👍

[artpol84]

@jsquyres do we target 2.0.1?

[jsquyres]

Yes, I think this is a good candidate for v2.0.1.