Opened 11 years ago

Closed 10 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:

Status badges

Description (last modified by jdemeyer)

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:

  1. http://trac.sagemath.org/sage_trac/raw-attachment/ticket/10351/10351_sagedoc.patch from #10351
  2. 10200_docbuilder.patch
  3. trac-10200_reviewer.patch
  4. 10200_exitcodes.patch

Attachments (4)

10200_docbuilder.patch (2.6 KB) - added by jdemeyer 11 years ago.
SAGELIB patch against sage-4.6.1.alpha0
10200_docbuilder-sage-4.6.patch (2.5 KB) - added by jdemeyer 11 years ago.
same SAGELIB patch against sage-4.6
trac-10200_reviewer.patch (3.6 KB) - added by mvngu 11 years ago.
10200_exitcodes.patch (2.9 KB) - added by jdemeyer 11 years ago.
Apply on top of reviewer patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 11 years ago by leif

  • Cc mvngu added
  • Keywords documentation added

Cf. #9426, #10191.

comment:2 follow-up: Changed 11 years ago by leif

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 11 years ago by jdemeyer

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 11 years ago by leif

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 11 years ago by jdemeyer

  • Status changed from new to needs_review

Changed 11 years ago by jdemeyer

SAGELIB patch against sage-4.6.1.alpha0

Changed 11 years ago by jdemeyer

same SAGELIB patch against sage-4.6

comment:6 Changed 11 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

Changed 11 years ago by mvngu

comment:7 follow-up: Changed 11 years ago by mvngu

  • 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.
  • Use
    <name> is not None
    
    instead of
    not <name> is None
    

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: Changed 11 years ago by jdemeyer

Replying to mvngu:

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: Changed 11 years ago by mvngu

Replying to jdemeyer:

I disagree. errno is meant for C functions like fopen(). 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 11 years ago by jdemeyer

  • 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.

Changed 11 years ago by jdemeyer

Apply on top of reviewer patch

comment:11 Changed 11 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Patch changing exit codes, apply on top of original patch and reviewer patch.

comment:12 Changed 11 years ago by mvngu

  • Description modified (diff)
  • Status changed from needs_review to positive_review

I like the documentation on exit codes. Positive review.

comment:13 Changed 11 years ago by jdemeyer

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 11 years ago by jdemeyer

  • 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 11 years ago by jdemeyer

  • Description modified (diff)

The last issue mentioned in the comment above is fixed at #10351.

comment:16 Changed 10 years ago by jdemeyer

  • Authors Jeroen Demeyer deleted
  • 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.

Note: See TracTickets for help on using tickets.