Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-12-08 Thread Andrew Schwartzmeyer 

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Dec. 7, 2017, 6:41 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Dec. 7, 2017, 6:41 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am b1318847df486f9f96c8b005998310bb56af91a9 
>   3rdparty/stout/include/Makefile.am 4ba91bbedecbe7914cf9f5c6a815b60ca02f0e01 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> bc2498d6fca1161c42172f5bd09a32785e0a2128 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/7/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-12-07 Thread Jeff Coffler 


> On Nov. 16, 2017, 5:50 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 45 (patched)
> > 
> >
> > The backslash at the end check seems a bit odd. Doesn't `isdir` return 
> > `true` if `/foo/bar/` is a directory? We also wouldn't be catching other 
> > cases like `/foo/bar/.` here, right?

isdir will return true, but it's a stat:: operation, so it will only return 
true if a directory existed.

The goal here was to try to limit Posix operations to be as follows:

1. Limits should not interfere with what Mesos was already doing, and
2. Limits should roughly put same restrictions in place as the Windows API code 
did. This probably wasn't perfect, but I felt it was a good stab, and generally 
good enough.

No, I think you're right, we wouldn't catch all cases (like /foo/bar/.), but I 
wasn't after every single case. Just to do basic limits. The crux of the 
problem here is that, on Posix, we use 'cp'. If you think about 'cp' behavior, 
it's pretty wierd from an API perspective (you can't determine what it will do 
without knowing state of things). I was trying to make it more deterministic, 
but I never intended to be "perfect" about it.


> On Nov. 16, 2017, 5:50 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 65 (patched)
> > 
> >
> > I believe this is equivalent to
> > ```
> > if (!WSUCCEEDED(status))
> > ```

Why, yes, it is. The problem is that WSUCCEEDED is defined in 
src/common/status_utils.hpp, and this is stout, so I can't include that here.


- Jeff


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


On Nov. 6, 2017, 6:08 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Nov. 6, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-12-07 Thread Jeff Coffler 

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

(Updated Dec. 8, 2017, 2:41 a.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li Li, 
and Michael Park.


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


Repository: mesos


Description
---

The os::copyfile() method provides an API to copy a file from one
location to another. Note that copying of directories are not
supported, and that the destination directory must exist before a
file can be copied into a directory.


Diffs (updated)
-

  3rdparty/stout/Makefile.am b1318847df486f9f96c8b005998310bb56af91a9 
  3rdparty/stout/include/Makefile.am 4ba91bbedecbe7914cf9f5c6a815b60ca02f0e01 
  3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt bc2498d6fca1161c42172f5bd09a32785e0a2128 
  3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 


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

Changes: https://reviews.apache.org/r/60621/diff/6-7/


Testing
---

See upstream

Note that Joe made some changes to this, I ended up taking his changes as is.


Thanks,

Jeff Coffler

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-11-16 Thread Michael Park 

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


Fix it, then Ship it!





3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 45 (patched)


The backslash at the end check seems a bit odd. Doesn't `isdir` return 
`true` if `/foo/bar/` is a directory? We also wouldn't be catching other cases 
like `/foo/bar/.` here, right?



3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 65 (patched)


I believe this is equivalent to
```
if (!WSUCCEEDED(status))
```



3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 46-47 (patched)


This message doesn't seem to have single-quotes around the paths whereas 
the POSIX version does.


- Michael Park


On Nov. 6, 2017, 10:08 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Nov. 6, 2017, 10:08 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li 
> Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-11-07 Thread Jeff Coffler 


> On Nov. 3, 2017, 6:29 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > 
> >
> > The general pattern is to just include the reason for an error, and to 
> > not include any information the caller already has, because otherwise the 
> > callers will double log:
> > 
> > ```
> > Try copy = copyfile(source, destination);
> > 
> > if (copy.isError()) {
> >   return ("Failed to copy '" + source + "'"
> >   " to '" + destination + "': " + copy.error();
> > }
> > ```
> > 
> > The current code would log:
> > 
> > "Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space 
> > left"
> > 
> > Can you guys do a sweep for this issue in the windows related code that 
> > has been added?
> 
> Andrew Schwartzmeyer wrote:
> I'm not sure I follow. Are you saying the _caller_ should always write 
> the actual error message, versus the _callee_ preparing it here?
> 
> I guess in your example, I don't get why the user of `os::copyfile` would 
> add `"Failed to copy..."` instead of just doing:
> 
> ```
> if (copy.isError()) {
> return Error(copy.error());
> }
> ```
> 
> And then the error message is only written once, in `os::copyfile`.
> 
> Benjamin Mahler wrote:
> What is the "actual" error message? :)
> 
> An error message consists of several parts, much like an exception: the 
> "reason" for the error, and multiple "stacks" of context. If you're referring 
> to the "reason" when you said "actual", either approach (the one we use, or 
> the one used in this patch) includes the reason in their returned error 
> message. The distinction lies in where the "stacks" of context get included.
> 
> The decision we took some time ago was to have the "owner" of the context 
> be responsible for including it. So if I call `os::copyfile` I know which 
> function I'm calling and which source and destination I'm passing into it. 
> This matches posix-style programming which I why I think we chose this 
> approach:
> 
> ```
> int main()
> {
>   int fd = open("/file");
>   
>   if (fd == -1) {
> // Caller logs the thing it was doing, and gets the reason for the 
> error:
> LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << 
> strerror(errno);
>   }
> }
> 
> vs
> 
> int main()
> {
>   Try fd = open("/file");
>   
>   if (fd.isError()) {
> // Caller logs the thing it was doing, and gets the reason for the 
> error:
> LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << 
> fd.error();
>   }
> }
> ```
> 
> Now of course, if you use the alternative approach to have the leaf 
> include all the information it has, then you have to compose differently:
> 
> ```
> int main()
> {
>   Try fd = os::open("/file");
>   
>   if (fd.isError()) {
> // Caller knows that no additional context needs to be added because 
> callee has all of it.
> LOG(ERROR) << "Failed to initialize: " << fd.error();
>   }
> }
> ```
> 
> The approach we chose was to treat the error as just the "reason" (like 
> strerror), so if you want to add context to it you can.
> 
> Both approaches work, but we have to pick one and apply it consistently 
> as best we can. In retrospect, I actually think the other approach would have 
> been a better choice because it fits more easily into Future::then style 
> composition. But we would have to discuss the change of approach and do a 
> sweep, because most of the code follows the first pattern shown.
> 
> Andrew Schwartzmeyer wrote:
> Ah, thank you for the detailed explanation. Unfortunately, the existing 
> error handling code was using the leaf approach, and we kept it consistent, 
> but consistently wrong. We'll have to do a sweep to fix it.
> 
> This explanation was really good, do we have something like it 
> documented? If not, let's get it added to the style guide.

