Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1571883002:
initial sphinx doc production patch for subzero (Closed)
|
Created:
4 years, 11 months ago by rkotlerimgtec Modified:
4 years, 11 months ago CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionSubzero: Use sphinx to generate production-quality docs.
BUG=
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=f96190eac95919eca82c9e0a16213418f934912e
Patch Set 1 #
Total comments: 12
Patch Set 2 : changes suggested by stichnot #Patch Set 3 : forgot to save last change #
Total comments: 4
Patch Set 4 : changes suggested by stichnot #
Total comments: 6
Patch Set 5 : changes suggested by stichnot #
Messages
Total messages: 16 (5 generated)
Description was changed from ========== initial sphinx checkin BUG= ========== to ========== initial sphinx checkin BUG= ==========
rkotlerimgtec@gmail.com changed reviewers: + jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org
This is not necessarily ready for checkin. It's a basic port of the LLVM build for the .rst docs in order to produce a project doc set. You could choose to check this in and we could start to work on it from this first version or you can make comments and I will start to work on this in the background with other real codegen work I have to do. LLVM uses the sphinx-build utility which on my machine was already installed by default. The main files you need to add are index.rst and conf.py and I have created a nominal index.rst and a quick and dirty port of conf.py. You can see these same files in the llvm/docs directory to see what they do. The way you make the doc set is to go to the docs directory and type: make -f Makefile.sphinx html Though you can make other forms too but I have not tried them to see if that works. For a few hours of work on my part for this, the doc you get actually looks pretty decent although it's not production quality at all. The doc root is in the build directory in build/sphinx/html/index.html Enjoy!
This is really cool! https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx File docs/Makefile.sphinx (right): https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode1 docs/Makefile.sphinx:1: # Makefile for Sphinx documentation Make document that this file originated from some particular LLVM file? https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode6 docs/Makefile.sphinx:6: SPHINXBUILD = mkdir -p $(BUILDDIR) && sphinx-build I would document somewhere (maybe at the very top) that the sphinx-build package is required, e.g. apt-get install python-sphinx https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode17 docs/Makefile.sphinx:17: .PHONY: help clean html dirhtml singlehtml pickle json htmlhelp qthelp devhelp epub latex latexpdf text man changes linkcheck doctest gettext There are a bunch of 80-col issues in this file, thanks most likely to tab characters. https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode74 docs/Makefile.sphinx:74: @echo "Build finished; now you can run HTML Help Workshop with the" \ I think it might be better to @echo multiple lines instead of using the \ continuation. https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode80 docs/Makefile.sphinx:80: @echo "Build finished; now you can run "qcollectiongenerator" with the" \ Quotes inside quotes probably aren't doing what you expect... You probably want something similar to what the "latex" target does. https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcod... docs/Makefile.sphinx:149: @echo "Link check complete; look for any errors in the above output " \ My OCD points out that some of these continued strings have a trailing space before the end quote, and some don't. Note that if you expand it out, something like: @echo "abc" "def" will print: abc def so you probably don't want the trailing space.
https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx File docs/Makefile.sphinx (right): https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode1 docs/Makefile.sphinx:1: # Makefile for Sphinx documentation On 2016/01/20 19:00:36, stichnot wrote: > Make document that this file originated from some particular LLVM file? Done. https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode6 docs/Makefile.sphinx:6: SPHINXBUILD = mkdir -p $(BUILDDIR) && sphinx-build On 2016/01/20 19:00:37, stichnot wrote: > I would document somewhere (maybe at the very top) that the sphinx-build package > is required, e.g. > apt-get install python-sphinx Comment put in. comes preinstalled on Ubuntu. We just need sphinx-build. https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode17 docs/Makefile.sphinx:17: .PHONY: help clean html dirhtml singlehtml pickle json htmlhelp qthelp devhelp epub latex latexpdf text man changes linkcheck doctest gettext On 2016/01/20 19:00:37, stichnot wrote: > There are a bunch of 80-col issues in this file, thanks most likely to tab > characters. Done. https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode74 docs/Makefile.sphinx:74: @echo "Build finished; now you can run HTML Help Workshop with the" \ On 2016/01/20 19:00:37, stichnot wrote: > I think it might be better to @echo multiple lines instead of using the \ > continuation. Done. https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode80 docs/Makefile.sphinx:80: @echo "Build finished; now you can run "qcollectiongenerator" with the" \ On 2016/01/20 19:00:37, stichnot wrote: > Quotes inside quotes probably aren't doing what you expect... You probably want > something similar to what the "latex" target does. Done. https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcod... docs/Makefile.sphinx:149: @echo "Link check complete; look for any errors in the above output " \ On 2016/01/20 19:00:36, stichnot wrote: > My OCD points out that some of these continued strings have a trailing space > before the end quote, and some don't. Note that if you expand it out, something > like: > @echo "abc" "def" > will print: > abc def > so you probably don't want the trailing space. Done.
I think I forgot to save my last change. Will resubmit. On Wed, Jan 20, 2016 at 2:50 PM, <rkotlerimgtec@gmail.com> wrote: > > https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx > File docs/Makefile.sphinx (right): > > > https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode1 > docs/Makefile.sphinx:1: # Makefile for Sphinx documentation > On 2016/01/20 19:00:36, stichnot wrote: > >> Make document that this file originated from some particular LLVM >> > file? > > Done. > > > https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode6 > docs/Makefile.sphinx:6: SPHINXBUILD = mkdir -p $(BUILDDIR) && > sphinx-build > On 2016/01/20 19:00:37, stichnot wrote: > >> I would document somewhere (maybe at the very top) that the >> > sphinx-build package > >> is required, e.g. >> apt-get install python-sphinx >> > > Comment put in. comes preinstalled on Ubuntu. We just need sphinx-build. > > > https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode17 > docs/Makefile.sphinx:17: .PHONY: help clean html dirhtml singlehtml > pickle json htmlhelp qthelp devhelp epub latex latexpdf text man changes > linkcheck doctest gettext > On 2016/01/20 19:00:37, stichnot wrote: > >> There are a bunch of 80-col issues in this file, thanks most likely to >> > tab > >> characters. >> > > Done. > > > https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode74 > docs/Makefile.sphinx:74: @echo "Build finished; now you can run HTML > Help Workshop with the" \ > On 2016/01/20 19:00:37, stichnot wrote: > >> I think it might be better to @echo multiple lines instead of using >> > the \ > >> continuation. >> > > Done. > > > https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcode80 > docs/Makefile.sphinx:80: @echo "Build finished; now you can run > "qcollectiongenerator" with the" \ > On 2016/01/20 19:00:37, stichnot wrote: > >> Quotes inside quotes probably aren't doing what you expect... You >> > probably want > >> something similar to what the "latex" target does. >> > > Done. > > > https://codereview.chromium.org/1571883002/diff/1/docs/Makefile.sphinx#newcod... > docs/Makefile.sphinx:149: @echo "Link check complete; look for any > errors in the above output " \ > On 2016/01/20 19:00:36, stichnot wrote: > >> My OCD points out that some of these continued strings have a trailing >> > space > >> before the end quote, and some don't. Note that if you expand it out, >> > something > >> like: >> @echo "abc" "def" >> will print: >> abc def >> so you probably don't want the trailing space. >> > > Done. > > https://codereview.chromium.org/1571883002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
Also make the first Description line conform to normal git standards. https://codereview.chromium.org/1571883002/diff/40001/docs/Makefile.sphinx File docs/Makefile.sphinx (right): https://codereview.chromium.org/1571883002/diff/40001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:3: # Requies sphinx-build which comes preinstalled on Ubuntu Requires FWIW, sphinx-build was not preinstalled on my work Ubuntu system, nor my home 14.04 system, so I would remove the assertion that it is preinstalled on Ubuntu. Suggestion: # Requires sphinx-build (e.g., "apt-get install python-sphinx") https://codereview.chromium.org/1571883002/diff/40001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:34: @echo " latex to make LaTeX files, you can set PAPER=a4 or PAPER=letter" Please fix the 80-col issues in lines 34, 41, 43. Remember that a tab character counts for up to 8 columns.
https://codereview.chromium.org/1571883002/diff/40001/docs/Makefile.sphinx File docs/Makefile.sphinx (right): https://codereview.chromium.org/1571883002/diff/40001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:3: # Requies sphinx-build which comes preinstalled on Ubuntu On 2016/01/21 01:36:09, stichnot wrote: > Requires > > FWIW, sphinx-build was not preinstalled on my work Ubuntu system, nor my home > 14.04 system, so I would remove the assertion that it is preinstalled on Ubuntu. > Suggestion: > > # Requires sphinx-build (e.g., "apt-get install python-sphinx") Done. https://codereview.chromium.org/1571883002/diff/40001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:34: @echo " latex to make LaTeX files, you can set PAPER=a4 or PAPER=letter" On 2016/01/21 01:36:09, stichnot wrote: > Please fix the 80-col issues in lines 34, 41, 43. Remember that a tab character > counts for up to 8 columns. Done.
https://codereview.chromium.org/1571883002/diff/60001/docs/Makefile.sphinx File docs/Makefile.sphinx (right): https://codereview.chromium.org/1571883002/diff/60001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:3: # Requies sphinx-build which may come preinstalled on your system. Requires https://codereview.chromium.org/1571883002/diff/60001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:38: @echo " latex to make LaTeX files, you can set PAPER=a4 or " Might as well remove the trailing space before the closing quote mark. https://codereview.chromium.org/1571883002/diff/60001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:46: @echo " changes to make an overview of all " ditto
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1571883002/diff/60001/docs/Makefile.sphinx File docs/Makefile.sphinx (right): https://codereview.chromium.org/1571883002/diff/60001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:3: # Requies sphinx-build which may come preinstalled on your system. On 2016/01/22 03:11:21, stichnot wrote: > Requires Done. https://codereview.chromium.org/1571883002/diff/60001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:38: @echo " latex to make LaTeX files, you can set PAPER=a4 or " On 2016/01/22 03:11:21, stichnot wrote: > Might as well remove the trailing space before the closing quote mark. Done. https://codereview.chromium.org/1571883002/diff/60001/docs/Makefile.sphinx#ne... docs/Makefile.sphinx:46: @echo " changes to make an overview of all " On 2016/01/22 03:11:21, stichnot wrote: > ditto Done.
lgtm
Description was changed from ========== initial sphinx checkin BUG= ========== to ========== Subzero: Use sphinx to generate production-quality docs. BUG= ==========
Description was changed from ========== Subzero: Use sphinx to generate production-quality docs. BUG= ========== to ========== Subzero: Use sphinx to generate production-quality docs. BUG= R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) manually as f96190eac95919eca82c9e0a16213418f934912e (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
