You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by M Lawindi <ml...@microsoft.com> on 2016/01/14 01:43:46 UTC
Re: Review Request 42016: Windows: Apply patch.exe without elevation
prompt
-----------------------------------------------------------
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 (updated)
-------
Windows:[2/2] Added zookeeper-3.4.5 to Mesos build
Diffs (updated)
-----
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
Re: Review Request 42016: Windows:[2/2] Added zookeeper-3.4.5 to
Mesos build.
Posted by M Lawindi <ml...@microsoft.com>.
> On Jan. 25, 2016, 7:51 p.m., Alex Clemmer wrote:
> > 3rdparty/CMakeLists.txt, line 48
> > <https://reviews.apache.org/r/42016/diff/2/?file=1192435#file1192435line48>
> >
> > 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.
This is only setting the string "TMP"
The line after takes care of the environment variable use: set(PATCHEXE_LOCATION $ENV{${USER_TMP_DIR}}/patch.exe)
- M
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42016/#review116138
-----------------------------------------------------------
On Jan. 27, 2016, 9:10 a.m., M Lawindi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42016/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2016, 9:10 a.m.)
>
>
> Review request for 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
> src/slave/cmake/SlaveConfigure.cmake cf378a27297474b2a9f338e0c832612370f7302a
>
> Diff: https://reviews.apache.org/r/42016/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> M Lawindi
>
>
Re: Review Request 42016: Windows: Apply patch.exe without elevation
prompt
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 42016: Windows: Apply patch.exe without elevation
prompt
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42016/#review116146
-----------------------------------------------------------
Bad patch!
Reviews applied: [42018]
Failed command: ./support/apply-review.sh -n -r 42018
Error:
...<truncated>...
2016-01-25 19:14:49 URL:https://reviews.apache.org/r/42018/diff/raw/ [32008/32008] -> "42018.patch" [1]
42018.patch:210: trailing whitespace.
42018.patch:214: trailing whitespace.
42018.patch:219: trailing whitespace.
42018.patch:223: trailing whitespace.
42018.patch:226: trailing whitespace.
warning: squelched 10 whitespace errors
warning: 15 lines add whitespace errors.
3rdparty/zookeeper-3.4.5.patch:202: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:206: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:211: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:215: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:218: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:229: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:231: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:235: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:451: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:462: trailing whitespace.
+
3rdparty/zookeeper-3.4.5.patch:481: trailing whitespace.
+- lock xadd dword ptr [eax], ecx;
3rdparty/zookeeper-3.4.5.patch:482: trailing whitespace.
+- lock xadd dword ptr [eax], ebx;
3rdparty/zookeeper-3.4.5.patch:483: trailing whitespace.
+- mov result, ecx; // result = ebx;
3rdparty/zookeeper-3.4.5.patch:485: trailing whitespace.
+- return result;
3rdparty/zookeeper-3.4.5.patch:489: trailing whitespace.
+
Full log: https://builds.apache.org/job/mesos-reviewbot/11034/console
- Mesos ReviewBot
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
>
>
Re: Review Request 42016: Windows: Apply patch.exe without elevation
prompt
Posted by Yi Sun <yi...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42016/#review116126
-----------------------------------------------------------
Ship it!
Ship It!
- Yi Sun
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
>
>