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