Opened 6 years ago
Closed 4 years ago
#19882 closed defect (wontfix)
Let "make doc" always work.
Reported by: | tmonteil | 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: |
Description
As requested in #18464. Currently make doc
can fail when some previous documentation was built.
Change History (46)
comment:1 Changed 6 years ago by
- Branch set to u/tmonteil/let__make_doc__always_work_
comment:2 Changed 6 years ago by
- Commit set to 131ff97a5e7fa8dfb33c907ad68dc2bfe6151a54
- Status changed from new to needs_review
comment:3 Changed 6 years ago by
- Status changed from needs_review to needs_work
No, this would force the doc to be rebuilt everytime you run make
.
comment:4 follow-up: ↓ 7 Changed 6 years ago by
So, what should be done ?
comment:5 Changed 6 years ago by
Figure out why the docbuilder sometimes fails and fix that.
comment:6 Changed 6 years ago by
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 6 years ago by
Replying to tmonteil:
So, what should be done ?
As quick fix, the following might work:
- try to build the documentation normally.
- 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 6 years ago by
- Commit changed from 131ff97a5e7fa8dfb33c907ad68dc2bfe6151a54 to 80a0a522ccc5c0768558ae32ea98c995a3c8e9ca
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 6 years ago by
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).
comment:10 follow-up: ↓ 12 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
- Commit changed from 80a0a522ccc5c0768558ae32ea98c995a3c8e9ca to 5b555ae3493879c81f7f7ba7190dd1e272f54dbd
Branch pushed to git repo; I updated commit sha1. New commits:
5b555ae | #19882 add one level of indirection (comment 12)
|
comment:14 follow-up: ↓ 16 Changed 6 years ago by
- Status changed from needs_work to needs_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 6 years ago by
Did you try make -q
too? I am afraid that will break.
comment:16 in reply to: ↑ 14 Changed 6 years ago by
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
#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
- Milestone changed from sage-7.0 to sage-7.5
- Status changed from needs_review to needs_work
Merge conflict...
comment:19 Changed 5 years ago by
- Commit changed from 5b555ae3493879c81f7f7ba7190dd1e272f54dbd to e80667dee0e1ad2fff59c3b3007a724d80c0aed5
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 5 years ago by
- Status changed from needs_work to needs_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 5 years ago by
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) \ 206 206 doc: doc-html 207 207 208 208 doc-html: $(DOC_DEPENDENCIES) 209 $(MAKE) docbuild-html || ( $(MAKE) doc-clean ; $(MAKE) docbuild-html )209 $(MAKE) docbuild-html || ( $(MAKE) doc-clean ; $(MAKE) docbuild-html ) 210 210 211 211 docbuild-html: 212 212 $(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 5 years ago by
- Commit changed from e80667dee0e1ad2fff59c3b3007a724d80c0aed5 to 83d8cc8c87a6c4bb32a8bb9ffad22bfa59872889
Branch pushed to git repo; I updated commit sha1. New commits:
83d8cc8 | #19882 : replace spaces by tabs.
|
comment:23 Changed 5 years ago by
Oops, thanks for noticing, i did a 'hand rebase', without noticing that my editor behave as for python files. Sorry.
comment:24 Changed 5 years ago by
- Reviewers set to John Palmieri
- Status changed from needs_review to positive_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 follow-up: ↓ 27 Changed 5 years ago by
Did you check that make -q
still works as expected?
comment:27 in reply to: ↑ 25 Changed 5 years ago by
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 5 years ago by
Check this out for alternative solution.
comment:29 Changed 5 years ago by
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.
comment:30 Changed 5 years ago by
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
So what should we do here, given #22588? Close as wontfix?
comment:32 follow-up: ↓ 33 Changed 5 years ago by
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
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
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
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 follow-up: ↓ 38 Changed 5 years ago by
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
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
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
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
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 . 23 23 success 'Error installing Sphinx' 24 24 echo 25 25 26 echo "A new version of Sphinx was installed, so deleting old Sage documentation output..." 27 echo "Removing \"$SAGE_DOC\"..." 28 rm -rf "$SAGE_DOC" 29 success 'Error removing the old documentation' 30 echo 31 26 32 cd "$CUR" 27 33 echo "Creating grammar pickle..." 28 34 sage-python23 create_grammar_pickle.py
comment:41 Changed 5 years ago by
If this is a good idea, we can do it here or open another ticket.
comment:42 Changed 5 years ago by
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
See #23803
comment:44 Changed 5 years ago by
- Milestone changed from sage-7.5 to sage-duplicate/invalid/wontfix
- Status changed from needs_info to needs_review
Okay, let's close this.
comment:45 Changed 5 years ago by
- Reviewers changed from John Palmieri to John Palmieri, Thierry Monteil
- Status changed from needs_review to positive_review
comment:46 Changed 4 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
The proposed fix is to just let the
doc
target depend ondoc-clean
. Perhaps is there a lighter solution ?New commits:
#19882 : let doc target depend on doc-clean.