You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Till Toenshoff via Review Board <no...@reviews.apache.org> on 2018/09/06 00:32:25 UTC

Review Request 68640: Added version check and bundling of libevent to autotools.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68640/
-----------------------------------------------------------

Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.

Bugs: MESOS-7076
https://issues.apache.org/jira/browse/MESOS-7076

Repository: mesos

Description
-------

Bundles libevent 2.0.22 to ensure functional SSL builds accross all
supported platforms. Namely macOS and Ubuntu have shown problems when
using more recent libevent versions. The underlying problem has been
under investigation for longer period of time - so far without a
solution. The bundled libevent includes a patch to make it libssl
version > 1.0.x compatible. That patch has been extracted from the
Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
builds against both, libssl 1.0.x as well as libssl 1.1.x.
For unbundled builds a version detection of known problematic
distributions vs. provided libevent is included.

Diffs
-----

3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757
3rdparty/libevent-2.0.22-stable.patch PRE-CREATION
3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION
3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5
configure.ac 9de51e59796e2a64138acb82cbbb080f1ae6cc6e
src/python/native_common/ext_modules.py.in 7efbb8ec91b74c32ac629f1f2e7de82986d518db

Diff: https://reviews.apache.org/r/68640/diff/1/

Testing
-------

Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. Additional testing is ongoing. CMake updates will follow.

Thanks,

Till Toenshoff

Re: Review Request 68640: Added version check and bundling of libevent to autotools.

Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.


> On Sept. 11, 2018, 11:25 p.m., James Peach wrote:
> > 3rdparty/Makefile.am
> > Lines 327 (patched)
> > <https://reviews.apache.org/r/68640/diff/2/?file=2086162#file2086162line327>
> >
> >     I think that `JEMALLOC_CONFIGURE_ARGS` is a typo?
> >     
> >     It reads more clearly to me if you just put the extra options inline:
> >     ```
> >     ./configure $(CONFIGURE_ARGS) \
> >       --disable-dependency-tracking \
> >       --disable-debug-mode \
> >       --disable-samples \
> >       ...
> >     ```

