----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67064/#review203354 -----------------------------------------------------------
3rdparty/CMakeLists.txt Lines 760 (patched) <https://reviews.apache.org/r/67064/#comment285488> Nit: one too few 3rdparty/CMakeLists.txt Lines 770-776 (patched) <https://reviews.apache.org/r/67064/#comment285485> Awesome; but we'll need a conditional for non-Visual Studio generators too (like Ninja). Should just be: ``` if (CMAKE_GENERATOR MATCHES "Visual Studio") set_target_properties( bzip2 PROPERTIES IMPORTED_LOCATION_DEBUG ${BZIP2_ROOT}-build/Debug/bzip2${BZIP2_SHARED}${LIBRARY_SUFFIX} IMPORTED_LOCATION_RELEASE ${BZIP2_ROOT}-build/Release/bzip2${BZIP2_SHARED}${LIBRARY_SUFFIX} IMPORTED_IMPLIB_DEBUG ${BZIP2_ROOT}-build/Debug/bzip2${CMAKE_IMPORT_LIBRARY_SUFFIX} IMPORTED_IMPLIB_RELEASE ${BZIP2_ROOT}-build/Release/bzip2${CMAKE_IMPORT_LIBRARY_SUFFIX}) else () set_target_properties( bzip2 PROPERTIES IMPORTED_LOCATION ${BZIP2_ROOT}-build/bzip2${BZIP2_SHARED}${LIBRARY_SUFFIX}) endif () 3rdparty/CMakeLists.txt Lines 782-784 (patched) <https://reviews.apache.org/r/67064/#comment285486> Should we forward either of the C or CXX args too? 3rdparty/CMakeLists.txt Lines 799 (patched) <https://reviews.apache.org/r/67064/#comment285487> Nit: one too many 3rdparty/CMakeLists.txt Lines 809-814 (patched) <https://reviews.apache.org/r/67064/#comment285489> Ditto on the `else()` for Ninja and other single-configuration generators on Windows. 3rdparty/CMakeLists.txt Lines 821-823 (patched) <https://reviews.apache.org/r/67064/#comment285490> Same question regarding C or CXX flags. 3rdparty/CMakeLists.txt Lines 910 (patched) <https://reviews.apache.org/r/67064/#comment285491> Nit: a lot too few 3rdparty/CMakeLists.txt Lines 912-964 (patched) <https://reviews.apache.org/r/67064/#comment285492> Nit: indentation is missing. 3rdparty/CMakeLists.txt Lines 917-918 (patched) <https://reviews.apache.org/r/67064/#comment285493> Should we forward either C or CXX flags too? 3rdparty/CMakeLists.txt Lines 932 (patched) <https://reviews.apache.org/r/67064/#comment285494> ? 3rdparty/CMakeLists.txt Lines 933 (patched) <https://reviews.apache.org/r/67064/#comment285496> Just to be clear: libarchive is fine with `UNICODE` etc. defined, just not their test suite? 3rdparty/CMakeLists.txt Lines 935-936 (patched) <https://reviews.apache.org/r/67064/#comment285495> But it was already disabled above. 3rdparty/CMakeLists.txt Lines 939 (patched) <https://reviews.apache.org/r/67064/#comment285497> The plus to using install logic, the location doesn't change ;) 3rdparty/CMakeLists.txt Lines 967 (patched) <https://reviews.apache.org/r/67064/#comment285498> I don't think this is necesary given we used `find_package`. According to its [docs](https://cmake.org/cmake/help/latest/module/FindLibArchive.html) it sets: ``` LibArchive_FOUND - true if libarchive was found LibArchive_INCLUDE_DIRS - include search path LibArchive_LIBRARIES - libraries to link LibArchive_VERSION - libarchive 3-component version number ``` So I think we just want to add an `IMPORTED` target for the `LibArchive_LIBRARIES` and `LibArchive_INCLUDE_DIRS`. In fact, without those, I don't think this would work as-is when trying to build with system libarchive. 3rdparty/CMakeLists.txt Line 1256 (original) <https://reviews.apache.org/r/67064/#comment285499> ? 3rdparty/libarchive-3.3.2.patch Lines 17-18 (patched) <https://reviews.apache.org/r/67064/#comment285500> What warnings do we have in their build that they weren't expecting? configure.ac Lines 1461-1495 (patched) <https://reviews.apache.org/r/67064/#comment285484> I can't review this part; I don't know the Autotools builds. Jie, or Joseph, can one of you? - Andrew Schwartzmeyer On May 15, 2018, 10:09 a.m., John Kordich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67064/ > ----------------------------------------------------------- > > (Updated May 15, 2018, 10:09 a.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 0075c133ef60bfef06f4123c14cae8a5672440f5 > 3rdparty/Makefile.am 8d9fa85dd65a94d91670d54dab36deea80d14996 > 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 6e91ecf4659ebec2e124ca4b2218c830608abeb0 > src/Makefile.am 7e91681e3b8b217f8b23fa5347e059640c62fc65 > > > Diff: https://reviews.apache.org/r/67064/diff/4/ > > > 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 > >
