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