Indeed a copy&paste error :(

And yes, I like the explicit variant a lot better - thanks!


- Till


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68640/#review208531
-----------------------------------------------------------


On Sept. 16, 2018, 8:40 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68640/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2018, 8:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.
> 
> 
> Bugs: MESOS-7076
>     https://issues.apache.org/jira/browse/MESOS-7076
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Bundles libevent 2.0.22 to ensure functional SSL builds accross all
> supported platforms. Namely macOS and Ubuntu have shown problems when
> using more recent libevent versions. The underlying problem has been
> under investigation for a longer period of time - so far without a
> solution. The bundled libevent includes a patch to make it libssl
> version > 1.0.x compatible. That patch has been extracted from the
> Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
> builds against both, libssl 1.0.x as well as libssl 1.1.x.
> For unbundled builds a version detection of known problematic
> distributions vs. provided libevent is included.
> 
> 
> Diffs
> -----
> 
>   3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757 
>   3rdparty/libevent-2.0.22-stable.patch PRE-CREATION 
>   3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION 
>   3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
>   configure.ac 472e1102bf049e62beaacc9ccb231b6cf11c5fe5 
>   m4/libevent.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68640/diff/3/
> 
> 
> Testing
> -------
> 
> Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. CMake updates will follow soon.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 68640: Added version check and bundling of libevent.

Posted by James Peach <jp...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68640/#review208531
-----------------------------------------------------------




3rdparty/Makefile.am
Lines 327 (patched)
<https://reviews.apache.org/r/68640/#comment292528>

    I think that `JEMALLOC_CONFIGURE_ARGS` is a typo?
    
    It reads more clearly to me if you just put the extra options inline:
    ```
    ./configure $(CONFIGURE_ARGS) \
      --disable-dependency-tracking \
      --disable-debug-mode \
      --disable-samples \
      ...
    ```



configure.ac
Lines 1616 (patched)
<https://reviews.apache.org/r/68640/#comment292527>

    At this point, you know the latest version that is acceptable. I think that you could do a `AC_RUN_IFELSE` to compare that version using `event_get_version_number`. That would remove the need to manually run the compiler.



configure.ac
Lines 1635 (patched)
<https://reviews.apache.org/r/68640/#comment292523>

    Can you make this whole version check logic a separate autoconf macro? I think that would make configure.ac easier to follow and also make it easier to duplicate in libprocess.
    
    ```
    MESOS_CHECK_LIBEVENT_IS_USABLE(
      action-if-usable,
      action-if-not-usable
    )
    ```


- James Peach


On Sept. 6, 2018, 10:07 a.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68640/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2018, 10:07 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.
> 
> 
> Bugs: MESOS-7076
>     https://issues.apache.org/jira/browse/MESOS-7076
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Bundles libevent 2.0.22 to ensure functional SSL builds accross all
> supported platforms. Namely macOS and Ubuntu have shown problems when
> using more recent libevent versions. The underlying problem has been
> under investigation for a longer period of time - so far without a
> solution. The bundled libevent includes a patch to make it libssl
> version > 1.0.x compatible. That patch has been extracted from the
> Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
> builds against both, libssl 1.0.x as well as libssl 1.1.x.
> For unbundled builds a version detection of known problematic
> distributions vs. provided libevent is included.
> 
> 
> Diffs
> -----
> 
>   3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757 
>   3rdparty/libevent-2.0.22-stable.patch PRE-CREATION 
>   3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION 
>   3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
>   configure.ac 9de51e59796e2a64138acb82cbbb080f1ae6cc6e 
>   src/python/native_common/ext_modules.py.in 7efbb8ec91b74c32ac629f1f2e7de82986d518db 
> 
> 
> Diff: https://reviews.apache.org/r/68640/diff/2/
> 
> 
> Testing
> -------
> 
> Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. Additional testing is ongoing. CMake updates will follow.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 68640: Added version check and bundling of libevent to autotools.

Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.


> On Sept. 21, 2018, 3:57 p.m., James Peach wrote:
> > configure.ac
> > Line 1491 (original), 1491 (patched)
> > <https://reviews.apache.org/r/68640/diff/3/?file=2089807#file2089807line1491>
> >
> >     Minor nit, this could be:
> >     
> >     ```
> >     test "x$without_bundled_libevent" = "xyes" -o "x$enable_bundled" != "xyes"
> >     ```

We generally do not use the `test` combinatoric options so far.


> On Sept. 21, 2018, 3:57 p.m., James Peach wrote:
> > m4/libevent.m4
> > Lines 42 (patched)
> > <https://reviews.apache.org/r/68640/diff/3/?file=2089808#file2089808line42>
> >
> >     Since these checks always error out, I think it would be easier to read if you just make them sequentially rather than nesting them.
> >     
> >     For example:
> >     ```
> >     AC_CHECK_HEADERS([event2/event.h], ...)
> >     AC_CHECK_LIB([event], [event_base_new], ...)
> >     
> >     etc.
> >     ```
> >     
> >     Since this is just extracted from the previous chec, just a suggestion. Refactor if you like :)

Yeah, much better read then - thanks for pointing it out!


> On Sept. 21, 2018, 3:57 p.m., James Peach wrote:
> > m4/libevent.m4
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/68640/diff/3/?file=2089808#file2089808line94>
> >
> >     Use `AS_IF` consistently here?

Thanks!


- Till


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68640/#review208863
-----------------------------------------------------------


