Re: Review Request 60621: Add new stout capability: os::copyfile.

[Andrew Schwartzmeyer]

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

Re: Review Request 60621: Add new stout capability: os::copyfile.

[Andrew Schwartzmeyer]

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


What was this included for? My only guess is `WIFEXITED`?



3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 29-31 (patched)


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)


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)


s/and/nor/g
s/a directory/directories/g



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.



3rdparty/stout/include/stout/os/posix/copyfile.hpp
Lines 48-49 (patched)


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)


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)


I didn't see `stout/path` included. IWYU.



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



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.



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?



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


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)


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 
>   

Re: Review Request 60621: Add new stout capability: os::copyfile.

[Jeff Coffler]

---
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: Add new stout capability: os::copyfile.

[Jeff Coffler]

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

(Updated Aug. 24, 2017, 10:39 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 (updated)
---

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.

[Jeff Coffler]

> On Aug. 16, 2017, 12:20 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 26 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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` 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 60621: Add new stout capability: os::copyfile.

[Andrew Schwartzmeyer]

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




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


Not sure how I missed that. Good catch.



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


This is odd. I though most of this code was copied from the original 
implementation (deleted in https://reviews.apache.org/r/60628) but apparently 
it was changed in the copy. I should have taken a closer look. That does 
explain why the logging is still here.


- Andrew Schwartzmeyer


On July 3, 2017, 12: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, 12: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
> 
>