This is a general issue all over the place. Rather than fix that here, I think 
this should be covered in a sweep of the code where we fix everything in one 
shot. So I'll drop this for now, but we will plan on doing a sweep to fix up a 
bunch of things like this.


- Jeff


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


On Nov. 6, 2017, 6:08 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-11-03 Thread Andrew Schwartzmeyer 


> On Nov. 3, 2017, 11:29 a.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > 
> >
> > The general pattern is to just include the reason for an error, and to 
> > not include any information the caller already has, because otherwise the 
> > callers will double log:
> > 
> > ```
> > Try copy = copyfile(source, destination);
> > 
> > if (copy.isError()) {
> >   return ("Failed to copy '" + source + "'"
> >   " to '" + destination + "': " + copy.error();
> > }
> > ```
> > 
> > The current code would log:
> > 
> > "Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space 
> > left"
> > 
> > Can you guys do a sweep for this issue in the windows related code that 
> > has been added?
> 
> Andrew Schwartzmeyer wrote:
> I'm not sure I follow. Are you saying the _caller_ should always write 
> the actual error message, versus the _callee_ preparing it here?
> 
> I guess in your example, I don't get why the user of `os::copyfile` would 
> add `"Failed to copy..."` instead of just doing:
> 
> ```
> if (copy.isError()) {
> return Error(copy.error());
> }
> ```
> 
> And then the error message is only written once, in `os::copyfile`.
> 
> Benjamin Mahler wrote:
> What is the "actual" error message? :)
> 
> An error message consists of several parts, much like an exception: the 
> "reason" for the error, and multiple "stacks" of context. If you're referring 
> to the "reason" when you said "actual", either approach (the one we use, or 
> the one used in this patch) includes the reason in their returned error 
> message. The distinction lies in where the "stacks" of context get included.
> 
> The decision we took some time ago was to have the "owner" of the context 
> be responsible for including it. So if I call `os::copyfile` I know which 
> function I'm calling and which source and destination I'm passing into it. 
> This matches posix-style programming which I why I think we chose this 
> approach:
> 
> ```
> int main()
> {
>   int fd = open("/file");
>   
>   if (fd == -1) {
> // Caller logs the thing it was doing, and gets the reason for the 
> error:
> LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << 
> strerror(errno);
>   }
> }
> 
> vs
> 
> int main()
> {
>   Try fd = open("/file");
>   
>   if (fd.isError()) {
> // Caller logs the thing it was doing, and gets the reason for the 
> error:
> LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << 
> fd.error();
>   }
> }
> ```
> 
> Now of course, if you use the alternative approach to have the leaf 
> include all the information it has, then you have to compose differently:
> 
> ```
> int main()
> {
>   Try fd = os::open("/file");
>   
>   if (fd.isError()) {
> // Caller knows that no additional context needs to be added because 
> callee has all of it.
> LOG(ERROR) << "Failed to initialize: " << fd.error();
>   }
> }
> ```
> 
> The approach we chose was to treat the error as just the "reason" (like 
> strerror), so if you want to add context to it you can.
> 
> Both approaches work, but we have to pick one and apply it consistently 
> as best we can. In retrospect, I actually think the other approach would have 
> been a better choice because it fits more easily into Future::then style 
> composition. But we would have to discuss the change of approach and do a 
> sweep, because most of the code follows the first pattern shown.

