Opened 12 years ago
Closed 11 years ago
#10200 closed defect (wontfix)
Catch Sphinx WARNING or ERROR
Reported by: | jdemeyer | Owned by: | GeorgSWeber |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | build | Keywords: | sphinx build documentation |
Cc: | mvngu | Merged in: | |
Authors: | Reviewers: | Minh Van Nguyen, Jeroen Demeyer | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
When Sphinx gives a warning or error but continues, the user will usually never notice. I propose that doc/common/builder.py
should look for these errors and warnings and abort if a problem is found.
Apply:
Attachments (4)
Change History (20)
comment:1 Changed 12 years ago by
- Cc mvngu added
- Keywords documentation added
comment:2 follow-up: ↓ 3 Changed 12 years ago by
It would also be better to have the different parts of the documentation split in the Makefile.
Btw, the component should be "documentation" I think.
comment:3 in reply to: ↑ 2 Changed 12 years ago by
Replying to leif:
Btw, the component should be "documentation" I think.
Well, it's about building the documentation more than about the documentation itself
comment:4 Changed 12 years ago by
Btw, we could also grep the logfile(s) (e.g. dochtml.log
) in the Makefile for Sphinx errors and warnings, at least to give a summary or short message.
comment:5 Changed 12 years ago by
- Status changed from new to needs_review
comment:6 Changed 12 years ago by
Changed 12 years ago by
comment:7 follow-up: ↓ 8 Changed 12 years ago by
- Description modified (diff)
- Reviewers set to Minh Van Nguyen
I'm mostly OK with jdemeyer's patch 10200_docbuilder.patch. But I also made some changes as follows:
- Don't use the backslash character "\" for line continuation. Line continuation is already implied by the opening parenthesis.
- Fix typos introduced by the patch 10200_docbuilder.patch.
- Use
instead of
<name> is not None
not <name> is None
- Use symbolic names of exit code as documented at http://docs.python.org/library/errno.html, rather than hard-code the actual numeric values.
See the ticket description for instructions on which patches to apply. If my patch gets a positive review, then the whole ticket gets a positive review.
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 12 years ago by
Replying to mvngu:
- Use symbolic names of exit code as documented at http://docs.python.org/library/errno.html, rather than hard-code the actual numeric values.
I disagree. errno
is meant for C functions like fopen()
. I have never seen these used as exit codes for a program.
Other reviewer changes are fine for me.
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 12 years ago by
Replying to jdemeyer:
I disagree.
errno
is meant for C functions likefopen()
. I have never seen these used as exit codes for a program.
Could you at least explain to me the meaning of the numeric values you used for exit code and document such values in your patch? Better still, for my benefit could you please point me to relevant documentation explaining exit code for the particular situations under consideration?
comment:10 in reply to: ↑ 9 Changed 12 years ago by
- Status changed from needs_review to needs_work
Replying to mvngu:
Better still, for my benefit could you please point me to relevant documentation explaining exit code for the particular situations under consideration?
I'm afraid there are not much standards about exit codes. The main thing is that 0 means success and anything else means failure. Also, many operation systems (if not all) use 1 byte for the exit code, so the range is ![0,255].
bash uses 128+sig
as exit code when a process fails with signal sig
. For example, type cat
(without arguments) and press CTRL-C. Then the exit code will be 130 (= 128 + 2 with SIGINT
== 2):
$ cat ^C $ echo $? 130
Because of this, exit codes are usually in the range ![0,127]. Some programs use higher numbers to indicate more serious errors (say: 1 for normal errors, 2 for serious errors), but this is not a general rule.
I will provide a patch with better documented exit codes.
comment:11 Changed 12 years ago by
- Status changed from needs_work to needs_review
Patch changing exit codes, apply on top of original patch and reviewer patch.
comment:12 Changed 12 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
I like the documentation on exit codes. Positive review.
comment:13 Changed 12 years ago by
Thanks for the review.
However, there is one important issue with this patch: if Sphinx gives warnings about latex being unavailable, this patch will also make that into an error (which is not desirable).
So this is what I will do: wait for #10118 to have positive_review, then fix the latex WARNING issue, then merge this patch.
comment:14 Changed 12 years ago by
- Status changed from positive_review to needs_work
There is another issue with this patch:
sage/misc/sagedoc.py
(very stupidly) reads the first 1000 bytes of doc/common/builder.py
and searches for this part:
SAGE_DOC = os.environ['SAGE_DOC'] LANGUAGES = ['en', 'fr'] SPHINXOPTS = "" PAPER = "" OMIT = ["introspect"] # docs/dirs to omit when listing and building 'all'
In particular, it needs the "LANGUAGES" and "OMIT" variables.
By adding my patch, these options are no longer in the first 1000 bytes. I believe we should use some Pythonic mechanism (import
doesn't seem to work since this file is in doc/
, not sage/
) to read these options instead of essentially grepping stuff from builder.py
. Any ideas...?
comment:15 Changed 12 years ago by
- Description modified (diff)
The last issue mentioned in the comment above is fixed at #10351.
comment:16 Changed 11 years ago by
- Milestone changed from sage-4.7.2 to sage-duplicate/invalid/wontfix
- Resolution set to wontfix
- Reviewers changed from Minh Van Nguyen to Minh Van Nguyen, Jeroen Demeyer
- Status changed from needs_work to closed
Closing this ticket as "wontfix" since Sphinx warnings can sometimes simply indicate a missing dvipng
for example. We should not punish the user for that.
I am still open for suggestions though. If you feel this ticket needs to be reopened, just let me know.
Cf. #9426, #10191.