You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by John Kordich via Review Board <no...@reviews.apache.org> on 2018/05/14 22:42:20 UTC
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67064/
-----------------------------------------------------------
(Updated May 14, 2018, 10:42 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
Changes
-------
Moved tarballs to a patch that this depends on, 67118
Summary (updated)
-----------------
Added libarchive, bzip2, and xz patches and associated build changes.
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 (updated)
-----
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/3/
Changes: https://reviews.apache.org/r/67064/diff/2-3/
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
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67064/#review203079
-----------------------------------------------------------
Oh, of course I see why there's an error here. I'm looking for bzip2/xz in 3rdparty deps. I'll submit a fix for that. Might as well get the archives in 3rdparty deps anyway: https://github.com/mesos/3rdparty/pull/17
- John Kordich
On May 14, 2018, 10:42 p.m., John Kordich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67064/
> -----------------------------------------------------------
>
> (Updated May 14, 2018, 10:42 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 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/3/
>
>
> 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
>
>
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On June 4, 2018, 1:50 p.m., Joseph Wu wrote:
> > src/Makefile.am
> > Lines 218 (patched)
> > <https://reviews.apache.org/r/67064/diff/7/?file=2034659#file2034659line218>
> >
> > Won't need this line when linking against the bundled libarchive since we'll be adding the library to libmesos further down the makefile.
Ah, nevermind. This becomes necessary as the build rules for libmesos and mesos-fetcher are slightly separate in the automake build.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67064/#review204069
-----------------------------------------------------------
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
>
>
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67064/
-----------------------------------------------------------
(Updated June 4, 2018, 7: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.
Changes
-------
Addressed Andy's comments
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/67064/diff/7-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
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67064/
-----------------------------------------------------------
(Updated June 1, 2018, 9:58 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
Changes
-------
Addressed Joseph Wu's build issues:
Added ENABLE_LZ4 option to the CMake build of libarchive, so that we can disable it.
Disabled all flags to libarchive that are unnecessary as part of the autotools build.
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 (updated)
-----
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/7/
Changes: https://reviews.apache.org/r/67064/diff/6-7/
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
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67064/
-----------------------------------------------------------
(Updated May 25, 2018, 2: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.
Changes
-------
Updated name and hash for the modified xz archive.
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 (updated)
-----
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 c08ac6e2f5deec4d05f59f71ff6c51382f216708
Diff: https://reviews.apache.org/r/67064/diff/6/
Changes: https://reviews.apache.org/r/67064/diff/5-6/
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
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67064/
-----------------------------------------------------------
(Updated May 23, 2018, 11:03 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
Changes
-------
Updated patch with requested changes.
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 (updated)
-----
3rdparty/CMakeLists.txt 3837a31562d48962ccf5aa72fa147fd109e58ca0
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 6d9c8ceb083984b9b61b30c29c39b3aef32fbd69
src/Makefile.am c08ac6e2f5deec4d05f59f71ff6c51382f216708
Diff: https://reviews.apache.org/r/67064/diff/5/
Changes: https://reviews.apache.org/r/67064/diff/4-5/
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
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67064/#review203511
-----------------------------------------------------------
3rdparty/CMakeLists.txt
Lines 758-759 (patched)
<https://reviews.apache.org/r/67064/#comment285802>
bzip doesn't seem to have a subtitle for itself (the one here is `zlib`s). So you could just call it a `high-quality data compressor`.
Also the link should not have an `https`:
http://www.bzip.org/
3rdparty/CMakeLists.txt
Lines 797 (patched)
<https://reviews.apache.org/r/67064/#comment285808>
Ditto about the subtitle.
3rdparty/CMakeLists.txt
Lines 921 (patched)
<https://reviews.apache.org/r/67064/#comment285809>
This option is unconditionally added a little below. So in the non-Windows case, you'd end up with an option list of: `-DENABLE_TEST=OFF ... -DENABLE_TEST=ON`
3rdparty/Makefile.am
Lines 280-286 (patched)
<https://reviews.apache.org/r/67064/#comment285812>
There seems to be mixed tabs/spaces here (and unfortunately, originally throughout this file D: ). You can set your tab settings to 8-spaces wide to see if the trailing backslaces line up.
Also, why are `CPPFLAGS` and `LDFLAGS` emptied? And should we be passing `$(CONFIGURE_ARGS)` to the configure script here?
3rdparty/bzip2-1.0.6.patch
Lines 7-20 (patched)
<https://reviews.apache.org/r/67064/#comment285813>
This seems very inconsistent style-wise...
Also, can you add a note saying that `bzip2.c` and `bzip2recover.c` are not built because we are only interested in building the library (not executables).
3rdparty/bzip2-1.0.6.patch
Lines 12-14 (patched)
<https://reviews.apache.org/r/67064/#comment285815>
You can probably move the module definitions file `libbz2.def` into the list of sources.
src/Makefile.am
Lines 221-226 (patched)
<https://reviews.apache.org/r/67064/#comment285817>
This looks like a mistake. Looks like you had temporarily commented parts of this out?
- Joseph Wu
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
>
>
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 67064: Added libarchive, bzip2,
and xz patches and associated build changes.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67064/
-----------------------------------------------------------
(Updated May 15, 2018, 5:09 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
Changes
-------
Fixed location new tarballs are located in.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/67064/diff/3-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