You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2017/10/02 18:55:12 UTC
Review Request 62732: Added CMake documentation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62732/
-----------------------------------------------------------
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
Re: Review Request 62732: Added CMake documentation.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> 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
>
>
Re: Review Request 62732: Added CMake documentation.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> 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.
>
> Andrew Schwartzmeyer wrote:
> 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.
Verified that this is no problem with the generated site.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62732/#review186899
-----------------------------------------------------------
On Oct. 5, 2017, noon, Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62732/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2017, noon)
>
>
> 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 1b10221429a92eddff5733e1059172b6c190b5f3
> docs/configuration.md e1fd9f75179b272c3cae3dd5be5e38f269044df5
> docs/home.md f9b35e3e8f9af024a58760c345931d73a83654ff
>
>
> Diff: https://reviews.apache.org/r/62732/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 62732: Added CMake documentation.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62732/#review186899
-----------------------------------------------------------
Partial skim-review-thing...
docs/cmake-examples.md
Lines 268 (patched)
<https://reviews.apache.org/r/62732/#comment263784>
This should have been caught by the linter.
docs/cmake.md
Lines 42 (patched)
<https://reviews.apache.org/r/62732/#comment263774>
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.
docs/cmake.md
Lines 50 (patched)
<https://reviews.apache.org/r/62732/#comment263775>
Ditto here.
docs/cmake.md
Lines 59-61 (patched)
<https://reviews.apache.org/r/62732/#comment263777>
Perhaps drop a link to https://cmake.org/cmake/help/v3.7/command/if.html
docs/cmake.md
Lines 104-105 (patched)
<https://reviews.apache.org/r/62732/#comment263782>
Odd wrapping here. Start the second line with the `e.g. if (...`
docs/cmake.md
Lines 188-192 (patched)
<https://reviews.apache.org/r/62732/#comment263783>
`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 ;)
- Joseph Wu
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
>
>
Re: Review Request 62732: Added CMake documentation.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62732/#review187021
-----------------------------------------------------------
TODO: Add link to this to the general "Documentation" page. It's currently hard to find in the generated site.
- Andrew Schwartzmeyer
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
>
>
Re: Review Request 62732: Added CMake documentation.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62732/#review187673
-----------------------------------------------------------
Ship it!
Going to make some tweaks (rewording a couple sentences) here and there before committing, but mostly LGTM.
docs/cmake-examples.md
Lines 36-41 (patched)
<https://reviews.apache.org/r/62732/#comment264733>
Going to replace this TODO with a link:
https://cmake.org/cmake/help/latest/module/ExternalProject.html
docs/cmake.md
Lines 76 (patched)
<https://reviews.apache.org/r/62732/#comment264726>
I'll make these links:
* https://cmake.org/cmake/help/latest/policy/CMP0012.html
* https://cmake.org/cmake/help/latest/policy/CMP0054.html
docs/cmake.md
Lines 119 (patched)
<https://reviews.apache.org/r/62732/#comment264728>
I feel this section deserves to be placed right after installation (and before discussion of mistakes or style).
docs/cmake.md
Lines 191-196 (patched)
<https://reviews.apache.org/r/62732/#comment264730>
This could be a numbered list.
docs/cmake.md
Lines 204-210 (patched)
<https://reviews.apache.org/r/62732/#comment264732>
Going to insert a link to: https://cmake.org/cmake/help/latest/command/link_directories.html
- Joseph Wu
On Oct. 5, 2017, noon, Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62732/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2017, noon)
>
>
> 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 1b10221429a92eddff5733e1059172b6c190b5f3
> docs/configuration.md e1fd9f75179b272c3cae3dd5be5e38f269044df5
> docs/home.md f9b35e3e8f9af024a58760c345931d73a83654ff
>
>
> Diff: https://reviews.apache.org/r/62732/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 62732: Added CMake documentation.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62732/
-----------------------------------------------------------
(Updated Oct. 5, 2017, noon)
Review request for mesos, Benjamin Bannier, Jeff Coffler, Greg Mann, John Kordich, James Peach, Joseph Wu, and Li Li.
Changes
-------
Addressed comments and added step-by-step for Java and SSL on Windows.
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 (updated)
-----
docs/cmake-examples.md PRE-CREATION
docs/cmake.md PRE-CREATION
docs/configuration-cmake.md 1b10221429a92eddff5733e1059172b6c190b5f3
docs/configuration.md e1fd9f75179b272c3cae3dd5be5e38f269044df5
docs/home.md f9b35e3e8f9af024a58760c345931d73a83654ff
Diff: https://reviews.apache.org/r/62732/diff/2/
Changes: https://reviews.apache.org/r/62732/diff/1-2/
Testing
-------
Thanks,
Andrew Schwartzmeyer