On Sept. 22, 2018, 7:53 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68640/
> -----------------------------------------------------------
> 
> (Updated Sept. 22, 2018, 7:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.
> 
> 
> Bugs: MESOS-7076
>     https://issues.apache.org/jira/browse/MESOS-7076
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Bundles libevent 2.0.22 to ensure functional SSL builds accross all
> supported platforms. Namely macOS and Ubuntu have shown problems when
> using more recent libevent versions. The underlying problem has been
> under investigation for a longer period of time - so far without a
> solution. The bundled libevent includes a patch to make it libssl
> version > 1.0.x compatible. That patch has been extracted from the
> Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
> builds against both, libssl 1.0.x as well as libssl 1.1.x.
> For unbundled builds a version detection of known problematic
> distributions vs. provided libevent is included.
> 
> 
> Diffs
> -----
> 
>   3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757 
>   3rdparty/libevent-2.0.22-stable.patch PRE-CREATION 
>   3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION 
>   3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
>   configure.ac 472e1102bf049e62beaacc9ccb231b6cf11c5fe5 
>   m4/libevent.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68640/diff/4/
> 
> 
> Testing
> -------
> 
> Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. CMake updates will follow soon.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 68640: Added version check and bundling of libevent to autotools.

Posted by James Peach <jp...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68640/#review208863
-----------------------------------------------------------


Fix it, then Ship it!





configure.ac
Line 1491 (original), 1491 (patched)
<https://reviews.apache.org/r/68640/#comment293102>

    Minor nit, this could be:
    
    ```
    test "x$without_bundled_libevent" = "xyes" -o "x$enable_bundled" != "xyes"
    ```



m4/libevent.m4
Lines 42 (patched)
<https://reviews.apache.org/r/68640/#comment293100>

    Since these checks always error out, I think it would be easier to read if you just make them sequentially rather than nesting them.
    
    For example:
    ```
    AC_CHECK_HEADERS([event2/event.h], ...)
    AC_CHECK_LIB([event], [event_base_new], ...)
    
    etc.
    ```
    
    Since this is just extracted from the previous chec, just a suggestion. Refactor if you like :)



m4/libevent.m4
Lines 94 (patched)
<https://reviews.apache.org/r/68640/#comment293101>

    Use `AS_IF` consistently here?


- James Peach


On Sept. 16, 2018, 8:40 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68640/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2018, 8:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.
> 
> 
> Bugs: MESOS-7076
>     https://issues.apache.org/jira/browse/MESOS-7076
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Bundles libevent 2.0.22 to ensure functional SSL builds accross all
> supported platforms. Namely macOS and Ubuntu have shown problems when
> using more recent libevent versions. The underlying problem has been
> under investigation for a longer period of time - so far without a
> solution. The bundled libevent includes a patch to make it libssl
> version > 1.0.x compatible. That patch has been extracted from the
> Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
> builds against both, libssl 1.0.x as well as libssl 1.1.x.
> For unbundled builds a version detection of known problematic
> distributions vs. provided libevent is included.
> 
> 
> Diffs
> -----
> 
>   3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757 
>   3rdparty/libevent-2.0.22-stable.patch PRE-CREATION 
>   3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION 
>   3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
>   configure.ac 472e1102bf049e62beaacc9ccb231b6cf11c5fe5 
>   m4/libevent.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68640/diff/3/
> 
> 
> Testing
> -------
> 
> Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. CMake updates will follow soon.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 68640: Added version check and bundling of libevent to autotools.

Posted by James Peach <jp...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68640/#review208930
-----------------------------------------------------------


Ship it!




Ship It!

- James Peach


