mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 67064: Added libarchive, bzip2, and xz patches and associated build changes.
Date Mon, 04 Jun 2018 20:50:53 GMT

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


Fix it, then Ship it!




A couple of small things I'll fix before committing:


3rdparty/CMakeLists.txt
Lines 937-948 (patched)
<https://reviews.apache.org/r/67064/#comment286677>

    We should add `-DENABLE_LibGCC=OFF` just to be safe.
    
    The option could have an effect on Windows if the condition `NOT WITHOUT_PCRE_STATIC AND
NOT PCRE_STATIC AND PCRE_FOUND` is satsified.  AFAICT, none of these variables are usually
defined.



3rdparty/CMakeLists.txt
Lines 954 (patched)
<https://reviews.apache.org/r/67064/#comment286675>

    Hum... tests appear to be disabled on all platforms...
    
    I'll change this to be a note about the linkage against other compression libraries (bzip2,
xz, zlib).



3rdparty/Makefile.am
Line 234 (original), 237 (patched)
<https://reviews.apache.org/r/67064/#comment286674>

    Some extraneous spacing changes (mostly tabs to spaces) seem to have snuck into this patch.
 I'll remove them before committing.



3rdparty/Makefile.am
Lines 281-290 (patched)
<https://reviews.apache.org/r/67064/#comment286683>

    We'll need to disable building bzip2 and xz via:
    ```
    --without-bz2lib
    --without-lzma
    ```
    As these are not explicit dependencies.  The libarchive build attempts to find these on
the system, and if found, it will require symbols found in those libraries.  We'd then get
a link-time failure as Mesos does not link to `-lbz2` and `-lxz`.



3rdparty/libarchive-3.3.2.patch
Lines 32-39 (patched)
<https://reviews.apache.org/r/67064/#comment286684>

    The whitespace changes in the patch are extraneous and can be removed.  (We don't need
to prettify 3rdparty sources :)



configure.ac
Lines 1469 (patched)
<https://reviews.apache.org/r/67064/#comment286671>

    Missing a part of the conditional here:
    `|| test "x$enable_bundled" != "xyes"`
    
    I can add this before committing.



src/Makefile.am
Lines 218 (patched)
<https://reviews.apache.org/r/67064/#comment286672>

    Won't need this line when linking against the bundled libarchive since we'll be adding
the library to libmesos further down the makefile.



3rdparty/CMakeLists.txt
Lines 987 (patched)
<https://reviews.apache.org/r/67064/#comment286685>

    I almost didn't notice the odd capitalization on these:
    `SET_target_properties`
    vs
    `set_target_properties`
    
    I'll fix up the three instances of this before committing.


- Joseph Wu


On June 4, 2018, 12:11 p.m., John Kordich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67064/
> -----------------------------------------------------------
> 
> (Updated June 4, 2018, 12:11 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, Eric Mumau,
Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
>     https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> These libraries being available will allow stout to be changed such that
> stout can invoke libarchive instead of shelling out to tar to extract
> archives. On Windows, tar usually doesn't exist, and even if it does, it
> rarely supports most compression formats. On Windows, we will build
> libarchive to support each of those compression formats.
> 
> 
> Diffs
> -----
> 
>   3rdparty/CMakeLists.txt ecb6946401d9b81c6610cf9f33dcf2caa9ff0f04 
>   3rdparty/Makefile.am 062df18643cadb4f8f5bcbd56c1dad63a7b8f894 
>   3rdparty/bzip2-1.0.6.patch PRE-CREATION 
>   3rdparty/cmake/Versions.cmake 2828acd2aa00c8778c6c462bc594716b2b6289ba 
>   3rdparty/libarchive-3.3.2.patch PRE-CREATION 
>   3rdparty/versions.am ada998d62d012a45b2cf17256c1ae16b92d611f2 
>   3rdparty/xz-5.2.3.patch PRE-CREATION 
>   configure.ac 03d333d65bcab2e46cff0b1329e5ad7bb26da697 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
> 
> 
> Diff: https://reviews.apache.org/r/67064/diff/8/
> 
> 
> Testing
> -------
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and have run all
tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message