This is an automatically generated e-mail. To reply, visit:

3rdparty/CMakeLists.txt (line 45)

    Could we please delete the extra comment line here?

3rdparty/CMakeLists.txt (lines 45 - 70)

    Similarly to my reasoning above, it seems like this should actually all go 
in `Mesos3rdpartyConfigure.cmake`.
    EDIT: Oh, sorry, everything except the last line, where we generate the 
`ZOOKEEPER_PATCH_CMD`. That can stay in this file.

3rdparty/CMakeLists.txt (lines 46 - 47)

    Could we please wrap these lines at 80 columns and put ``` characters 
aroudn the path?

3rdparty/CMakeLists.txt (line 48)

    The comment here seems like it could be clearer -- it's not actually 
setting a directory, it's setting the environment variable that we use to 
retrieve the tmp directory, right?
    I recommend just changing the code to something like the following (NOTE: I 
have NOT tested this):
      # Points at user temp directory, e.g. `\Users\<user>\AppData\Local\Temp`.
      set(USER_TMP_DIR $ENV{"TMP"})
    This would simplify some of the logic below also, as well as making the 
comment less confusing.

3rdparty/CMakeLists.txt (line 49)

    Can we put a space between these "logical blocks" of code, please? Same 
goes for line 53.

3rdparty/CMakeLists.txt (line 61)

    Can we please actually delete all the lines that are just empty comments?

3rdparty/CMakeLists.txt (line 63)

    Can we please wrap this at 80 characters?
    Also: can you please expand this comment slightly? For my own 
understanding, it is worth mentioning that `patch` does not require admin, but 
asks for it anyway, and this manifest just tells Windows to run it without. If 
you have a URL that shows this has precedent out in the community, that is even 

3rdparty/CMakeLists.txt (line 66)

    Confusingly, our CMake style differs a bit from our C++ style. Can we 
please make it like this:
        OUTPUT  patch.exe
    Note also that I've taken out the extra spaces between `COMMAND` and the 
variable. Please do this as well. :)

3rdparty/CMakeLists.txt (line 69)

    Can we please add a space between the `#` and `Third`?

3rdparty/CMakeLists.txt (line 72)

    Can we please delete the extra line here?

CMakeLists.txt (lines 88 - 116)

    I believe this should go in `3rdparty/cmake/Mesos3rdPartyConfigure.cmake`. 
We like to have the `*Configure.cmake` files contain all the intense logic of 
configuring things like how to run the patch command, so that the 
`CMakeLists.txt` only do very simple things, like run the patch command 
(instead of configuring it and running it).
    We do this for two reasons. First, because CMake has incredibly strange 
dynamic scoping rules for variable evaluation, which means that it is really 
worth collecting all the intense platform-dependent config logic into one 
place. If you don't, then it gets _really, really hard_ to tell when you're 
setting one variable, or why some other variable has the value it does.
    Second, because when we have all the configuration logic in one place, it 
makes it much easier to reason about where configuration logic rests. For 
example, if I was looking for where we're configuring the `patch.exe` on 
Windows, I'd start by looking in `Mesos3rdpartyConfigure.cmake`, because I know 
we are patching things in the `3rdparty` directory (as well as 
`3rdparty/libprocess/3rdparty`) and so the most general configuration file it 
would be in is the one for the `3rdparty` directory.

CMakeLists.txt (lines 88 - 89)

    Please make comments end with periods. Since this happens throughout the 
patch, I'll just mention it once instead of opening a billion issues for you to 
close. :)
    Also this might be reworded a bit to make it clearer. Perhaps: `Configure 
Windows use of the GNU patch utility; we attempt to find it in its default 
location, but this path may be customized also.`

CMakeLists.txt (lines 95 - 96)

    Looks like these two lines are > 80 characters long. Can we please break 
them up?

CMakeLists.txt (line 98)

    Our style is following K&R C style: we put a space between the `if` and the 
`(`. Like this: `if (WIN32)`.
    Please also do this for every conditional like `else` and `endif`

CMakeLists.txt (line 102)

    Should there be a period and a space at the end of this line?

CMakeLists.txt (line 104)

    Perhaps change this to indicate that we use it to apply updates generally 
to third-party packages?

CMakeLists.txt (line 107)

    Hmm, is this needed? Logging a `FATAL_ERROR` will stop the build, so I 
think it's not necessary.

CMakeLists.txt (line 108)

    For else and elseif, can we please add the conditional? Otherwise it gets 
really confusing which `endif` is closing which `if` when you have a lot of 
    This would look like:
    Note that the `else` clause behaves the same as it did before, it's not an 
`elseif` or anything like that.

CMakeLists.txt (lines 110 - 113)

    Can we please indent these lines?

- Alex Clemmer

On Jan. 14, 2016, 12:43 a.m., M Lawindi wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42016/
> -----------------------------------------------------------
> (Updated Jan. 14, 2016, 12:43 a.m.)
> Review request for mesos, Alex Naparu, Daniel Pravat, Alex Clemmer, M 
> Lawindi, and Yi Sun.
> Repository: mesos
> Description
> -------
> Windows:[2/2] Added zookeeper-3.4.5 to Mesos build
> Diffs
> -----
>   3rdparty/CMakeLists.txt ac5c25a8797a687e84384682975ab99fb3e30448 
>   3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
> 34e61ff90eca0ffdddb6b6b8e2f8e552691637fa 
>   3rdparty/patch.exe.manifest PRE-CREATION 
>   CMakeLists.txt 9b7044b7860fd64b854ac27b28a48d297dfdeae8 
>   src/slave/cmake/SlaveConfigure.cmake 
> cf378a27297474b2a9f338e0c832612370f7302a 
> Diff: https://reviews.apache.org/r/42016/diff/
> Testing
> -------
> Thanks,
> M Lawindi

Reply via email to