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

Reply via email to