----------------------------------------------------------- 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 > >
