Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updated documentation build instructions #2648

Merged
merged 10 commits into from
Jun 28, 2016
Merged

Updated documentation build instructions #2648

merged 10 commits into from
Jun 28, 2016

Conversation

liangfok
Copy link
Contributor

@liangfok liangfok commented Jun 25, 2016 •
edited by david-german-tri
Loading

Updates documentation build instructions support ninja / out-of-source build tool / style.

Closes #2641.

This change is Reviewable

@liangfok liangfok self-assigned this Jun 25, 2016
@liangfok
Copy link
Contributor Author

Since the next business day is Monday and this is a documentation-only change, +@sherm1 for both levels of review.

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Jun 27, 2016

One review pass done. I did not try to generate the documentation yet.

Reviewed 2 of 4 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

drake/doc/documentation_instructions.rst, line 4 [r2] (raw file):

*************************************
Documentation Generation Instructions

These instructions seem to depend on some previous actions having been completed, such as a successful superbuild step. Does this appear in a context where that has already been explained? It would be good to mention here what assumptions are being made, and ideally provide a link to the instructions that explain what has to be done before this step.

drake/doc/documentation_instructions.rst, line 18 [r2] (raw file):

first be enabled prior to documentation generation. This process depends
on whether Drake is being built in-source our out-of-source, and which build
system is being used, e.g., ``make``, ``ninja``, or ``MSVC``. The following

The Windows instructions appear to assume Visual Studio. If that's correct, it would be better to say Visual Studio here rather than MSVC since CMake doesn't provide an MSVC Generator.

drake/doc/documentation_instructions.rst, line 20 [r2] (raw file):

system is being used, e.g., ``make``, ``ninja``, or ``MSVC``. The following
subsections describe the process for the officially-supported configurations.
Please jump to the subsection that matches the build style and tool employed.

Can you provide links to click on here to facilitate that jumping?

drake/doc/documentation_instructions.rst, line 74 [r2] (raw file):

.. _documentation-out-of-source-msvc:

Out-of-source Builds Using ``MSVC``

MSVC->Visual Studio?

drake/doc/documentation_instructions.rst, line 80 [r2] (raw file):

    $ cd drake-build/drake
    $ cmake -DBUILD_DOCUMENTATION=ON .

Why $ here? Are you assuming a bash shell? If this is intended to be used with Command Prompt then using > would be better.

drake/doc/documentation_instructions.rst, line 80 [r2] (raw file):

    $ cd drake-build/drake
    $ cmake -DBUILD_DOCUMENTATION=ON .

