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 42016: Windows: Apply patch.exe without elevation prompt
Date Mon, 25 Jan 2016 19:51:40 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42016/#review116138
-----------------------------------------------------------




3rdparty/CMakeLists.txt (line 45)
<https://reviews.apache.org/r/42016/#comment177143>

    Could we please delete the extra comment line here?



3rdparty/CMakeLists.txt (lines 45 - 70)
<https://reviews.apache.org/r/42016/#comment177145>

    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)
<https://reviews.apache.org/r/42016/#comment177144>

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



3rdparty/CMakeLists.txt (line 48)
<https://reviews.apache.org/r/42016/#comment177146>

    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)
<https://reviews.apache.org/r/42016/#comment177147>

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



3rdparty/CMakeLists.txt (line 61)
<https://reviews.apache.org/r/42016/#comment177150>

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



3rdparty/CMakeLists.txt (line 63)
<https://reviews.apache.org/r/42016/#comment177151>

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



3rdparty/CMakeLists.txt (line 66)
<https://reviews.apache.org/r/42016/#comment177154>

    Confusingly, our CMake style differs a bit from our C++ style. Can we please make it like
this:
    
    ```
      add_custom_command(
        OUTPUT  patch.exe
        COMMAND ${APPLY_PATCH_MANIFEST_COMMAND})
    ```
    
    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)
<https://reviews.apache.org/r/42016/#comment177152>

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



3rdparty/CMakeLists.txt (line 72)
<https://reviews.apache.org/r/42016/#comment177155>

    Can we please delete the extra line here?



CMakeLists.txt (lines 88 - 116)
<https://reviews.apache.org/r/42016/#comment177129>

    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)
<https://reviews.apache.org/r/42016/#comment177148>

    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)
<https://reviews.apache.org/r/42016/#comment177137>

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



CMakeLists.txt (line 98)
<https://reviews.apache.org/r/42016/#comment177136>

    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)
<https://reviews.apache.org/r/42016/#comment177130>

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



CMakeLists.txt (line 104)
<https://reviews.apache.org/r/42016/#comment177132>

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



CMakeLists.txt (line 107)
<https://reviews.apache.org/r/42016/#comment177133>

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



CMakeLists.txt (line 108)
<https://reviews.apache.org/r/42016/#comment177135>

    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 them.
    
    This would look like:
    
    ```
    if (NOT EXISTS ${GNUWIN32_PATCH_EXECUTABLE})
      ...
    else (NOT EXISTS ${GNUWIN32_PATCH_EXECUTABLE})
      ...
    endif(NOT EXISTS ${GNUWIN32_PATCH_EXECUTABLE})
    ```
    
    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)
<https://reviews.apache.org/r/42016/#comment177134>

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


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