Opened 7 years ago
Closed 7 years ago
#15727 closed defect (fixed)
Check for doc build errors in "make doc"
Reported by: | vbraun | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.2 |
Component: | documentation | Keywords: | |
Cc: | ncohen | Merged in: | |
Authors: | John Palmieri | Reviewers: | Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | u/jhpalmieri/docbuild-errors | Commit: | 7a0c4ba6bf7f52da7bf00973695bc13df7d78439 |
Dependencies: | Stopgaps: |
Description (last modified by )
Errors in the documentation should cause the doc
makefile targets to fail, but they do not.
Change History (21)
comment:1 Changed 7 years ago by
- Description modified (diff)
- Summary changed from Check for doc build errors in "make [p]test[long]" to Check for doc build errors in "make doc"
comment:2 follow-up: ↓ 3 Changed 7 years ago by
comment:3 in reply to: ↑ 2 Changed 7 years ago by
Replying to vbraun:
To reliably check for errors we must delete the previous output. So if you do that in "make doc" then there is no makefile target for incremental builds.
I hope you're not proposing that make [p]test[long]
would delete all doc output and rebuild all docs from scratch. That would add a significant amount of time to doctesting.
comment:4 Changed 7 years ago by
It doesn't add that much. If you have an alternative to reliably test documentation build then please explain...
comment:5 Changed 7 years ago by
We could just error out in "make doc" without rebuilding from scratch, but add a big fat hint at the end
Incremental documentation builds sometimes cause spurious error messages. To be certain that these are real errors, run "make doc-clean" first if you had not done so already.
comment:6 Changed 7 years ago by
An additional problem is that when doing make doc
twice, the errors are only shown the first time.
comment:7 Changed 7 years ago by
Certainly having make doc
quit after an error would encourage people to fix documentation errors before putting things up for review. At least they would be more likely to notice the errors. We would need a helpful error message at the end, not just that the "reference manual failed to build", but that "reference/plotting failed to build because of errors in the file animate.py". Can we tell ahead of time if it was an incremental build? Otherwise, Volker's suggested error message might be misleading.
Next, we should also make "sage -t FILE" build the docs for that file ;)
comment:8 Changed 7 years ago by
Getting a summary of the errors is relatively easy, just grep through the log.
I'd say incremental build <=> src/doc/output exists
comment:9 Changed 7 years ago by
- Cc ncohen added
Using the --warn-links
option might be a good idea, too.
comment:10 follow-up: ↓ 18 Changed 7 years ago by
I don't think so. First, we don't usually require internet access for building or doctesting Sage, and second, I've had random false positives with --warn-links
: maybe the website was down or maybe there was some other problem. Maybe the patchbot should run with that option, but it should not be the default for docbuilding.
comment:11 Changed 7 years ago by
- Branch set to u/jhpalmieri/docbuild-errors
- Commit set to eace780090b0bcc6445542589ab72c09ee6af4eb
- Status changed from new to needs_review
comment:12 Changed 7 years ago by
- Reviewers set to Volker Braun
comment:13 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:14 Changed 7 years ago by
In a parallel doc build you are exiting only one of the workers. The whole build then waits forever for that worker process because of the old problem with pickling exceptions.
comment:15 Changed 7 years ago by
- Commit changed from eace780090b0bcc6445542589ab72c09ee6af4eb to e70986ee307a64853b40f86f0139e7ad3aa5a285
Branch pushed to git repo; I updated commit sha1. New commits:
e70986e | When parallel docbuilding, terminate all workers if an error occurs.
|
comment:16 Changed 7 years ago by
- Commit changed from e70986ee307a64853b40f86f0139e7ad3aa5a285 to 7a0c4ba6bf7f52da7bf00973695bc13df7d78439
Branch pushed to git repo; I updated commit sha1. New commits:
7a0c4ba | Fix whitespace errors.
|
comment:17 Changed 7 years ago by
I think this fixes it.
comment:18 in reply to: ↑ 10 Changed 7 years ago by
I don't think so. First, we don't usually require internet access for building or doctesting Sage, and second, I've had random false positives with
--warn-links
: maybe the website was down or maybe there was some other problem. Maybe the patchbot should run with that option, but it should not be the default for docbuilding.
Oh. Well, I did not even know that --warn-links
checked the internet links. My only use for it is that it detects broken internal links (:class:,:func:, ...), and there is a *LOT* of it.
Except in the graph/, numerical/ and combinat/designs/ folders of course :-P
Nathann
comment:19 follow-up: ↓ 20 Changed 7 years ago by
- Status changed from needs_review to positive_review
looks good to me!
The --warn-links
mode (Sphinx's nitpicky
mode) could be customized by monkey patching _warn_missing_reference
. But that is only really useful unless somebody also would clean up broken links in the docs. So that is for another ticket, if anybody volunteers ;-)
comment:20 in reply to: ↑ 19 Changed 7 years ago by
The
--warn-links
mode (Sphinx'snitpicky
mode) could be customized by monkey patching_warn_missing_reference
. But that is only really useful unless somebody also would clean up broken links in the docs. So that is for another ticket, if anybody volunteers ;-)
Well. That really is a lot of work if it is to be done by somebody who does not know the code. A couple of times I tried to clean the links of the combinat/ folder, and it is REALLY painful when you have no idea what link was intended. Unless we have a way to make sure that all new links are correct I am afraid this will never be done. Unless people clean their respective code of course.
Nathann
comment:21 Changed 7 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
Ideally we would check in "make doc", but incremental builds often lead to sphinx failures. To reliably check for errors we must delete the previous output. So if you do that in "make doc" then there is no makefile target for incremental builds.