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