Ah, thank you for the detailed explanation. Unfortunately, the existing error 
handling code was using the leaf approach, and we kept it consistent, but 
consistently wrong. We'll have to do a sweep to fix it.

This explanation was really good, do we have something like it documented? If 
not, let's get it added to the style guide.


- Andrew


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


On Oct. 19, 2017, 2:32 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Oct. 19, 2017, 2:32 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile()

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-11-03 Thread Benjamin Mahler 


> On Nov. 3, 2017, 6:29 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > 
> >
> > The general pattern is to just include the reason for an error, and to 
> > not include any information the caller already has, because otherwise the 
> > callers will double log:
> > 
> > ```
> > Try copy = copyfile(source, destination);
> > 
> > if (copy.isError()) {
> >   return ("Failed to copy '" + source + "'"
> >   " to '" + destination + "': " + copy.error();
> > }
> > ```
> > 
> > The current code would log:
> > 
> > "Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space 
> > left"
> > 
> > Can you guys do a sweep for this issue in the windows related code that 
> > has been added?
> 
> Andrew Schwartzmeyer wrote:
> I'm not sure I follow. Are you saying the _caller_ should always write 
> the actual error message, versus the _callee_ preparing it here?
> 
> I guess in your example, I don't get why the user of `os::copyfile` would 
> add `"Failed to copy..."` instead of just doing:
> 
> ```
> if (copy.isError()) {
> return Error(copy.error());
> }
> ```
> 
> And then the error message is only written once, in `os::copyfile`.

What is the "actual" error message? :)

An error message consists of several parts, much like an exception: the 
"reason" for the error, and multiple "stacks" of context. If you're referring 
to the "reason" when you said "actual", either approach (the one we use, or the 
one used in this patch) includes the reason in their returned error message. 
The distinction lies in where the "stacks" of context get included.

The decision we took some time ago was to have the "owner" of the context be 
responsible for including it. So if I call `os::copyfile` I know which function 
I'm calling and which source and destination I'm passing into it. This matches 
posix-style programming which I why I think we chose this approach:

```
int main()
{
  int fd = open("/file");
  
  if (fd == -1) {
// Caller logs the thing it was doing, and gets the reason for the error:
LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << 
strerror(errno);
  }
}

vs

int main()
{
  Try fd = open("/file");
  
  if (fd.isError()) {
// Caller logs the thing it was doing, and gets the reason for the error:
LOG(ERROR) << "Failed to initialize: Failed to open '/file': " << 
fd.error();
  }
}
```

Now of course, if you use the alternative approach to have the leaf include all 
the information it has, then you have to compose differently:

```
int main()
{
  Try fd = os::open("/file");
  
  if (fd.isError()) {
// Caller knows that no additional context needs to be added because callee 
has all of it.
LOG(ERROR) << "Failed to initialize: " << fd.error();
  }
}
```

The approach we chose was to treat the error as just the "reason" (like 
strerror), so if you want to add context to it you can.

Both approaches work, but we have to pick one and apply it consistently as best 
we can. In retrospect, I actually think the other approach would have been a 
better choice because it fits more easily into Future::then style composition. 
But we would have to discuss the change of approach and do a sweep, because 
most of the code follows the first pattern shown.


