You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by haosdent huang <ha...@gmail.com> on 2015/08/30 15:40:31 UTC
Re: Review Request 37273: [1/3]Add CMake macro VsBuildCommand in
libprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37273/
-----------------------------------------------------------
(Updated Aug. 30, 2015, 1:40 p.m.)
Review request for mesos and Alex Clemmer.
Summary (updated)
-----------------
[1/3]Add CMake macro VsBuildCommand in libprocess.
Repository: mesos
Description
-------
Add CMake macro VsBuildCommand in libprocess.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b
3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78
3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e
3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION
3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION
Diff: https://reviews.apache.org/r/37273/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in
libprocess.
Posted by haosdent huang <ha...@gmail.com>.
> On Aug. 31, 2015, 6:46 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/CMakeLists.txt, lines 72-84
> > <https://reviews.apache.org/r/37273/diff/3/?file=1060006#file1060006line72>
> >
> > (This is just speculation, since I haven't tried this on a Windows box.)
> > `TRUE` won't quite work on Windows, because it'll get run as a binary, and therefore won't be found.
> >
> > Perhaps you should define a macro, like:
> >
> > ```
> > if (WIN32)
> > set(CMAKE_NOOP ${CMAKE_COMMAND} -E echo)
> > else (WIN32)
> > set(CMAKE_NOOP TRUE)
> > endif (WIN32)
> > ```
> >
> > The `-E echo` is fairly close to a no-op.
> >
> > See: http://public.kitware.com/pipermail/cmake/2014-June/057885.html
Thank you very much! Because I install mingw in windows before, so it could pass. I have already remove it.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37273/#review97108
-----------------------------------------------------------
On Sept. 5, 2015, noon, haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37273/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2015, noon)
>
>
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add CMake macro VsBuildCommand in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b
> 3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78
> 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e
> 3rdparty/libprocess/cmake/macros/Noop.cmake PRE-CREATION
> 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION
> 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37273/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in
libprocess.
Posted by haosdent huang <ha...@gmail.com>.
> On Aug. 31, 2015, 6:46 p.m., Joseph Wu wrote:
> > For any future CMake reviews, could you also add Artem (hartem) and I (kaysoky) as reviewers?
> > Thanks in advance :)
Sure? thank you very much.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37273/#review97108
-----------------------------------------------------------
On Aug. 30, 2015, 1:42 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37273/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2015, 1:42 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add CMake macro VsBuildCommand in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b
> 3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78
> 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e
> 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION
> 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37273/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in
libprocess.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37273/#review97108
-----------------------------------------------------------
For any future CMake reviews, could you also add Artem (hartem) and I (kaysoky) as reviewers?
Thanks in advance :)
3rdparty/libprocess/3rdparty/CMakeLists.txt (lines 68 - 71)
<https://reviews.apache.org/r/37273/#comment152860>
This comment needs some rewording. Something like:
# NOTE: The no-ops are important in the following commands. Building \`glog\` on WIN32 must be done in Visual Studio. In that case, these commands must be no-ops. If the no-ops are removed, then CMake will treat \`glog\` as an empty build command and will attemp to build \`glog\` as a CMake project.
3rdparty/libprocess/3rdparty/CMakeLists.txt (lines 72 - 84)
<https://reviews.apache.org/r/37273/#comment152857>
(This is just speculation, since I haven't tried this on a Windows box.)
`TRUE` won't quite work on Windows, because it'll get run as a binary, and therefore won't be found.
Perhaps you should define a macro, like:
```
if (WIN32)
set(CMAKE_NOOP ${CMAKE_COMMAND} -E echo)
else (WIN32)
set(CMAKE_NOOP TRUE)
endif (WIN32)
```
The `-E echo` is fairly close to a no-op.
See: http://public.kitware.com/pipermail/cmake/2014-June/057885.html
3rdparty/libprocess/3rdparty/CMakeLists.txt (line 175)
<https://reviews.apache.org/r/37273/#comment152880>
For clarity, I think you should comment that this function overwrites the `GMOCK_BUILD_CMD`. Same for the other usages.
3rdparty/libprocess/3rdparty/CMakeLists.txt (line 189)
<https://reviews.apache.org/r/37273/#comment152881>
Comment, same reason as above.
3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 19)
<https://reviews.apache.org/r/37273/#comment152865>
I think the mesos style should still apply here. So you should add a period at the end of all comments.
3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 24)
<https://reviews.apache.org/r/37273/#comment152867>
Delimited
3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (lines 30 - 40)
<https://reviews.apache.org/r/37273/#comment152872>
Is there any cleaner way of finding the MSVC install directory?
Try reading the directory from the registry:
* http://stackoverflow.com/questions/445167/how-can-i-get-the-value-of-a-registry-key-from-within-a-batch-script
* http://pascoal.net/2011/04/getting-visual-studio-installation-directory/
3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 35)
<https://reviews.apache.org/r/37273/#comment152866>
Period at the end.
3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (lines 39 - 43)
<https://reviews.apache.org/r/37273/#comment152874>
Instead of the `goto`, use `ELSE`:
```
) ELSE (
exit
)
```
- Joseph Wu
On Aug. 30, 2015, 6:42 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37273/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2015, 6:42 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add CMake macro VsBuildCommand in libprocess.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b
> 3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78
> 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e
> 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION
> 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37273/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in
libprocess.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37273/
-----------------------------------------------------------
(Updated Aug. 30, 2015, 1:42 p.m.)
Review request for mesos and Alex Clemmer.
Summary (updated)
-----------------
[1/2]Add CMake macro VsBuildCommand in libprocess.
Repository: mesos
Description
-------
Add CMake macro VsBuildCommand in libprocess.
Diffs
-----
3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b
3rdparty/libprocess/cmake/ProcessConfigure.cmake 12506a1369de005285268f895f365aba0c560f78
3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e
3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION
3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION
Diff: https://reviews.apache.org/r/37273/diff/
Testing
-------
Thanks,
haosdent huang