mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 62732: Added CMake documentation.
Date Tue, 03 Oct 2017 21:06:23 GMT


> On Oct. 2, 2017, 3:26 p.m., Joseph Wu wrote:
> > docs/cmake-examples.md
> > Lines 268 (patched)
> > <https://reviews.apache.org/r/62732/diff/1/?file=1842222#file1842222line268>
> >
> >     This should have been caught by the linter.

Yeah... it should have, especially considering I wrote this on Linux (and therefore with all
the hooks enabled and working... weird).


> On Oct. 2, 2017, 3:26 p.m., Joseph Wu wrote:
> > docs/cmake.md
> > Lines 42 (patched)
> > <https://reviews.apache.org/r/62732/diff/1/?file=1842223#file1842223line42>
> >
> >     Hm... The website's markdown generator is not super sophisticated, so I'm not
sure if it will render this link correctly.
> >     
> >     It's ok to go over 80 characters on a line if it's just a URL.

Ha! It better work, this is in Daring Fireball's original Markdown spec (i.e. it's not any
"flavor's" extension). I'll bug Vinod and make sure, but it really ought to work.

My problem with embedding URLs in the text is just that, it makes the plaintext so much more
difficult to read. But maybe that's just me.


> On Oct. 2, 2017, 3:26 p.m., Joseph Wu wrote:
> > docs/cmake.md
> > Lines 59-61 (patched)
> > <https://reviews.apache.org/r/62732/diff/1/?file=1842223#file1842223line59>
> >
> >     Perhaps drop a link to https://cmake.org/cmake/help/v3.7/command/if.html

Ah, totally agree. Thanks.


> On Oct. 2, 2017, 3:26 p.m., Joseph Wu wrote:
> > docs/cmake.md
> > Lines 104-105 (patched)
> > <https://reviews.apache.org/r/62732/diff/1/?file=1842223#file1842223line104>
> >
> >     Odd wrapping here.  Start the second line with the `e.g. if (...`

I just use auto-wrap... I'll rewrap it manually.


> On Oct. 2, 2017, 3:26 p.m., Joseph Wu wrote:
> > docs/cmake.md
> > Lines 188-192 (patched)
> > <https://reviews.apache.org/r/62732/diff/1/?file=1842223#file1842223line188>
> >
> >     `add_dependencies` isn't actually that rare.  We almost always need it whenever
we have 2+ targets that are not libraries.
> >     
> >     Say if you have the `mesos-agent` binary and the `mesos-containerizer` helper
binary.  Both depend on the `mesos` library to build, but `mesos-agent` depends on `mesos-containerizer`
at runtime.
> >     
> >     (This, BTW, is currently a bug, as we do _not_ have this dependency in place
;)

I should say not that it's rare but that you should be _extra careful_ when using it, as often
it is superfluous in CMake. Dependent executables are probably exception (3).

I think I have an open bug to clean up the executable dependencies (rather than relying on
the all target and `mesos-tests`.


- Andrew


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


On Oct. 2, 2017, 11:55 a.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62732/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2017, 11:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jeff Coffler, Greg Mann, John Kordich, James
Peach, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-3107
>     https://issues.apache.org/jira/browse/MESOS-3107
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This adds a CMake documentation file with best practices, CMake By
> Example, and consolidates, fixes, and updates the CMake configuration
> options to the configuration documentation.
> 
> 
> Diffs
> -----
> 
>   docs/cmake-examples.md PRE-CREATION 
>   docs/cmake.md PRE-CREATION 
>   docs/configuration-cmake.md d2eb571c9458c7fade415e8077c676dd34e63242 
>   docs/configuration.md e1fd9f75179b272c3cae3dd5be5e38f269044df5 
> 
> 
> Diff: https://reviews.apache.org/r/62732/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


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