- Benjamin


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


On Oct. 19, 2017, 9:32 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Oct. 19, 2017, 9:32 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-11-03 Thread Andrew Schwartzmeyer 


> On Nov. 3, 2017, 11:29 a.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > 
> >
> > The general pattern is to just include the reason for an error, and to 
> > not include any information the caller already has, because otherwise the 
> > callers will double log:
> > 
> > ```
> > Try copy = copyfile(source, destination);
> > 
> > if (copy.isError()) {
> >   return ("Failed to copy '" + source + "'"
> >   " to '" + destination + "': " + copy.error();
> > }
> > ```
> > 
> > The current code would log:
> > 
> > "Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space 
> > left"
> > 
> > Can you guys do a sweep for this issue in the windows related code that 
> > has been added?

I'm not sure I follow. Are you saying the _caller_ should always write the 
actual error message, versus the _callee_ preparing it here?

I guess in your example, I don't get why the user of `os::copyfile` would add 
`"Failed to copy..."` instead of just doing:

```
if (copy.isError()) {
return Error(copy.error());
}
```

And then the error message is only written once, in `os::copyfile`.


- Andrew


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


On Oct. 19, 2017, 2:32 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Oct. 19, 2017, 2:32 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-11-03 Thread Benjamin Mahler 

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




3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 56-57 (patched)


The general pattern is to just include the reason for an error, and to not 
include any information the caller already has, because otherwise the callers 
will double log:

```
Try copy = copyfile(source, destination);

if (copy.isError()) {
  return ("Failed to copy '" + source + "'"
  " to '" + destination + "': " + copy.error();
}
```

The current code would log:

"Failed to copy 's' to 'd': Failed to copy 's' to 'd': No disk space left"

Can you guys do a sweep for this issue in the windows related code that has 
been added?


- Benjamin Mahler


On Oct. 19, 2017, 9:32 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Oct. 19, 2017, 9:32 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/6/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-10-19 Thread Jeff Coffler 

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

