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



src/slave/CMakeLists.txt (lines 17 - 19)
<https://reviews.apache.org/r/41092/#comment169109>

    Historically, we have put the `*_TARGET` inside the relevant 
`*Configure.cmake` files. I recommend we follow this convention and put this 
target inside the `SlaveConfigure.cmake`.
    
    Also, what do you think of calling it `AGENT_TARGET` instead of 
`AGENT_EXEC_TARGET`? I think the former is a lot clearer, personally, 
especially since we have exec directories in the project.



src/slave/CMakeLists.txt (line 21)
<https://reviews.apache.org/r/41092/#comment169121>

    Possibly there should be a space between `#` and `SOURCE`?



src/slave/CMakeLists.txt (line 22)
<https://reviews.apache.org/r/41092/#comment169122>

    Can we make the end of the line with `###` line up with the comment?



src/slave/CMakeLists.txt (line 24)
<https://reviews.apache.org/r/41092/#comment169108>

    In the `CMakeLists.txt` that already exists, we tend to refer to these as 
`AGENT_`, for example in `src/CMakeLists.txt`, we call it `AGENT_SRC`. I'd like 
to propose we use a similar naming scheme here.



src/slave/CMakeLists.txt (line 26)
<https://reviews.apache.org/r/41092/#comment169124>

    Alright, just to be clear: this will break the build, which is why we 
didn't `add_subdirectory(slave)`, right?
    
    That's fine, I just wanted to call this out on the review.



src/slave/CMakeLists.txt (line 30)
<https://reviews.apache.org/r/41092/#comment169123>

    My guess is that this is left over from the `CMakeLists.txt` in process? If 
so it seems like the `PROCESS` word on this line snuck in accidentally. :)



src/slave/CMakeLists.txt (line 32)
<https://reviews.apache.org/r/41092/#comment169117>

    My recollection is that, when called with an empty source list, 
`add_executable` will result in an error that claims you didn't pass it enough 
arguments.
    
    Therefore, I recommend that we actually just move the `if (NOT WIN32)` 
conditionals to just not `add_subdirectory` on the `slave/` directory.



src/slave/CMakeLists.txt (line 37)
<https://reviews.apache.org/r/41092/#comment169119>

    It seems like these should possibly be in alphabetical order?



src/slave/CMakeLists.txt (line 39)
<https://reviews.apache.org/r/41092/#comment169120>

    Can we remove the trailing whitespace? And add a period?


- Alex Clemmer


On Dec. 9, 2015, 3:43 a.m., Diana Arroyo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41092/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2015, 3:43 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
>     https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added CMake file for agent executable build.
> 
> 
> Diffs
> -----
> 
>   src/slave/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41092/diff/
> 
> 
> Testing
> -------
> 
> Tested on Ubuntu and OSX.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Reply via email to