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