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 41092: Added CMake file for agent executable build.
Date Wed, 09 Dec 2015 17:29:57 GMT

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


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