Re: Review Request 44001: CMake: Add MasterConfigure for master executable build.

[Joris Van Remoortere]

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On Feb. 25, 2016, 8:53 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44001/
> ---
> 
> (Updated Feb. 25, 2016, 8:53 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-4773
> https://issues.apache.org/jira/browse/MESOS-4773
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Add MasterConfigure for master executable build.
> 
> 
> Diffs
> -
> 
>   src/master/cmake/MasterConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44001/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 44001: CMake: Add MasterConfigure for master executable build.

[Alex Clemmer]

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




src/master/cmake/MasterConfigure.cmake (line 49)


Which of these do we actually need for the master? It looks like we're 
copying this directly from the stuff in `SlaveConfigure.cmake`, is that right?

If so, two notes:

(1) The `AGENT_INCLUDE_DIRS`, `AGENT_DEPENDENCIES`, and so on, are actually 
misnomers -- it should really be called `MESOS_INCLUDE_DIRS`, _etc_., because 
it is building libmesos, not the slave.
(2) Therefore, I recommend renaming those variables to be `MESOS_*` instead 
of `AGENT_*`, and then just passing the `MESOS_INCLUDE_DIRS` into the 
`MASTER_INCLUDE_DIRS` instead of copying them everywhere. Possibly this 
involves moving those definitions to `MesosConfigure.cmake` instead, as the 
libmesos configuration really doesn't belong in `SlaveConfigure.cmake`.

My rationale for point 2 here, is that we are just copying around the smae 
include paths in a bunch of places now, and this is bad. Every time we add a 
directory, we'll need to add it to two places. Why not just add it to the 
libmesos configuration once, and have both other things consume that 
configuration instead?

Thoughts?


- Alex Clemmer


On Feb. 25, 2016, 8:53 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44001/
> ---
> 
> (Updated Feb. 25, 2016, 8:53 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-4773
> https://issues.apache.org/jira/browse/MESOS-4773
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Add MasterConfigure for master executable build.
> 
> 
> Diffs
> -
> 
>   src/master/cmake/MasterConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44001/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 44001: CMake: Add MasterConfigure for master executable build.

[Diana Arroyo]

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

(Updated Feb. 25, 2016, 8:53 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


Changes
---

Added check to call FindApr and FindSvn in non-windows build environment.


Bugs: MESOS-4773
https://issues.apache.org/jira/browse/MESOS-4773


Repository: mesos


Description
---

CMake: Add MasterConfigure for master executable build.


Diffs (updated)
-

  src/master/cmake/MasterConfigure.cmake PRE-CREATION 

Diff: https://reviews.apache.org/r/44001/diff/


Testing
---

Tested on Ubuntu.


Thanks,

Diana Arroyo

Re: Review Request 44001: CMake: Add MasterConfigure for master executable build.

[Diana Arroyo]

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

(Updated Feb. 25, 2016, 8:13 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


Changes
---

Added a check for non-windows before making the call to FindApr and FindSvn.


Bugs: MESOS-4773
https://issues.apache.org/jira/browse/MESOS-4773


Repository: mesos


Description
---

CMake: Add MasterConfigure for master executable build.


Diffs (updated)
-

  src/master/cmake/MasterConfigure.cmake PRE-CREATION 

Diff: https://reviews.apache.org/r/44001/diff/


Testing
---

Tested on Ubuntu.


Thanks,

Diana Arroyo