mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Clemmer" <clemmer.alexan...@gmail.com>
Subject Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in libprocess.
Date Fri, 11 Sep 2015 22:41:25 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message