----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37273/#review98687 -----------------------------------------------------------
3rdparty/libprocess/3rdparty/CMakeLists.txt (line 75) <https://reviews.apache.org/r/37273/#comment155227> I'd like to suggest that this comment is not necessary. :) 3rdparty/libprocess/3rdparty/CMakeLists.txt (line 76) <https://reviews.apache.org/r/37273/#comment155228> I need to write down a style guide, but can we for now follow the Google C++ style guide as closely as possible, and put `GLOG` here on a new line? 3rdparty/libprocess/3rdparty/CMakeLists.txt (line 177) <https://reviews.apache.org/r/37273/#comment155234> Ditto comment about moving `GLOG` to new line. 3rdparty/libprocess/3rdparty/CMakeLists.txt (line 192) <https://reviews.apache.org/r/37273/#comment155226> Hmm, does protobuf build for you? This command should fail on VS2015 because stdex is not included in VS2015 (see[1] and [2]). Because this issue does not appear to be resolved in the current release of protobuf[3], we will probably need to change the projects to include `-D _SILENCE_STDEXT_HASH_DEPRECATION_WARNINGS`, either through a patch file, or by somehow passing the flags into the build system on Windows. I'm not sure which is more realistic. [1] http://stackoverflow.com/questions/30430789/c-hash-deprecation-warning [2] http://blogs.msdn.com/b/vcblog/archive/2014/06/06/c-14-stl-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1.aspx [3] https://github.com/google/protobuf/issues/792 3rdparty/libprocess/3rdparty/CMakeLists.txt (line 193) <https://reviews.apache.org/r/37273/#comment155235> Also, please move `PROTOBUF` to new line, see comment about `GLOG`. 3rdparty/libprocess/cmake/ProcessConfigure.cmake (line 92) <https://reviews.apache.org/r/37273/#comment155232> Following up from the comment thread with Joseph, what is this responding to? Is the protobuf include directory required to build the process library? It seems to build fine for me. Another note: if we actually need to include protobufs here and I simply missed it, then we have to do the following also: * Add `${PROTOBUF_TARGET}` to `${PROCESS_DEPENDENCIES}` (if you don't add this, then CMake will not require protobufs to be uncompressed and built before the process library is built) * Add appropriate protobuf library paths to `${PROCESS_LIB_DIRS}` * Add appropriate flags to `${PROCESS_LIBS}` (otherwise the appropriate -l flags won't be generated on linux). We punt on some of these in process library tests because we don't need to link to protobufs yet, but if you move it and we have a script that builds them, it's worth adding them. 3rdparty/libprocess/cmake/ProcessConfigure.cmake (line 119) <https://reviews.apache.org/r/37273/#comment155233> I can't remember if I've brought this up, but can we remove this message? I'd personally like to keep the configure/build output clear. 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 22) <https://reviews.apache.org/r/37273/#comment155243> Rather than interpolating a `'#'` between the projects, can we pass in all the projects as a list and loop through them here with a `for` instead? Something like: ``` for %%I IN (%*) DO (... stuff goes here ...) ``` 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 23) <https://reviews.apache.org/r/37273/#comment155244> Yak shaving: Is there a way to make this pluggable so we can get debug builds too? If it's too much work, can we add a JIRA ticket to investigate this? 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 29) <https://reviews.apache.org/r/37273/#comment155248> Consider new names for `KEY_NAME` and `VALUE_NAME` here. Perhaps `VS2015_KEY` and `VS2015_INSTALLDIR_VALUE`, or something along those lines? That would make the `REG QUERY` more readable below. 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 33) <https://reviews.apache.org/r/37273/#comment155251> Could we change `MSVC_DIR` to be something like `VS2015_DIR` to make it clear that it is supposed to point to VS2015 directory specifically? This would make it clear that when we set the `SOLUTION_VER` below, that it's supposed to be 12 -- right now the `if defined MSVC_DIR ( set SOLUTION_VER=12.00 )` makes it seem like we want to set `SOLUTION_VER=12.00` regardless of the `MSVC_DIR` we find. It would be much clearer to write `if defined VS2015_DIR ( set SOLUTION_VER=12.00 )` instead. 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 44) <https://reviews.apache.org/r/37273/#comment155252> Can we add a comment here saying that this is the VS Command Prompt? Right now it's kind of opaque. 3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 46) <https://reviews.apache.org/r/37273/#comment155255> Could we add a comment here explaining that this is upgrading the solution if it's not VS2015? That's important to point out, because it might lead to unexpected behavior if the user isn't expecting it. Also, can we print out a message when we do this, so the user clearly sees that it's happening in the logs? 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake (line 22) <https://reviews.apache.org/r/37273/#comment155239> For the `LIB_NAME_UPPER` parameter here, I suggest that it would be better to pass in any string, and uppercase it inside the method. 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake (line 26) <https://reviews.apache.org/r/37273/#comment155240> Can we standardize on where to find the build script, instead of searching for it? 3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake (line 29) <https://reviews.apache.org/r/37273/#comment155242> Perhaps we should get rid of the extra space after `BUILD_CMD_VAR` here. We tend to only line up the values of a set if they form a logical block, like the build/install/configure commands in the 3rdparty `CMakeLists.txt`. - Alex Clemmer On Sept. 9, 2015, 2:40 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37273/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2015, 2:40 a.m.) > > > Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van > Remoortere, 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 > >
