Opened 7 years ago

Closed 5 years ago

#19882 closed defect (wontfix)

Let "make doc" always work.

Reported by: Thierry Monteil Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords:
Cc: Merged in:
Authors: Reviewers: John Palmieri, Thierry Monteil
Report Upstream: N/A Work issues:
Branch: u/tmonteil/let__make_doc__always_work_ (Commits, GitHub, GitLab) Commit: 83d8cc8c87a6c4bb32a8bb9ffad22bfa59872889
Dependencies: Stopgaps:

Status badges

Description

As requested in #18464. Currently make doc can fail when some previous documentation was built.

Change History (46)

comment:1 Changed 7 years ago by Thierry Monteil

Branch: u/tmonteil/let__make_doc__always_work_

comment:2 Changed 7 years ago by Thierry Monteil

Authors: Thierry Monteil
Commit: 131ff97a5e7fa8dfb33c907ad68dc2bfe6151a54
Status: newneeds_review

The proposed fix is to just let the doc target depend on doc-clean. Perhaps is there a lighter solution ?


New commits:

131ff97#19882 : let doc target depend on doc-clean.

comment:3 Changed 7 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

No, this would force the doc to be rebuilt everytime you run make.

comment:4 Changed 7 years ago by Thierry Monteil

So, what should be done ?

comment:5 Changed 7 years ago by Jeroen Demeyer

Figure out why the docbuilder sometimes fails and fix that.

comment:6 Changed 7 years ago by John Palmieri

Do we have ideas why? If a .py or .pyx file is present in one branch and not in another, this confuses the docbuilder. What else goes wrong?

comment:7 in reply to:  4 Changed 7 years ago by Jeroen Demeyer

Replying to tmonteil:

So, what should be done ?

As quick fix, the following might work:

  1. try to build the documentation normally.
  2. if that fails, run make doc-clean and rebuild the documentation.

Step 1 is crucial to avoid a lot of needless rebuilds of the documentation.

comment:8 Changed 7 years ago by git

Commit: 131ff97a5e7fa8dfb33c907ad68dc2bfe6151a5480a0a522ccc5c0768558ae32ea98c995a3c8e9ca

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

80a0a52#19882 : run doc-clean only if doc-html lead to an error.

comment:9 Changed 7 years ago by Jeroen Demeyer

I am wondering if the recursive call of $(MAKE) is a good idea (I'm not saying it's bad, I just need to think about it).

Last edited 7 years ago by Jeroen Demeyer (previous) (diff)

comment:10 Changed 7 years ago by Thierry Monteil

The current Makefile already calls $(MAKE), see e.g. in #18710:

bdist-clean: clean
        $(MAKE) misc-clean

That said, there is no real recursivity since the doc target depends on doc-html and doc-clean, and none of those depend on doc, so i do not see any loop possibility. I am currently doing a few tests to see how it works in various cases.

comment:11 Changed 7 years ago by Jeroen Demeyer

It's problematic that doc-html has dependencies which also appear as dependencies of all-sage. So we can end up in a situation where there are two instances of make building the same targets.

comment:12 in reply to:  10 Changed 7 years ago by Jeroen Demeyer

Maybe you can solve this with an extra level of indirection:

doc-html: $(DOC_DEPENDENCIES)
    $(MAKE) docbuild-html || ( $(MAKE) doc-clean ; $(MAKE) docbuild-html )

docbuild-html:
    cd ../.. && sage-logger './sage --docbuild --no-pdf-links all html $(SAGE_DOCBUILD_OPTS)' logs/dochtml.log

Ugly, but I guess it would work.

comment:13 Changed 7 years ago by git

Commit: 80a0a522ccc5c0768558ae32ea98c995a3c8e9ca5b555ae3493879c81f7f7ba7190dd1e272f54dbd

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

5b555ae#19882 add one level of indirection (comment 12)

comment:14 Changed 7 years ago by Thierry Monteil

Status: needs_workneeds_review

It seems to work. Do someone know a way to reproduce that kind of docbuild error that disappears after a doc-clean ?

comment:15 Changed 7 years ago by Jeroen Demeyer

Did you try make -q too? I am afraid that will break.

comment:16 in reply to:  14 Changed 7 years ago by Travis Scrimshaw

Replying to tmonteil:

It seems to work. Do someone know a way to reproduce that kind of docbuild error that disappears after a doc-clean ?

I would just create a dummy file with some simple doc1, build the doc, delete the file, and rebuild the doc (perhaps on a branch based on this one).

Alternatively, you could create a branch where you just remove a file and try to build the doc.

1 - Don't forget to add an entry in the .rst files.

comment:17 Changed 6 years ago by Sébastien Labbé

