Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

2018-06-04 Thread Joseph Wu 

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


Ship it!




A bunch of minor things that I'll fix before committing.


3rdparty/stout/include/stout/archiver.hpp
Lines 71-76 (patched)


Two newlines too many here.  I can fixup before committing.



3rdparty/stout/tests/CMakeLists.txt
Lines 106 (patched)


Extraneous newline which seems to have snuck into the review.  I'll remove 
before committing.



3rdparty/stout/tests/archiver_tests.cpp
Lines 42 (patched)


s/os::getcwd()/sandbox.get()/

Here and throughout the test.  The TemporaryDirectoryTest defines this 
variable so that we don't need to make that call repeatedly.



3rdparty/stout/tests/archiver_tests.cpp
Lines 52 (patched)


A few periods missing on comments like this throughout this file.



3rdparty/stout/tests/archiver_tests.cpp
Lines 94 (patched)


s/fo/to/



3rdparty/stout/tests/archiver_tests.cpp
Lines 411 (patched)


s/extrat/extract/



3rdparty/stout/tests/archiver_tests.cpp
Lines 457-458 (patched)


This test passes on non-Windows too, so it's better not to `#ifdef` it.


- 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/67065/
> ---
> 
> (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
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 86111a8709cab709ba1bf844043d3a1da07b6241 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-06-04 Thread John Kordich via Review Board 

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

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

This archiver utility can be invoked to extract a compressed or
uncompressed archive with several different supported formats.


Diffs (updated)
-

  3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
  3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
  3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
  3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 86111a8709cab709ba1bf844043d3a1da07b6241 
  3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/67065/diff/8/

Changes: https://reviews.apache.org/r/67065/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-06-04 Thread Andrew Schwartzmeyer 

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




3rdparty/stout/include/stout/archiver.hpp
Lines 82 (patched)


Super tiny style nit (but this will fail mesos-tidy): `fd->crt()`.



3rdparty/stout/include/stout/archiver.hpp
Lines 84 (patched)


s/crt/CRT

But also this needs a comment similar to:

NOTE: On Windows, we need to explicitly allocate a CRT file descriptor 
because libarchive requires it. Once the CRT `fd` is allocated, it must be 
closed with `_close` instead of `os::close`.



3rdparty/stout/include/stout/archiver.hpp
Lines 94-98 (patched)


This `Closer` could be shared, which the ifdef just in the dtor.



3rdparty/stout/include/stout/archiver.hpp
Lines 101 (patched)


Magic number?



3rdparty/stout/include/stout/archiver.hpp
Lines 165-167 (patched)


Error handling looks way better than before :)


- Andrew Schwartzmeyer


On June 1, 2018, 3:40 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated June 1, 2018, 3:40 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
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 86111a8709cab709ba1bf844043d3a1da07b6241 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-06-04 Thread Andrew Schwartzmeyer 

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




3rdparty/stout/include/stout/archiver.hpp
Lines 20-22 (patched)


I still don't think this is necessary here.

Joe had said in an issue:

> This definition should go into the CMake file(s):
> # This is a definition that only has an effect on Windows and is necessary
> # when building a static libarchive instead of a DLL.
> `target_compile_definitions(libarchive PUBLIC LIBARCHIVE_STATIC)`

And then you said in a new iteration:

> Updated to include a comment about why we need a define. There were plans 
to set this define at a global level inside the cmake build using the 
target_compile_definitions cmake function, but this function is available for 
IMPORTED targets only in cmake versions > 3.11. We don't want to bump the 
required version of cmake for such a small change, so we kept the #define in 
this file.

But that's because for imported targets, you're "supposed" to set 
properties manually. So this should instead look like:

```
set_target_properties(
  archive PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS LIBARCHIVE_STATIC)
```


- Andrew Schwartzmeyer


On June 1, 2018, 3:40 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated June 1, 2018, 3:40 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
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 86111a8709cab709ba1bf844043d3a1da07b6241 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-06-01 Thread John Kordich via Review Board 

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

