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

Reply via email to