Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Switch to makefile-based doc build.#9513
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This seems to work on my machine. I checked the options that were documented, and they seemed to work. I also checked the built docs and they look fine, though of course I didn't go down each tree.
doc/README.txt Outdated
| Matplotlib documentation | ||
| ======================== | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This seems to duplicate documenting_mpl.rst Can all of this just be removed and replaced by reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yup, fixed
doc/devel/documenting_mpl.rst Outdated
| .. note:: | ||
| * You'll need a minimal working LaTeX distribution for many examples to run. | ||
| * `Graphviz <http://www.graphviz.org/Download.php>`_ is not a python package, and needs | ||
| * `Graphviz <http://www.graphviz.org/Download.php>`_ is not a python package, and needs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unnecessary change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed, dunno how the spare space got there
doc/devel/release_guide.rst Outdated
| @@ -54,7 +54,7 @@ temporarily comment out the include and toctree glob; re-instate these after a | |||
| release. Finally, make sure that the docs build cleanly :: | |||
| pushd doc | |||
| pythonmake.py htmllatex -n 16 | |||
| make htmllatexpdf -n$(nproc) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
O=-n$(nproc)? Currently, this will buildhtml andlatexpdf in parallel, but I'm not sure Sphinx will like two runs occurring in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ooops, I didn't check this one. Good eye: that doesn't run at all on my machine.
Glad someone more knowledgeable is reviewing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think you only fixed the one below that didn't have this comment. 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
try again...
doc/make.bat Outdated
| goto end | ||
| :help | ||
| %SPHINXBUILD% -M help %SOURCEDIR% %BUILDDIR% %SPHINXOPTS% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't you need to check for sphinx build before running this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should be reported to upstream but I fixed it locally.
(can you do it?)
Comments addressed in force-pushed commit. Thanks for the review. |
7e8ccbc to3438f84Comparerebased after the merge of#9311 |
doc/Makefile Outdated
| # | ||
| # You can set these variables from the command line. | ||
| SPHINXOPTS = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we default to-W?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would much prefer not: when building locally I can easily check from the log whether the build failed, but I'd rather not have the build (which can be quite lengthy) fail halfway, fix one error, restart a build, wait another 5min, fix the next error, etc. (WellI know --allowsphinxwarnings, or, on the new system, how to override O=; but I think it's not the most new-contributor friendly way to do it...)
(xrefsphinx-doc/sphinx#3627)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The CI runs with it so new contributors are going to have to deal with the failures one way or the other and getting them locally by default seems nicer than having to go through CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Warnings are perfectly well printed without -W as well, the only effects of -W are that 1) the build stops after the first failure (not so nice for new contribs) and 2) the return status is nonzero in that case (relevant for CI, not so much for new contribs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can see both sides, but I guess I think it'd be more confusing to have it build locally and then fail on CI. OTOH it'd be great to have the warning switch very clearly documented so knowledgable users can run that way and then clean all their errors in one compile.
Besides, the current behaviour is to fail on warning by default.
Minor edit to use |
Seems to need a rebase. |
fixed |
I prefer to default to fail-on-warning, but will not block the PR over it. |
changed to -W by default |
| @@ -1,228 +0,0 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, this all looks great to me, and it works, and the tests pass, so I think its good to go.But do we need to removemake.py? Someone may have a fancy script that uses it for something and want to keep it around, maybe with a big warning at the beginning that its going away soon?
The converse of that argument is that it might be confusing to have both.
I don't really care, but was just trying to think of what could go wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we ever made any guarantees as to backcompatibility of tooling (and wouldn't want to do so either).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Even I think we don't need to worry about this too much. The downstream packagers (debian, fedora, etc) will need to adapt their build scripts, but it is clearly documented what to change it too.
Thanks@anntzer |
Yay! |
PR Summary
My own attempt at switching to
make htmlfor docs build.PR Checklist