#21378 got positive review (duplicate won't fix) saying that this ticket (#19882) is a better solution. But there was no update on #19882 since 8 months...

No later than yesterday, I run make ptestlong overnight to realize the day after that the doc failed and no test at all were ever made:

[dochtml] OSError: [homology ] Exception occurred:
[dochtml] 
[dochtml] 
[dochtml] Note: incremental documentation builds sometimes cause spurious
[dochtml] error messages. To be certain that these are real errors, run
[dochtml] "make doc-clean" first and try again.
Makefile:1061 : la recette pour la cible « doc-html » a échouée

I would like that make ptestlong always work...

comment:18 Changed 6 years ago by Jeroen Demeyer

Milestone: sage-7.0sage-7.5
Status: needs_reviewneeds_work

Merge conflict...

comment:19 Changed 6 years ago by git

Commit: 5b555ae3493879c81f7f7ba7190dd1e272f54dbde80667dee0e1ad2fff59c3b3007a724d80c0aed5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e80667d#19882 : rebased on 7.6.beta6.

comment:20 Changed 6 years ago by Thierry Monteil

Status: needs_workneeds_review

Here is a rebased version. Unfortunately, my laptop does not have enough memory anymore to build Sage doc, so that i can not test it seriously :(

comment:21 Changed 6 years ago by John Palmieri

I got the error

***********************************************
Makefile:1093: *** missing separator.  Stop.

with this, until I made the change

  • build/make/deps

    diff --git a/build/make/deps b/build/make/deps
    index e18fd6f..da0a4e9 100644
    a b DOC_DEPENDENCIES = sagelib $(inst_sphinx) $(inst_sagenb) \ 
    206206doc: doc-html
    207207
    208208doc-html: $(DOC_DEPENDENCIES)
    209     $(MAKE) docbuild-html || ( $(MAKE) doc-clean ; $(MAKE) docbuild-html )
     209       $(MAKE) docbuild-html || ( $(MAKE) doc-clean ; $(MAKE) docbuild-html )
    210210
    211211docbuild-html:
    212212       $(AM_V_at)cd ../.. && sage-logger -p './sage --docbuild --no-pdf-links all html $(SAGE_DOCBUILD_OPTS)' logs/dochtml.log

(that is, replacing spaces with a tab).

comment:22 Changed 6 years ago by git

Commit: e80667dee0e1ad2fff59c3b3007a724d80c0aed583d8cc8c87a6c4bb32a8bb9ffad22bfa59872889

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

83d8cc8#19882 : replace spaces by tabs.

comment:23 Changed 6 years ago by Thierry Monteil

Oops, thanks for noticing, i did a 'hand rebase', without noticing that my editor behave as for python files. Sorry.

comment:24 Changed 6 years ago by John Palmieri

Reviewers: John Palmieri
Status: needs_reviewpositive_review

This works. I wish there were a less drastic solution, but let's merge it now and see if someone comes up with something better later.

comment:25 Changed 6 years ago by Jeroen Demeyer

Did you check that make -q still works as expected?

comment:26 Changed 6 years ago by John Palmieri

Status: positive_reviewneeds_info

No, good point.

comment:27 in reply to:  25 Changed 6 years ago by John Palmieri

Replying to jdemeyer:

Did you check that make -q still works as expected?

I guess I don't know what you mean, now that I've tested it. With the current develop branch, if I run make successfully, then I see

$ make -q
$ echo $?
1

So it doesn't work (IMHO) with the develop branch. It continues to not work with this branch. Is that working "as expected"?

comment:28 Changed 6 years ago by Kwankyu Lee

Check this out for alternative solution.

https://trac.sagemath.org/ticket/22588

comment:29 Changed 6 years ago by John Palmieri

I think that the alternate solution at #22588 and this one both have advantages. For example, if you change your version of Sphinx, you may need to delete the docs and rebuild from scratch (maybe because of the inventory files? I'm not sure). I'm not sure that #22588 will catch that. So why not use both approaches?

That said, my question above about make -q still remains.

Last edited 6 years ago by John Palmieri (previous) (diff)

comment:30 Changed 5 years ago by John Palmieri

Jeroen: can you explain what you meant in comment:25 about make -q? With or without this branch, make -q leads to echo $? reporting "1", not "0". Do you object to a positive review for this?

(As I said, I also think that there is reason to keep both this and #22588.)

comment:31 Changed 5 years ago by Jeroen Demeyer

So what should we do here, given #22588? Close as wontfix?

comment:32 Changed 5 years ago by John Palmieri

Will #22588 always work, or will the docs still fail to build some time (I guessed at one scenario in comment:29)?

comment:33 in reply to:  32 Changed 5 years ago by Jeroen Demeyer

Replying to jhpalmieri:

Will #22588 always work, or will the docs still fail to build some time

That's something to be seen I guess. The question is: if issues arise with #22588, should we try to fix those issues or should we implement the "nuclear option" of this ticket?

comment:34 Changed 5 years ago by John Palmieri

I don't have strong feelings about it. Since it took so long to get an attempted fix, I'm not that optimistic that future problems will be dealt with, but I don't mind closing this and seeing what happens.

comment:35 Changed 5 years ago by Kwankyu Lee

Now with #22588, I hope that the doc builder is reliable. I mean: if doc build fails, then the reason is genuine, that is, there is a problem with the doc source, and doc build would fail even after doc-clean. Then the solution of this ticket is actually just to delay the time to recognize the problem of the doc source.

But time will tell whether I am just too optimistic..

comment:36 Changed 5 years ago by John Palmieri

I just did an incremental build from 8.1.beta3 to 8.1.beta4 and got

[dochtml] Building reference manual, first pass.
[dochtml] 
[dochtml] Error building the documentation.
[dochtml] Traceback (most recent call last):
[dochtml]   File "/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/runpy.py", line 174, in _run_module_as_main
[dochtml]     "__main__", fname, loader, pkg_name)
[dochtml]   File "/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/runpy.py", line 72, in _run_code
[dochtml]     exec code in run_globals
[dochtml]   File "/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
[dochtml]     main()
[dochtml]   File "/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 1675, in main
[dochtml]     builder()
[dochtml]   File "/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 310, in _wrapper
[dochtml]     getattr(get_builder(document), 'inventory')(*args, **kwds)
[dochtml]   File "/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 505, in _wrapper
[dochtml]     build_many(build_ref_doc, L)
[dochtml]   File "/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 246, in build_many
[dochtml]     ret = x.get(99999)
[dochtml]   File "/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/multiprocessing/pool.py", line 567, in get
[dochtml]     raise self._value
[dochtml] AttributeError: 'module' object has no attribute 'WarningStream'
[dochtml] 
[dochtml] Note: incremental documentation builds sometimes cause spurious
[dochtml] error messages. To be certain that these are real errors, run
[dochtml] "make doc-clean" first and try again.

comment:37 Changed 5 years ago by John Palmieri

And this seems to be repeatable for me: I switched back to 8.1.beta3, did make doc-clean; make, and then when I tried to build 8.1.beta4, same error.

comment:38 in reply to:  36 Changed 5 years ago by Jeroen Demeyer

Replying to jhpalmieri:

[dochtml] AttributeError: 'module' object has no attribute 'WarningStream'

This is very likely due to the Sphinx upgrade #23023. It makes sense to require a complete rebuild of the docs when changing Sphinx versions. This should be automated, just like we automatically re-cythonize the Sage library whenever the Cython version changes.

comment:39 Changed 5 years ago by John Palmieri

We could delete the docs in the Sphinx spkg-install script. We used to do this, but then at some point (#11665) we decided to try to keep the old docs.

comment:40 Changed 5 years ago by John Palmieri

For example like this:

  • build/pkgs/sphinx/spkg-install

    diff --git a/build/pkgs/sphinx/spkg-install b/build/pkgs/sphinx/spkg-install
    index 200965b566..05085aaa79 100644
    a b $PIP_INSTALL . 
    2323success 'Error installing Sphinx'
    2424echo
    2525
     26echo "A new version of Sphinx was installed, so deleting old Sage documentation output..."
     27echo "Removing \"$SAGE_DOC\"..."
     28rm -rf "$SAGE_DOC"
     29success 'Error removing the old documentation'
     30echo
     31
    2632cd "$CUR"
    2733echo "Creating grammar pickle..."
    2834sage-python23 create_grammar_pickle.py

comment:41 Changed 5 years ago by John Palmieri

If this is a good idea, we can do it here or open another ticket.

comment:42 Changed 5 years ago by Jeroen Demeyer

I like the principle, but not the approach of using the Sphinx installer to remove the docs. It goes against seperation of concerns. Better would be to implement this in the docbuilder: find out the timestamp or version of the installed Sphinx and then take action.

comment:43 Changed 5 years ago by Jeroen Demeyer

See #23803

comment:44 Changed 5 years ago by John Palmieri

Milestone: sage-7.5sage-duplicate/invalid/wontfix
Status: needs_infoneeds_review

Okay, let's close this.

comment:45 Changed 5 years ago by Jeroen Demeyer

Authors: Thierry Monteil
Reviewers: John PalmieriJohn Palmieri, Thierry Monteil
Status: needs_reviewpositive_review

comment:46 Changed 5 years ago by Erik Bray

Resolution: wontfix
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.