Currently I think this also requires -DDISABLE_MATLAB=ON (for those unfortunates like me who have Matlab installed but don't want to use it or have no license); see #2650. I suspect that is true on non-Windows platforms also.

I see from your instruction here that I may have been doing the wrong thing: I have been running the CMake GUI in the source drake-distro/drake directory rather than the build/drake binary directory. Should that work?

drake/doc/documentation_instructions.rst, line 113 [r2] (raw file):

- Drake website: ``drake-distro/drake/pod-build/doc/sphinx/index.html``
- Doxygen C++ website: ``drake-distro/drake/pod-build/doc/doxygen_cxx/html/index.html``
- Doxygen Matlab website: ``drake-distro/drake/pod-build/doc/doxygen_matlab/html/index.html``

no newline at eof

Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Jun 27, 2016

@liangfok, please see issue #2650 and PR #2655 -- that PR should change these instructions (for the better!).

Review status: all files reviewed at latest revision, 7 unresolved discussions.

Comments from Reviewable

@liangfok
Copy link
Contributor Author

OK I will wait for #2655 to be merged before updating this PR.

Review status: all files reviewed at latest revision, 7 unresolved discussions.

Comments from Reviewable

@david-german-tri
Copy link
Contributor

FYI, #2655 has been merged.

@liangfok
Copy link
Contributor Author

Thanks for the note. Updating this PR now.

Chien-Liang Fok added 2 commits June 27, 2016 16:57
Merge branch 'master' into feature/updated_sphinx_doc
eff66ff 
Conflicts:
	drake/doc/doxygen_instructions.rst
Updated documentation instructions given the fact that the documentat…
33841e1 
…ion build target is now on by default.
@liangfok
Copy link
Contributor Author

I addressed all reviewer comments. This is ready for another round of review.

Review status: 3 of 5 files reviewed at latest revision, 7 unresolved discussions.

drake/doc/documentation_instructions.rst, line 4 [r2] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

These instructions seem to depend on some previous actions having been completed, such as a successful superbuild step. Does this appear in a context where that has already been explained? It would be good to mention here what assumptions are being made, and ideally provide a link to the instructions that explain what has to be done before this step.

Done. I added a sentence at the very beginning stating that Drake must first be built from source. This statement includes a link to the build instructions.

drake/doc/documentation_instructions.rst, line 18 [r2] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

The Windows instructions appear to assume Visual Studio. If that's correct, it would be better to say Visual Studio here rather than MSVC since CMake doesn't provide an MSVC Generator.

Done.

drake/doc/documentation_instructions.rst, line 20 [r2] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Can you provide links to click on here to facilitate that jumping?

Yes. Done.

drake/doc/documentation_instructions.rst, line 74 [r2] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

MSVC->Visual Studio?

Done.

drake/doc/documentation_instructions.rst, line 80 [r2] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Why $ here? Are you assuming a bash shell? If this is intended to be used with Command Prompt then using > would be better.

Yes, I was using a bash shell within Windows. Note that in the [officially documented instructions](http://drake.mit.edu/windows.html#compile-drake), it specifies the use of a bash shell.

Given this, do you still want to change $ to be >?

drake/doc/documentation_instructions.rst, line 80 [r2] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Currently I think this also requires -DDISABLE_MATLAB=ON (for those unfortunates like me who have Matlab installed but don't want to use it or have no license); see #2650. I suspect that is true on non-Windows platforms also.

I see from your instruction here that I may have been doing the wrong thing: I have been running the CMake GUI in the source drake-distro/drake directory rather than the build/drake binary directory. Should that work?

I believe this was address by #2655. There's no longer any need to enable documentation since it's on by default. I've updated this PR to reflect this.

drake/doc/documentation_instructions.rst, line 113 [r2] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

no newline at eof

Fixed. Thanks.

Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Jun 27, 2016

A few more comments.

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/doc/documentation_instructions.rst, line 20 [r2] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Yes. Done.

Nice! But when I view this it has four entries -- the last is "view the generated documentation" which doesn't match the "please jump" instructions above.

drake/doc/documentation_instructions.rst, line 80 [r2] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Yes, I was using a bash shell within Windows. Note that in the officially documented instructions, it specifies the use of a bash shell.

Given this, do you still want to change $ to be >?

OK, `$` makes sense if they are supposed to be using the bash shell.

drake/doc/documentation_instructions.rst, line 21 [r3] (raw file):

Drake's documentation is built using the ``documentation`` build target. It must
first be enabled prior to documentation generation. This process depends

I'm not sure this is accurate any more since the target is now built by default.

Comments from Reviewable

@liangfok
Copy link
Contributor Author

Ready for another round of review.

Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions.

drake/doc/documentation_instructions.rst, line 20 [r2] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Nice! But when I view this it has four entries -- the last is "view the generated documentation" which doesn't match the "please jump" instructions above.

Fixed.

drake/doc/documentation_instructions.rst, line 21 [r3] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

I'm not sure this is accurate any more since the target is now built by default.

You're right. Fixed.

Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Jun 28, 2016

:lgtm: -- there is one missing newline. Resolve & merge.

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

drake/doc/documentation_instructions.rst, line 20 [r2] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Fixed.

LGTM now.

drake/doc/documentation_instructions.rst, line 87 [r4] (raw file):

- Drake website: ``drake-build/drake/doc/sphinx/index.html``
- Doxygen C++ website: ``drake-build/drake/doc/doxygen_cxx/html/index.html``
- Doxygen Matlab website: ``drake-build/drake/doc/doxygen_matlab/html/index.html``

Looks like the trailing newline got lost.

Comments from Reviewable

@liangfok
Copy link
Contributor Author

I've added the missing new line. Waiting for CI now.

Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.

drake/doc/documentation_instructions.rst, line 87 [r4] (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Looks like the trailing newline got lost.

Done. PTAL. Thanks!

Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Jun 28, 2016

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Jun 28, 2016

Windows builds seem stuck but inspection shows they completed successfully so I'm going to force-merge this.

@sherm1 sherm1 merged commit 7e4dd10 into RobotLocomotion:master Jun 28, 2016
@jwnimmer-tri
Copy link
Collaborator

For the record:

Given that documentation is only tested by the linux target of pre-merge CI, though, ignoring windows for the purposes of merging is just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liangfok liangfok

@sherm1 sherm1

Labels
priority: medium type: documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liangfok @sherm1 @david-german-tri @jwnimmer-tri