> On Sept. 22, 2015, 6:32 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/cmake/ProcessConfigure.cmake, lines 59-65
> > <https://reviews.apache.org/r/38542/diff/1/?file=1077780#file1077780line59>
> >
> >     What do you think about moving this `if` block into `Versions.cmake`?
> >     
> >     Pro: Versions are kept in one place.  If we unilaterally update to glog 
> > 0.3.4, we'll only need to update it in one place.
> >     
> >     Con: Logic in the version file, which we don't really have a precedent 
> > for.

I think it would be best to keep it in `ProcessConfigure.cmake`. The reason is 
that then anyone who uses libprocess need only `include(ProcessConfigure)` and 
the third-party build logic is generated seemlessly for them. If you put this 
in `Versions.cmake`, then you'd have to `include` both to fully configure and 
use libprocess, and you'd have to move `Versions.cmake` back into the 
`libprocess` directory.

Besides that, we're going to upgrade glog to 0.3.5 at some point anyway, and in 
that case, the conditional will disappear, and it will stop making much sense 
to have it in `Versions.cmake`.


- Alex


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


On Sept. 21, 2015, 3:23 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38542/
> -----------------------------------------------------------
> 
> (Updated Sept. 21, 2015, 3:23 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Currently we configure the version information of third-party
> dependencies in the CMake build system from magic strings.
> 
> This commit will transition away from the magic string solution and
> towards the variables we define in `Versions.cmake`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> a5f8d399e151acad87bb72ecb1f7372b2c467423 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 
> 
> Diff: https://reviews.apache.org/r/38542/diff/
> 
> 
> Testing
> -------
> 
> Compiled and ran made sure libprocess and stout tests ran and passed on the 
> following platforms:
> 
> * OS X 10.10
> * Windows 10
> * Ubuntu 14.04.2
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Reply via email to