Opened 5 years ago

Closed 5 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 jdemeyer)

Errors in the documentation should cause the doc makefile targets to fail, but they do not.

Change History (21)

comment:1 Changed 5 years ago by jdemeyer

  • 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: Changed 5 years ago by vbraun

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.

comment:3 in reply to: ↑ 2 Changed 5 years ago by jdemeyer

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 5 years ago by vbraun

It doesn't add that much. If you have an alternative to reliably test documentation build then please explain...

comment:5 Changed 5 years ago by vbraun

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

An additional problem is that when doing make doc twice, the errors are only shown the first time.

comment:7 Changed 5 years ago by jhpalmieri

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 5 years ago by vbraun

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 5 years ago by vbraun

  • Cc ncohen added

Using the --warn-links option might be a good idea, too.

comment:10 follow-up: Changed 5 years ago by jhpalmieri

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 5 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/docbuild-errors
  • Commit set to eace780090b0bcc6445542589ab72c09ee6af4eb
  • Status changed from new to needs_review

Here's an attempt.


New commits:

eace780Errors in docbuilding should raise an error.

comment:12 Changed 5 years ago by vbraun

  • Authors set to John Palmieri
  • Reviewers set to Volker Braun

comment:13 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:14 Changed 5 years ago by vbraun

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 5 years ago by git

  • Commit changed from eace780090b0bcc6445542589ab72c09ee6af4eb to e70986ee307a64853b40f86f0139e7ad3aa5a285

Branch pushed to git repo; I updated commit sha1. New commits:

e70986eWhen parallel docbuilding, terminate all workers if an error occurs.

comment:16 Changed 5 years ago by git

  • Commit changed from e70986ee307a64853b40f86f0139e7ad3aa5a285 to 7a0c4ba6bf7f52da7bf00973695bc13df7d78439

Branch pushed to git repo; I updated commit sha1. New commits:

7a0c4baFix whitespace errors.

comment:17 Changed 5 years ago by jhpalmieri

I think this fixes it.

comment:18 in reply to: ↑ 10 Changed 5 years ago by ncohen

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: Changed 5 years ago by vbraun

  • 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 5 years ago by ncohen

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 ;-)

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 5 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.