On Sept. 22, 2018, 7:53 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68640/
> -----------------------------------------------------------
> 
> (Updated Sept. 22, 2018, 7:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.
> 
> 
> Bugs: MESOS-7076
>     https://issues.apache.org/jira/browse/MESOS-7076
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Bundles libevent 2.0.22 to ensure functional SSL builds accross all
> supported platforms. Namely macOS and Ubuntu have shown problems when
> using more recent libevent versions. The underlying problem has been
> under investigation for a longer period of time - so far without a
> solution. The bundled libevent includes a patch to make it libssl
> version > 1.0.x compatible. That patch has been extracted from the
> Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
> builds against both, libssl 1.0.x as well as libssl 1.1.x.
> For unbundled builds a version detection of known problematic
> distributions vs. provided libevent is included.
> 
> 
> Diffs
> -----
> 
>   3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757 
>   3rdparty/libevent-2.0.22-stable.patch PRE-CREATION 
>   3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION 
>   3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
>   configure.ac 472e1102bf049e62beaacc9ccb231b6cf11c5fe5 
>   m4/libevent.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68640/diff/4/
> 
> 
> Testing
> -------
> 
> Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. CMake updates will follow soon.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 68640: Added version check and bundling of libevent to autotools.

Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68640/
-----------------------------------------------------------

(Updated Sept. 22, 2018, 7:53 p.m.)


Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.


Changes
-------

Addressed comments. Thanks James!


Bugs: MESOS-7076
    https://issues.apache.org/jira/browse/MESOS-7076


Repository: mesos


Description
-------

Bundles libevent 2.0.22 to ensure functional SSL builds accross all
supported platforms. Namely macOS and Ubuntu have shown problems when
using more recent libevent versions. The underlying problem has been
under investigation for a longer period of time - so far without a
solution. The bundled libevent includes a patch to make it libssl
version > 1.0.x compatible. That patch has been extracted from the
Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
builds against both, libssl 1.0.x as well as libssl 1.1.x.
For unbundled builds a version detection of known problematic
distributions vs. provided libevent is included.


Diffs (updated)
-----

  3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757 
  3rdparty/libevent-2.0.22-stable.patch PRE-CREATION 
  3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION 
  3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
  configure.ac 472e1102bf049e62beaacc9ccb231b6cf11c5fe5 
  m4/libevent.m4 PRE-CREATION 


Diff: https://reviews.apache.org/r/68640/diff/4/

Changes: https://reviews.apache.org/r/68640/diff/3-4/


Testing
-------

Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. CMake updates will follow soon.


Thanks,

Till Toenshoff

Re: Review Request 68640: Added version check and bundling of libevent to autotools.

Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68640/
-----------------------------------------------------------

(Updated Sept. 16, 2018, 8:40 p.m.)


Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.


Changes
-------

Addressed comments - thanks for the reviews James!


Summary (updated)
-----------------

Added version check and bundling of libevent to autotools.


Bugs: MESOS-7076
    https://issues.apache.org/jira/browse/MESOS-7076


Repository: mesos


Description
-------

Bundles libevent 2.0.22 to ensure functional SSL builds accross all
supported platforms. Namely macOS and Ubuntu have shown problems when
using more recent libevent versions. The underlying problem has been
under investigation for a longer period of time - so far without a
solution. The bundled libevent includes a patch to make it libssl
version > 1.0.x compatible. That patch has been extracted from the
Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
builds against both, libssl 1.0.x as well as libssl 1.1.x.
For unbundled builds a version detection of known problematic
distributions vs. provided libevent is included.


Diffs (updated)
-----

  3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757 
  3rdparty/libevent-2.0.22-stable.patch PRE-CREATION 
  3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION 
  3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
  configure.ac 472e1102bf049e62beaacc9ccb231b6cf11c5fe5 
  m4/libevent.m4 PRE-CREATION 


Diff: https://reviews.apache.org/r/68640/diff/3/

Changes: https://reviews.apache.org/r/68640/diff/2-3/


Testing (updated)
-------

Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. CMake updates will follow soon.


Thanks,

Till Toenshoff

Re: Review Request 68640: Added version check and bundling of libevent.

Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.

(Updated Sept. 6, 2018, 10:07 a.m.)

Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.

Changes
-------

Fixed missing binaries.

Summary (updated)
-----------------

Added version check and bundling of libevent.

Bugs: MESOS-7076
https://issues.apache.org/jira/browse/MESOS-7076

Repository: mesos

Description (updated)
-------

Bundles libevent 2.0.22 to ensure functional SSL builds accross all
supported platforms. Namely macOS and Ubuntu have shown problems when
using more recent libevent versions. The underlying problem has been
under investigation for a longer period of time - so far without a
solution. The bundled libevent includes a patch to make it libssl
version > 1.0.x compatible. That patch has been extracted from the
Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
builds against both, libssl 1.0.x as well as libssl 1.1.x.
For unbundled builds a version detection of known problematic
distributions vs. provided libevent is included.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/68640/diff/2/

Changes: https://reviews.apache.org/r/68640/diff/1-2/

Testing
-------

Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. Additional testing is ongoing. CMake updates will follow.

Thanks,

Till Toenshoff