You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jeff Coffler <je...@taltos.com> on 2017/10/06 04:03:19 UTC
Re: Review Request 60621: Add new stout capability: os::copyfile.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60621/
-----------------------------------------------------------
(Updated Oct. 6, 2017, 4:03 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
-------
Add new stout capability: os::copyfile.
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/2/
Changes: https://reviews.apache.org/r/60621/diff/1-2/
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).
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> 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)
> > <https://reviews.apache.org/r/60621/diff/2/?file=1847145#file1847145line29>
> >
> > 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)
> > <https://reviews.apache.org/r/60621/diff/2/?file=1847147#file1847147line33>
> >
> > 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).
Posted by Jeff Coffler <je...@taltos.com>.
> On Oct. 8, 2017, 2:56 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 16 (patched)
> > <https://reviews.apache.org/r/60621/diff/2/?file=1847144#file1847144line16>
> >
> > 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)
> > <https://reviews.apache.org/r/60621/diff/2/?file=1847144#file1847144line43>
> >
> > 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)
> > <https://reviews.apache.org/r/60621/diff/2/?file=1847144#file1847144line60>
> >
> > 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)
> > <https://reviews.apache.org/r/60621/diff/2/?file=1847145#file1847145line29>
> >
> > 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)
> > <https://reviews.apache.org/r/60621/diff/2/?file=1847145#file1847145line52>
> >
> > 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)
> > <https://reviews.apache.org/r/60621/diff/2/?file=1847147#file1847147line33>
> >
> > 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 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).
Posted by Jeff Coffler <je...@taltos.com>.
> On Oct. 8, 2017, 2:56 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os/copyfile_tests.cpp
> > Lines 33 (patched)
> > <https://reviews.apache.org/r/60621/diff/2/?file=1847147#file1847147line33>
> >
> > 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: Add new stout capability: os::copyfile.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60621/#review187346
-----------------------------------------------------------
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).
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 16 (patched)
<https://reviews.apache.org/r/60621/#comment264288>
What was this included for? My only guess is `WIFEXITED`?
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 29-31 (patched)
<https://reviews.apache.org/r/60621/#comment264281>
Can this comment document why we don't support a directory as the destination path?
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 33-34 (patched)
<https://reviews.apache.org/r/60621/#comment264287>
Per the style guide:
> We use snake_case for variable names in the libprocess and stout libraries.
I'll let this issue cover the rest of the review.
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 39-40 (patched)
<https://reviews.apache.org/r/60621/#comment264280>
s/and/nor/g
s/a directory/directories/g
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 43-45 (patched)
<https://reviews.apache.org/r/60621/#comment264283>
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.
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 48-49 (patched)
<https://reviews.apache.org/r/60621/#comment264282>
This is an exacty copy of the message above; let's not duplicate it. The error conditions can be checked in the same expression.
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 52-53 (patched)
<https://reviews.apache.org/r/60621/#comment264284>
Cool, but this should probably be in the header comment, it's a pretty important exception.
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 54 (patched)
<https://reviews.apache.org/r/60621/#comment264289>
I didn't see `stout/path` included. IWYU.
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 60-71 (patched)
<https://reviews.apache.org/r/60621/#comment264285>
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`.
3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 29-32 (patched)
<https://reviews.apache.org/r/60621/#comment264286>
Since this implementation _doesn't_ use the equivalent of `cp`, it makes even more sense to be consistent in the POSIX implementation.
3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 52-53 (patched)
<https://reviews.apache.org/r/60621/#comment264290>
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?
3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 56-59 (patched)
<https://reviews.apache.org/r/60621/#comment264291>
This is probably cleaner as `if (!::CopyFileW` since the Windows `BOOL` and `FALSE` types are so weird.
3rdparty/stout/tests/os/copyfile_tests.cpp
Lines 33 (patched)
<https://reviews.apache.org/r/60621/#comment264292>
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
- Andrew Schwartzmeyer
On Oct. 5, 2017, 9:03 p.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2017, 9:03 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
> -------
>
> Add new stout capability: os::copyfile.
>
>
> 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/2/
>
>
> 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).
Posted by Benjamin Mahler <bm...@apache.org>.
> On Nov. 3, 2017, 6:29 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > <https://reviews.apache.org/r/60621/diff/6/?file=1864337#file1864337line56>
> >
> > 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<Nothing> 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<int> 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<int> 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
> 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).
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Nov. 3, 2017, 11:29 a.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > <https://reviews.apache.org/r/60621/diff/6/?file=1864337#file1864337line56>
> >
> > 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<Nothing> 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<int> 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<int> 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() 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).
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Nov. 3, 2017, 11:29 a.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > <https://reviews.apache.org/r/60621/diff/6/?file=1864337#file1864337line56>
> >
> > 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<Nothing> 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).
Posted by Jeff Coffler <je...@taltos.com>.
> On Nov. 3, 2017, 6:29 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/os/windows/copyfile.hpp
> > Lines 56-57 (patched)
> > <https://reviews.apache.org/r/60621/diff/6/?file=1864337#file1864337line56>
> >
> > 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<Nothing> 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<int> 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<int> 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/
> -----------------------------------------------------------
>
> (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).
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/60621/#comment267296>
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<Nothing> 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).
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
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).
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
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).
Posted by Jeff Coffler <je...@taltos.com>.
> On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 38 (patched)
> > <https://reviews.apache.org/r/60621/diff/4/?file=1858855#file1858855line38>
> >
> > 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)
> > <https://reviews.apache.org/r/60621/diff/4/?file=1858855#file1858855line45>
> >
> > 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)
> > <https://reviews.apache.org/r/60621/diff/4/?file=1858856#file1858856line37>
> >
> > 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)
> > <https://reviews.apache.org/r/60621/diff/4/?file=1858856#file1858856line51>
> >
> > nit: indentation
Fixed.
> On Oct. 17, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/os/copyfile_tests.cpp
> > Lines 79 (patched)
> > <https://reviews.apache.org/r/60621/diff/4/?file=1858858#file1858858line79>
> >
> > 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).
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/60621/#comment265406>
nit: whitespace
3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 45-50 (patched)
<https://reviews.apache.org/r/60621/#comment265407>
nit: indentation
3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 37 (patched)
<https://reviews.apache.org/r/60621/#comment265408>
nit: whitespace
3rdparty/stout/include/stout/os/windows/copyfile.hpp
Lines 51-55 (patched)
<https://reviews.apache.org/r/60621/#comment265409>
nit: indentation
3rdparty/stout/tests/os/copyfile_tests.cpp
Lines 79 (patched)
<https://reviews.apache.org/r/60621/#comment265410>
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).
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
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).
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
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
Re: Review Request 60621: Add new stout capability: os::copyfile.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60621/#review187352
-----------------------------------------------------------
Summary and description need to follow Mesos standards (past-tense, full-sentence summary, with description in the body).
- Andrew Schwartzmeyer
On Oct. 5, 2017, 9:03 p.m., Jeff Coffler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2017, 9:03 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
> -------
>
> Add new stout capability: os::copyfile.
>
>
> 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/2/
>
>
> Testing
> -------
>
> See upstream
>
> Note that Joe made some changes to this, I ended up taking his changes as is.
>
>
> Thanks,
>
> Jeff Coffler
>
>