> On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/os/posix/copyfile.hpp > > Lines 26 (patched) > > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line26> > > > > Oops, can't include a namespace in a header.
I removed this and resolved resulting compilation issues. > On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/os/posix/copyfile.hpp > > Lines 30 (patched) > > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line30> > > > > There's no good reason to return the destination path when successful, > > as the same path is already known to the caller. We can go with a > > `Try<Nothing>` instead. Okay, changed. > On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/os/posix/copyfile.hpp > > Lines 31 (patched) > > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line31> > > > > The checks before `cp` are all good (and they are consistent with how > > the fetcher currently/coincidentally passes arguments in). However, these > > checks should be applied to the `sourcePath` too. i.e. Neither the source > > nor destination can be a directory; or a relative path. Yeah, that's good. But the source can be relative, and that's okay. Either we found the source or we did not. I think that's okay to not be absolute (and, as I recall, a few unit tests check for that specifically). Changed code to check source path isn't a directory and that it doesn't end in "/". > On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/os/posix/copyfile.hpp > > Lines 46-47 (patched) > > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line46> > > > > Instead of indexing, we can just use `destinationPath.back()` (C++11 !) Done. > On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/os/posix/copyfile.hpp > > Lines 47 (patched) > > <https://reviews.apache.org/r/60621/diff/1/?file=1768693#file1768693line47> > > > > Don't need to check for backslashes on Posix. Originally this ran on Windows too, and was #ifdef'ed. Resolved. - Jeff ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60621/#review182914 ----------------------------------------------------------- On July 3, 2017, 7:29 p.m., Jeff Coffler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60621/ > ----------------------------------------------------------- > > (Updated July 3, 2017, 7:29 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 7956d1ae392cd3fa560474bc3ac38877cddce857 > 3rdparty/stout/include/Makefile.am dec7293949e16b6580509c2fd41a851e349fbef4 > 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 > 8d881ab7ac571dea7aace269332a856feb7a6c43 > 3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/60621/diff/1/ > > > Testing > ------- > > See upstream > > > Thanks, > > Jeff Coffler > >