(Updated Oct. 19, 2017, 9:32 p.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li 
Li.


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


Repository: mesos


Description
---

The os::copyfile() method provides an API to copy a file from one
location to another. Note that copying of directories are not
supported, and that the destination directory must exist before a
file can be copied into a directory.


Diffs (updated)
-

  3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
  3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
  3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
  3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 


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

Changes: https://reviews.apache.org/r/60621/diff/5-6/


Testing
---

See upstream

Note that Joe made some changes to this, I ended up taking his changes as is.


Thanks,

Jeff Coffler

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-10-19 Thread Jeff Coffler 


> On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 38 (patched)
> > 
> >
> > nit: whitespace

Removed blank line.


> On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 45-50 (patched)
> > 
> >
> > nit: indentation

Emacs default, sorry. Fixed.


> On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 37 (patched)
> > 
> >
> > nit: whitespace

Fixed.


> On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 51-55 (patched)
> > 
> >
> > nit: indentation

Fixed.


> On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os/copyfile_tests.cpp
> > Lines 79 (patched)
> > 
> >
> > nit: indentation

Fixed.


- Jeff


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


On Oct. 17, 2017, 1:16 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Oct. 17, 2017, 1:16 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/4/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-10-19 Thread Jeff Coffler 

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

(Updated Oct. 19, 2017, 6:16 p.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li 
Li.


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


Repository: mesos


Description
---

The os::copyfile() method provides an API to copy a file from one
location to another. Note that copying of directories are not
supported, and that the destination directory must exist before a
file can be copied into a directory.


Diffs (updated)
-

  3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
  3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
  3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
  3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 


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

Changes: https://reviews.apache.org/r/60621/diff/4-5/


Testing
---

See upstream

Note that Joe made some changes to this, I ended up taking his changes as is.


Thanks,

Jeff Coffler

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-10-17 Thread Andrew Schwartzmeyer 

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


Ship it!




Looks good to me other than some funky indentation. Content is fine; decision 
to not yet introduce `boost::filesystem` is fine.


3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 38 (patched)


nit: whitespace



3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 45-50 (patched)


nit: indentation



3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 37 (patched)


nit: whitespace



3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 51-55 (patched)


nit: indentation



3rdparty/stout/tests/os/copyfile_tests.cpp
Lines 79 (patched)


nit: indentation


- Andrew Schwartzmeyer


On Oct. 16, 2017, 6:16 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Oct. 16, 2017, 6:16 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/4/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-10-16 Thread Jeff Coffler 

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

(Updated Oct. 17, 2017, 1:16 a.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li 
Li.


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


Repository: mesos


Description (updated)
---

The os::copyfile() method provides an API to copy a file from one
location to another. Note that copying of directories are not
supported, and that the destination directory must exist before a
file can be copied into a directory.


Diffs (updated)
-

  3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
  3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
  3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
  3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 


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

Changes: https://reviews.apache.org/r/60621/diff/3-4/


Testing
---

See upstream

Note that Joe made some changes to this, I ended up taking his changes as is.


Thanks,

Jeff Coffler

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-10-16 Thread Jeff Coffler 


> On Oct. 8, 2017, 2:56 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os/copyfile_tests.cpp
> > Lines 33 (patched)
> > 
> >
> > This is being repeated... and it's already part of the base class: 
> > https://github.com/apache/mesos/blob/72752fc6deb8ebcbfbd5448dc599ef3774339d31/3rdparty/stout/include/stout/tests/utils.hpp#L64
> 
> Jeff Coffler wrote:
> This is an option in the base class, not the actual string. Simply 
> removing the line caused compilation problems.
> 
> Note that I got this pattern from other consumers of the base class ...
> 
> Andrew Schwartzmeyer wrote:
> I'm not seeing that. They almost all just use `sandbox.get()` from the 
> base class. Here's [one 
> example](https://github.com/apache/mesos/blob/72752fc6deb8ebcbfbd5448dc599ef3774339d31/src/tests/containerizer/io_switchboard_tests.cpp#L180).

Not all tests do this. Some use sandbox, some redefine. In any case, I've 
changed to be as you've suggested.


- Jeff


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


On Oct. 11, 2017, 11:30 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Oct. 11, 2017, 11:30 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new stout capability: os::copyfile(source, dest).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/3/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-10-13 Thread Andrew Schwartzmeyer 


> On Oct. 7, 2017, 7:56 p.m., Andrew Schwartzmeyer wrote:
> > The commit message needs to be in the past tense, and generally you can 
> > ignore mentioning stout as the files imply it (though it still gets 
> > mentioned in commits a lot). E.g.::
> > 
> > > Added `os::copyfile(from, to)`.
> > > This patch...
> > 
> > Currently the description is a copy of the summary (which happens when the 
> > commit body is left empty). This should usually be avoided (the exception 
> > being trivial commits).

This still needs to be addressed.


> On Oct. 7, 2017, 7:56 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 29-32 (patched)
> > 
> >
> > Since this implementation _doesn't_ use the equivalent of `cp`, it 
> > makes even more sense to be consistent in the POSIX implementation.
> 
> Jeff Coffler wrote:
> But the 'cp' program has behavior that the underlying code never depended 
> on. Rather than force us on Windows to NOT be able to use CopyFile (or to 
> write a bunch of extra code to copy directories, etc), I'd rather have the 
> code be restricted to operations it uses today. Then, if more operations are 
> needed later, we can implement those at the time.
> 
> Basic agile programming. Do what you need when you need it, not because 
> maybe you'll need it down the line, maybe not.

> or to write a bunch of extra code

It really wouldn't be a bunch of extra code to use `boost::filesystem` (and 
then literally just drop the `boost` part when we move to C++17), but it's fine.

Our long-term plan with Mesos is to pare down `stout`, especially when 
platform-differences no longer need to be handled by use but instead by 
improved standard C++ libraries.


> On Oct. 7, 2017, 7:56 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os/copyfile_tests.cpp
> > Lines 33 (patched)
> > 
> >
> > This is being repeated... and it's already part of the base class: 
> > https://github.com/apache/mesos/blob/72752fc6deb8ebcbfbd5448dc599ef3774339d31/3rdparty/stout/include/stout/tests/utils.hpp#L64
> 
> Jeff Coffler wrote:
> This is an option in the base class, not the actual string. Simply 
> removing the line caused compilation problems.
> 
> Note that I got this pattern from other consumers of the base class ...

I'm not seeing that. They almost all just use `sandbox.get()` from the base 
class. Here's [one 
example](https://github.com/apache/mesos/blob/72752fc6deb8ebcbfbd5448dc599ef3774339d31/src/tests/containerizer/io_switchboard_tests.cpp#L180).


- Andrew


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


On Oct. 11, 2017, 4:30 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> ---
> 
> (Updated Oct. 11, 2017, 4:30 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new stout capability: os::copyfile(source, dest).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/3/
> 
> 
> Testing
> ---
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-10-12 Thread Jeff Coffler 


> On Oct. 8, 2017, 2:56 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 16 (patched)
> > 
> >
> > What was this included for? My only guess is `WIFEXITED`?

Removed, no longer needed.


> On Oct. 8, 2017, 2:56 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 43-45 (patched)
> > 
> >
> > This sounds to me like the problem isn't `stat::isdir` but that we 
> > didn't test for existence first, which would have avoided this edge case.

Not exactly. I really don't want the destination to imply a directory in any 
shape or form. Implying a directory, even if it does not exist, is not what I 
want.

The intent here is to try and make the 'cp' command behave like an 'API'. Sure, 
I could code 'cp' in code, but I'm quite sure that the actual 'cp' program can 
make intelligent decisions for performance that I shouldn't need to do myself.

This is the point of the Windows "CopyFile" API, which can make intelligent 
decisions on how to get the file copied very quickly, regardless of file 
system, size, etc.

I reworded the comment to avoid any implication of "defect" in stat::isdir.


> On Oct. 8, 2017, 2:56 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 60-71 (patched)
> > 
> >
> > Do we have to use `cp`? Generally we should prefer just using C++ to 
> > shelling out for something like copying a file, especially if we're 
> > implementing a stout function like `os::copyfile`.

I don't agree with this, as I implied in a prior comment. An API (which Linux 
doesn't offer), or the 'cp' command, can do all sorts of special things to make 
the file get copied faster (i.e. create new file pointers, use larger buffer 
sizes, etc). I would rather depend on a system utility to do this "right" and 
as fast as possible regardless of underlying file system.

That's one of the reasons that Windows implemented this as an API. It can be 
optimized to do things much faster than "read a block, write a block, until 
EOF". For example, large block sizes in copying is much faster than small block 
sizes, but to a point (not beyond the size can be read in a single operation).


> On Oct. 8, 2017, 2:56 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 29-32 (patched)
> > 
> >
> > Since this implementation _doesn't_ use the equivalent of `cp`, it 
> > makes even more sense to be consistent in the POSIX implementation.

But the 'cp' program has behavior that the underlying code never depended on. 
Rather than force us on Windows to NOT be able to use CopyFile (or to write a 
bunch of extra code to copy directories, etc), I'd rather have the code be 
restricted to operations it uses today. Then, if more operations are needed 
later, we can implement those at the time.

Basic agile programming. Do what you need when you need it, not because maybe 
you'll need it down the line, maybe not.


> On Oct. 8, 2017, 2:56 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 52-53 (patched)
> > 
> >
> > s/Posix/POSIX/g
> > 
> > This is not apparent in the POSIX implementation at all. Is it just a 
> > quirk of `cp` or are we calling it specially or what?

The 'cp' program allows destination files to be overwritten. And frankly, from 
an API perspective for stout, this seemed reasonable to me. Sure, I could make 
it easy enough to not allow this in 'cp' with a "precheck", but in this case, I 
didn't feel it was necessary.

I did change the POSIX casing.


> On Oct. 8, 2017, 2:56 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os/copyfile_tests.cpp
> > Lines 33 (patched)
> > 
> >
> > This is being repeated... and it's already part of the base class: 
> > https://github.com/apache/mesos/blob/72752fc6deb8ebcbfbd5448dc599ef3774339d31/3rdparty/stout/include/stout/tests/utils.hpp#L64

This is an option in the base class, not the actual string. Simply removing the 
line caused compilation problems.

Note that I got this pattern from other consumers of the base class ...


- Jeff


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


On Oct. 11, 2017, 11:30 p.m., Jeff Coffler wrote:
> 
> ---
> This

Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

2017-10-11 Thread Jeff Coffler 

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

(Updated Oct. 11, 2017, 11:30 p.m.)


Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li 
Li.


Summary (updated)
-

Added new stout capability: os::copyfile(source, dest).


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


Repository: mesos


Description (updated)
---

Added new stout capability: os::copyfile(source, dest).


Diffs (updated)
-

  3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
  3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
  3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
  3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 


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

Changes: https://reviews.apache.org/r/60621/diff/2-3/


Testing
---

See upstream

Note that Joe made some changes to this, I ended up taking his changes as is.


Thanks,

Jeff Coffler