(Updated June 1, 2018, 10:40 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 to include a comment about why we need a define. There were plans to 
set this define at a global level inside the cmake build using the 
target_compile_definitions cmake function, but this function is available for 
IMPORTED targets only in cmake versions > 3.11. We don't want to bump the 
required version of cmake for such a small change, so we kept the #define in 
this file.


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


Repository: mesos


Description
---

This archiver utility can be invoked to extract a compressed or
uncompressed archive with several different supported formats.


Diffs (updated)
-

  3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
  3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
  3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
  3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 86111a8709cab709ba1bf844043d3a1da07b6241 
  3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/67065/diff/7/

Changes: https://reviews.apache.org/r/67065/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-06-01 Thread Eric Mumau via Review Board 

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


Ship it!




Ship It!

- Eric Mumau


On June 1, 2018, 9:58 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (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.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 86111a8709cab709ba1bf844043d3a1da07b6241 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-06-01 Thread John Kordich via Review Board 

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

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

Added Joseph Wu's refactoring of archiver.hpp changes, with one of my own 
modifications.


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


Repository: mesos


Description
---

This archiver utility can be invoked to extract a compressed or
uncompressed archive with several different supported formats.


Diffs (updated)
-

  3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
  3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
  3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
  3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 86111a8709cab709ba1bf844043d3a1da07b6241 
  3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/67065/diff/6/

Changes: https://reviews.apache.org/r/67065/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-05-31 Thread Joseph Wu 

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




3rdparty/stout/include/stout/archiver.hpp
Lines 20 (patched)


This definition should go into the CMake file(s):
```
# This is a definition that only has an effect on Windows and is necessary
# when building a static libarchive instead of a DLL.
target_compile_definitions(libarchive PUBLIC LIBARCHIVE_STATIC)
```



3rdparty/stout/include/stout/archiver.hpp
Lines 36-58 (patched)


We might not need to have a helper for error handling.  If you treat all 
`result <= ARCHIVE_WARN` as an error, you can get the roughly same behavior 
(minus the WARNING logs).



3rdparty/stout/include/stout/archiver.hpp
Lines 124-127 (patched)


This error check could be made more explicit:
```
if (result != ARCHIVE_OK) { ... }
```


- Joseph Wu


On May 30, 2018, 1:29 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated May 30, 2018, 1:29 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
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 86111a8709cab709ba1bf844043d3a1da07b6241 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-05-30 Thread John Kordich via Review Board 

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

(Updated May 30, 2018, 8:29 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
---

Added archiver header file to the appropriate automake file.


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


Repository: mesos


Description
---

This archiver utility can be invoked to extract a compressed or
uncompressed archive with several different supported formats.


Diffs (updated)
-

  3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
  3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
  3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
  3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 86111a8709cab709ba1bf844043d3a1da07b6241 
  3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/67065/diff/5/

Changes: https://reviews.apache.org/r/67065/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-05-23 Thread John Kordich via Review Board 

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

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

This archiver utility can be invoked to extract a compressed or
uncompressed archive with several different supported formats.


Diffs (updated)
-

  3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
  3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
  3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 43c5e873a3a3b8f79f0f888a450e186c6123d938 
  3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 


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

Changes: https://reviews.apache.org/r/67065/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

Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

2018-05-21 Thread Joseph Wu 


> On May 17, 2018, 11:59 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/archiver.hpp
> > Lines 93 (patched)
> > 
> >
> > Does libarchive really require a CRT file descriptor, it's not happy 
> > with a `HANDLE`?

Looks like libarchive uses Posix functions on the given FD, so it probably 
expects a CRT FD.
https://github.com/libarchive/libarchive/blob/v3.3.2/libarchive/archive_read_open_fd.c#L73


- Joseph


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


On May 14, 2018, 3:42 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated May 14, 2018, 3: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
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 28674c9873b7bbccb6b990ec16b7e40a5bf4f9ec 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-05-21 Thread Joseph Wu 


> On May 17, 2018, 11:59 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/archiver.hpp
> > Lines 68-70 (patched)
> > 
> >
> > Wait, this is C++, can't we `s/struct archive/archive`? (Thinking aloud 
> > here; should we typedef it locally so the typename is Capitalized?)

Some bits of style or naming inconsistency is unavoidable when interfacing with 
thirdparty headers.  We shouldn't over-engineer this file to match our style.


- Joseph


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


On May 14, 2018, 3:42 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated May 14, 2018, 3: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
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 28674c9873b7bbccb6b990ec16b7e40a5bf4f9ec 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-05-17 Thread Andrew Schwartzmeyer 

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




3rdparty/stout/CMakeLists.txt
Line 35 (original), 36 (patched)


We shouldn't need to do this if we set the `INTERFACE_LINK_LIBRARIES` 
property of the `libarchive` target correctly.



3rdparty/stout/include/stout/archiver.hpp
Lines 27-32 (patched)


If you run `clang-format` on the file, I think it'll sort these out 
correctly.



3rdparty/stout/include/stout/archiver.hpp
Lines 36-40 (patched)


I think these were meant to be above `extract()`.



3rdparty/stout/include/stout/archiver.hpp
Lines 42-43 (patched)


Style: in stout we use `snake_case` instead of `camelCase`.



3rdparty/stout/include/stout/archiver.hpp
Lines 45-56 (patched)


I can't make sense of this function. What is the meaning of its return 
value and out parameters?



3rdparty/stout/include/stout/archiver.hpp
Lines 59-62 (patched)


This needs to be documented. Is `destination` the folder in to which the 
archive will be extracted?



3rdparty/stout/include/stout/archiver.hpp
Lines 68 (patched)


If you wanted, you could replace `std::function` 
with `declspec(&archive_read_close)`.



3rdparty/stout/include/stout/archiver.hpp
Lines 68-70 (patched)


Wait, this is C++, can't we `s/struct archive/archive`? (Thinking aloud 
here; should we typedef it locally so the typename is Capitalized?)



3rdparty/stout/include/stout/archiver.hpp
Lines 84 (patched)


s/insure/ensure



3rdparty/stout/include/stout/archiver.hpp
Lines 86 (patched)


Third argument can be ommitted since `0` is the default.



3rdparty/stout/include/stout/archiver.hpp
Lines 89 (patched)


Delete most of that and just `return Error(fd.error())`.



3rdparty/stout/include/stout/archiver.hpp
Lines 93 (patched)


Does libarchive really require a CRT file descriptor, it's not happy with a 
`HANDLE`?



3rdparty/stout/include/stout/archiver.hpp
Lines 103 (patched)


Well we just leaked a handle (or worse) because a CRT fd was allocated 
above, and not closed. I think this is undefined behavior on Windows (closing a 
`HANDLE` with `CloseHandle` when a CRT fd has been allocated; the docs just say 
don't do it).



3rdparty/stout/include/stout/archiver.hpp
Lines 115-118 (patched)


I think the interface we're looking for is more like:

```
// Returns `Nothing()` if ARCHIVE_OK, otherwise returns an `Error`.
Try get_archive_error(int, archive);
...
Try error = get_archive_error(result, writer.get());
if (error.isError()) {
  // Don't have to close anything because the FDs should be wrapped in a 
deleter.
  return error;
}
```



3rdparty/stout/include/stout/archiver.hpp
Lines 126 (patched)


Are we _sure_ this handles long paths internally? We're not adding `\\?\` 
ourselves...



3rdparty/stout/include/stout/archiver.hpp
Lines 136 (patched)


Nit: Mesos style attaches `*` to the type, not the name. (Run clang-format!)



3rdparty/stout/include/stout/archiver.hpp
Lines 165 (patched)


What's the ordering between `ARCHIVE_OK` and `ARCHIVE_WARN`? I don't 
understand these `<` comparisons without knowning the enumeration of `result`.



3rdparty/stout/tests/CMakeLists.txt
Line 103 (original), 104 (patched)


This isn't necessary as the `target_link_libraries` command adds this 
dependency.



3rdparty/stout/tests/CMakeLists.txt
Lines 106 (patched)


I think actually you meant to add `libarchive` to the  stout `INTERFACE` 
library, which means all users of it (like `stout-tests` or `libmesos` etc.) 
would be pick it up transitively.


- Andrew Schwartzmeyer


On May 14, 2018, 3:42 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
>

Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

2018-05-14 Thread John Kordich via Review Board 

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

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

This archiver utility can be invoked to extract a compressed or
uncompressed archive with several different supported formats.


Diffs (updated)
-

  3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
  3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
  3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 28674c9873b7bbccb6b990ec16b7e40a5bf4f9ec 
  3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 


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

Changes: https://reviews.apache.org/r/67065/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 67065: Added a new stout utility header file which interfaces with libarchive.

2018-05-14 Thread John Kordich via Review Board 

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

(Updated May 14, 2018, 5:13 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 patch to include binary files


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


Repository: mesos


Description
---

This archiver utility can be invoked to extract a compressed or
uncompressed archive with several different supported formats.


Diffs (updated)
-

  3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
  3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
  3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 28674c9873b7bbccb6b990ec16b7e40a5bf4f9ec 
  3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 


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

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


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