Opened 6 years ago

Closed 6 years ago

## #16396 closed enhancement (fixed)

Reported by: Owned by: rws major sage-6.4 documentation speed, slow, build, rst strogdon, Snark Ralf Stephan, Steven Trogdon François Bissey, Julien Puydt, John Palmieri Reported upstream. No feedback yet. de9630d (Commits)

According to https://groups.google.com/forum/#!topic/sphinx-users/EC7_wNtE5To Sphinx-1.2 is 25% faster than 1.1.2, and this matters with doc-clean Sage builds. (This assumption turned out to be false, see comment:18)

Source:

### comment:1 Changed 6 years ago by fbissey

The sage-on-gentoo experience with this is that we need to clean up a number of things before upgrading. It of course can be done at the same time.

### comment:3 Changed 6 years ago by rws

• Commit set to cd12a2bfbea0cb98f48d69b009e7d3c8ccdae885

New commits:

 ​cd12a2b 16396: sphinx-1.2.2 install: fix grammar script; bump version

Lots of SEEALSO problems apparently.

Last edited 6 years ago by rws (previous) (diff)

### comment:4 Changed 6 years ago by git

• Commit changed from cd12a2bfbea0cb98f48d69b009e7d3c8ccdae885 to 50e24c23c2d80d64bf50fc288f328f56fe022003

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

 ​50e24c2 16396: fix SEEALSO indentation

### comment:5 Changed 6 years ago by rws

• Report Upstream changed from N/A to Reported upstream. No feedback yet.
• Status changed from new to needs_info

### comment:6 Changed 6 years ago by rws

• Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers deny it's a bug.
• Status changed from needs_info to needs_work

Upstream says:

Sphinx-1.2 has changed the signature of sphinx.search.IndexBuilder class (e5bd5a84b5fd) and Sage extension has used the IndexBuilder directly without a appended argument.



### comment:7 Changed 6 years ago by fbissey

I will have a look. I have a number of patches for sphinx 1.2 in sage-on-gentoo. We have success at building the html doc with sphinx 1.2. However there is a mystery on whether or not we can build the pdf documentation at the moment.

### comment:8 Changed 6 years ago by fbissey

OK, you did the right thing for SEEALSO what you are missing can be found here https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sage-doc/files/sage-doc-6.2-sphinx.patch that should solve the crash you reported. Not that because you are not sage-on-gentoo you do not have to bother with testing the sphinx version. However we would appreciate if you did.

### comment:9 Changed 6 years ago by git

• Commit changed from 50e24c23c2d80d64bf50fc288f328f56fe022003 to 53bf86e6d78f000e728cf3d57efa2a62d1664412

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

 ​f81c55d 16396: fixing sage-doc-9999-r1 with Steve's help ​53bf86e 16396: Depend on Sage since docs are now built

### comment:10 Changed 6 years ago by rws

• Report Upstream changed from Reported upstream. Developers deny it's a bug. to N/A
• Status changed from needs_work to needs_review

I did a sage -tp --long src/doc/ if you meant that. Except for pdf it's ready for review now. Not sure if we should wait for it.

### comment:11 Changed 6 years ago by jhpalmieri

Can you provide a link to the new tarball?

### comment:12 Changed 6 years ago by fbissey

In sage-on-gentoo pdf generation is officially broken with sphinx 1.2.2. Some math is generated in pure tex rather than latex ("$$...$$" rather than "$...$") and it breaks the pdf engine. We are investigating at this stage.

### comment:13 follow-up: ↓ 15 Changed 6 years ago by rws

• Description modified (diff)

If the pdf issue is an upstream issue, is there a ticket? Can you please add it to this ticket description and set the resp. field? Any other links with information?

### comment:14 Changed 6 years ago by rws

I should say that I manually untarred, renamed the Sphinx directory to sphinx and rebzipped. There must be a better way of doing this, as there is no difference in names to the previous version.

### comment:15 in reply to: ↑ 13 Changed 6 years ago by fbissey

If the pdf issue is an upstream issue, is there a ticket? Can you please add it to this ticket description and set the resp. field? Any other links with information?

We don't know yet if it is an upstream issue or something that sage's doc need to be adapted to. sage-on-gentoo is technically downstream from you not upstream.

### comment:16 follow-up: ↓ 18 Changed 6 years ago by jhpalmieri

For what it's worth, I did several iterations of make doc-clean followed by make. Docbuilding did not speed up with the new version of Sphinx:

• old version: 219.8 seconds, 217.3 seconds, 224.7 seconds
• new version: 227.5 seconds, 218.3 seconds, 219.5 seconds

(This was with 4 threads.) Given the problems with PDF building, maybe there isn't much hurry with this? Or are other people seeing a speed improvement with the new version?

### comment:17 Changed 6 years ago by fbissey

• Report Upstream changed from N/A to Reported upstream. No feedback yet.

OK Steve has found the commit of sphinx that break things for pdflatex and I have left a comment of the corresponding issue https://bitbucket.org/birkenfeld/sphinx/issue/929/parsed-literal-creates-bad-latex-math. If they don't pick up the comment in a couple of day I'll open an issue proper with them.

### comment:18 in reply to: ↑ 16 Changed 6 years ago by rws

For what it's worth, I did several iterations of make doc-clean followed by make. Docbuilding did not speed up with the new version of Sphinx:

• old version: 219.8 seconds, 217.3 seconds, 224.7 seconds
• new version: 227.5 seconds, 218.3 seconds, 219.5 seconds

(This was with 4 threads.) Given the problems with PDF building, maybe there isn't much hurry with this? Or are other people seeing a speed improvement with the new version?

No, it confirms my impression. There is no hurry. I thought it might somehow fix the docbuild issue discussed in sage-releases but I cannot reproduce that at the moment, so I cannot even look if Sphinx-1.2 makes a difference there.

### comment:20 Changed 6 years ago by fbissey

Given the lack of answer to my first comment on the commit that breaks pdf generation I opened a brand new issue for the problem: https://bitbucket.org/birkenfeld/sphinx/issue/1480/latex-math-output-is-broken-for-inline

### comment:21 Changed 6 years ago by fbissey

Upstream is quite unresponsive. If anyone has a direct line they can try that. I think it is time to patch on our side.

### comment:22 follow-up: ↓ 24 Changed 6 years ago by strogdon

I believe I have a patch that's not to invasive, i.e. it basically reverts to the old behavior when using inline math and doesn't disrupt (hopefully) the need to use pure tex in a verbatim (alltt) environment. I'll post here since it will take me time to remember how to commit

--- sphinx/writers/latex.py.orig        2014-06-05 23:38:03.808669401 -0500
+++ sphinx/writers/latex.py     2014-06-05 23:27:22.752376713 -0500
@@ -264,6 +264,7 @@
self.next_figure_ids = set()
self.next_table_ids = set()
# flags
+        self.verbatim = False
self.in_title = 0
self.in_production_list = 0
self.in_footnote = 0
@@ -1319,6 +1320,7 @@
(self.curfilestack[-1], node.line))
if node.rawsource != node.astext():
# most probably a parsed-literal block -- don't highlight
+            self.verbatim = True
self.body.append('\\begin{alltt}\n')
else:
code = node.astext().rstrip('\n')
@@ -1351,6 +1353,7 @@
raise nodes.SkipNode
def depart_literal_block(self, node):
self.body.append('\n\\end{alltt}\n')
+        self.verbatim = False
visit_doctest_block = visit_literal_block
depart_doctest_block = depart_literal_block

--- sphinx/ext/mathbase.py.orig 2014-06-05 23:39:36.773293241 -0500
+++ sphinx/ext/mathbase.py      2014-06-05 23:18:53.949020605 -0500
@@ -88,7 +88,10 @@

def latex_visit_math(self, node):
-    self.body.append('\$$' + node['latex'] + '\$$')
+    if self.verbatim:
+        self.body.append('\$$' + node['latex'] + '\$$')
+    else:
+        self.body.append('$' + node['latex'] + '$')
raise nodes.SkipNode

def latex_visit_displaymath(self, node):


### comment:23 Changed 6 years ago by git

• Commit changed from 53bf86e6d78f000e728cf3d57efa2a62d1664412 to 761be309a895a85c91ec24af8acc69eca0126faf

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

 ​86ba11c Merge branch 'develop' into t/16396/upgrade_sphinx_to_1_2 ​761be30 16396: latex math output is broken for inline math in chapters

### comment:24 in reply to: ↑ 22 Changed 6 years ago by rws

I believe I have a patch that's not to invasive, i.e. it basically reverts to the old behavior when using inline math and doesn't disrupt (hopefully) the need to use pure tex in a verbatim (alltt) environment. I'll post here since it will take me time to remember how to commit

### comment:25 Changed 6 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:26 Changed 6 years ago by fbissey

Anything left to do with this ticket?

### comment:28 Changed 6 years ago by Snark

I wanted to give it a try, but it failed to download the tarball from sagemath.org ; when I took the tarball from the link in the ticket description the sums didn't match.

### comment:29 Changed 6 years ago by fbissey

There are several wrong things. checksums.ini looks for

tarball=sphinx-VERSION.tar.bz2


the linked tarball is Sphinx-1.2.2.tar.gz so of course the checksum won't match. We need to recommit that with

tarball=Sphinx-VERSION.tar.gz


and new checksum to be sure. Once checksum.ini is corrected for the right filename run

./sage -sh sage-fix-pkg-checksums


As detailed in http://sagemath.org/doc/developer/packaging.html#checksums.

Last edited 6 years ago by fbissey (previous) (diff)

### comment:30 follow-up: ↓ 31 Changed 6 years ago by Snark

Changing the filename doesn't modify the checksum as far as I know, so I just renamed the tarball. The fact that it was wrong was enough to see the ticket wasn't ready for review.

### comment:31 in reply to: ↑ 30 Changed 6 years ago by fbissey

• Status changed from needs_review to needs_work

Changing the filename doesn't modify the checksum as far as I know, so I just renamed the tarball. The fact that it was wrong was enough to see the ticket wasn't ready for review.

It is indeed not ready to review. Switching to French Julien.

Il est vrai que changer le nom de fichier ne suffit pas. Mon commentaire a deux instructions:

• changer le nom
• executer le script "./sage -sh sage-fix-pkg-checksums" qui va recalculer les checksums d'apres les fichiers que tu as deja telecharge.

Renommer un tar.gz en tar.bz2 n'est pas une bonne idee, decompresser et recompresser ne donneras pas la meme checksum, si la version de bzip2 n'est pas completement identique le bzip2 serat legerement different.

### comment:32 Changed 6 years ago by Snark

Ooooooooh, I hadn't noticed the original was a .gz and not a .bz2! Indeed, no wonder that didn't work... I had only seen the Sphinx vs sphinx.

I'll probably give it a try later today, to see exactly how much work is involved in fixing that ticket. I'm quite interested in getting docutils upgraded, and this is a prerequisite!

### comment:33 follow-up: ↓ 35 Changed 6 years ago by Snark

Ok, doing things less stupidly does make things better (who would have thought?! ;-P ) : it builds and there are three failing doctests, which are trivial to fix: those doctests expect a newline before a div, and there are none now:

sage -t --long local/lib/python2.7/site-packages/sagenb-0.10.8.2-py2.7.egg/sagenb/misc/sphinxify.py
**********************************************************************
File "local/lib/python2.7/site-packages/sagenb-0.10.8.2-py2.7.egg/sagenb/misc/sphinxify.py", line 69, in sagenb.misc.sphinxify.sphinxify
Failed example:
sphinxify('A test')
Expected:
'\n<div class="docstring">\n    \n  <p>A test</p>\n\n\n</div>'
Got:
'<div class="docstring">\n    \n  <p>A test</p>\n\n\n</div>'
**********************************************************************
File "local/lib/python2.7/site-packages/sagenb-0.10.8.2-py2.7.egg/sagenb/misc/sphinxify.py", line 71, in sagenb.misc.sphinxify.sphinxify
Failed example:
sphinxify('**Testing**\nmonospace')
Expected:
'\n<div class="docstring"...<strong>Testing</strong>\n<span class="math"...</p>\n\n\n</div>'
Got:
'<div class="docstring">\n    \n  <p><strong>Testing</strong>\n<span class="math">monospace</span></p>\n\n\n</div>'
**********************************************************************
File "local/lib/python2.7/site-packages/sagenb-0.10.8.2-py2.7.egg/sagenb/misc/sphinxify.py", line 73, in sagenb.misc.sphinxify.sphinxify
Failed example:
sphinxify('x=y')
Expected:
'\n<div class="docstring">\n    \n  <p><span class="math">x=y</span></p>\n\n\n</div>'
Got:
'<div class="docstring">\n    \n  <p><span class="math">x=y</span></p>\n\n\n</div>'
**********************************************************************
3 of  10 in sagenb.misc.sphinxify.sphinxify
[14 tests, 3 failures, 0.44 s]


I also had a look at the branch to see how the patches looked like : there is a strange chunk on src/doc/common/multidocs.py with a test on whether the sphinx version is less or above 1.2... which makes no sense if we're upgrading to 1.2.2!

So I would say : rebase the branch, fix the tarball issues (name, checksum), remove the chunk mentioned above, fix the doctests and it will be good to review and close!

PS: in fact, I may give git-trac a try and push a patch fixing the doctests this evening... if I manage to.

### comment:34 Changed 6 years ago by Snark

Hmmmmmm... it's in sagenb... how does one proceed in such a case? Ask sagenb to make a new release with patched doctests, then push a ticket to upgrade to that, making this one depend on that new ticket, etc?

### comment:35 in reply to: ↑ 33 Changed 6 years ago by fbissey

Ok, doing things less stupidly does make things better (who would have thought?! ;-P ) > I also had a look at the branch to see how the patches looked like : there is a strange chunk on src/doc/common/multidocs.py with a test on whether the sphinx version is less or above 1.2... which makes no sense if we're upgrading to 1.2.2!

See http://trac.sagemath.org/ticket/16396#comment:8 but like I said we are not strongly attached to it.

For sagenb you can open a ticket on their github https://github.com/sagemath/sagenb and yes we'll need a new release.

### comment:36 Changed 6 years ago by fbissey

Do you want to open an issue for sagenb and make a pull request or would you prefer I do it Julien?

### comment:37 follow-up: ↓ 38 Changed 6 years ago by Snark

First eclib and its detection of NTL, now sagenb and its sphinx doctests... you're late to the party again! ;-P

### comment:38 in reply to: ↑ 37 Changed 6 years ago by fbissey

First eclib and its detection of NTL, now sagenb and its sphinx doctests... you're late to the party again! ;-P

Yes you work while I sleep. I cannot really complain about that ;)

### comment:39 Changed 6 years ago by ppurka

Something is wrong with your patches. First, checksums.ini contains the patch:

• ## build/pkgs/sphinx/checksums.ini

diff --git a/build/pkgs/sphinx/checksums.ini b/build/pkgs/sphinx/checksums.ini
index a9fda75..5befdc5 100644
 a tarball=sphinx-VERSION.tar.bz2 sha1=2a6f238185a58cbb5c597c985d715ab9653e0fbe md5=9ea0a6f218779ddbe3222c5ef8c0f6e6 cksum=1040528499 sha1=deb2a9308d50a508ed9c26b02373a3965c2a5500 md5=a81ab4ca80eec8fa1cc72cb5858f7f5a cksum=2539330004

which neither matches the filename of the actual download, the extension, nor the md5sum of the download when converted to the correct extension using

gunzip -c Sphinx-1.2.2.tar.gz | bzip2 > sphinx-1.2.2.tar.bz2


### comment:40 Changed 6 years ago by ppurka

• Description modified (diff)

### comment:41 Changed 6 years ago by Snark

I checked that when the sagenb upgrade #16911 will be done, this ticket will go well.

### comment:42 Changed 6 years ago by Snark

What's the status?

### comment:43 Changed 6 years ago by fbissey

Are we getting a new sagenb in that can take this and a new docutils?

### comment:44 Changed 6 years ago by Snark

From what I know, we need a new sagenb(#16911) so we can get a new sphinx(here) so we can get a new docutils(#16733) (in that order). And Volker has added a reverse dep from the new sphinx to the new sagenb to the mix, see #16911.

### comment:45 Changed 6 years ago by fbissey

OK so #16911 is positively reviewed, it looks like we need to check everything here, ppurka said the checksum is wrong so it has to be checked and given a last test. After we put on a positive review and hope that it get in the next beta/rc. Can you do the work on the checksum I have a couple of other issues I am dealing with.

### comment:46 Changed 6 years ago by Snark

Ok, I started from ppurka's update_sagenb branch, merged the t/16396/upgrade_sphinx_to_1_2 branch, got the upstream tarball and fixed the checksums as in the ticket description, reinstalled sagenb, reinstalled sphinx, rebuilt sage, then "make ptestlong".

All tests passed.

### comment:47 Changed 6 years ago by ppurka

You also need to submit the updated checksums to the ticket. Then, you can set this to needs review so that @fbissey can confirm the changes.

### comment:48 Changed 6 years ago by Snark

I'm actually reviewing the ticket...

### comment:49 Changed 6 years ago by kcrisman

If there isn't too much left to do here other than this checksum/tarball issue, it would be wonderful to get this and #16911 in the next beta.

### comment:50 follow-up: ↓ 68 Changed 6 years ago by fbissey

I am working on updating the branch (package name, checksums and patch refresh to eliminate fuzz). I'll also update the description once I am finished. It will then be up to Julien to push the button. I am not doing anything new, just clean up.

### comment:51 Changed 6 years ago by fbissey

• Authors set to Ralph Stephan
• Branch changed from u/rws/upgrade_sphinx_to_1_2 to u/fbissey/sphinx-1.2.2
• Commit changed from 761be309a895a85c91ec24af8acc69eca0126faf to 428dcc685e35ffee200792a307167084e6afdbf1
• Description modified (diff)
• Reviewers set to François Bissey, Julien Puydt
• Status changed from needs_work to needs_review

Here it comes....

New commits:

 ​428dcc6 Correct checksum, refresh patch to eliminate the fuzz.

### comment:52 follow-up: ↓ 53 Changed 6 years ago by kcrisman

Is the change from sphinx to Sphinx going to cause any upgrading troubles?

### comment:53 in reply to: ↑ 52 Changed 6 years ago by strogdon

Is the change from sphinx to Sphinx going to cause any upgrading troubles?

I've tried this and I didn't see an issue. But François may see something.

### comment:54 Changed 6 years ago by fbissey

• Status changed from needs_review to positive_review

We've been there before with another package. The answer is not anymore. Since Julien is not around and Steve tried it I am pushing the button to get on with this. We waited long enough.

### comment:55 Changed 6 years ago by strogdon

And now to get the sphinx patches out of #16733 so hopefully #16736 can be resolved.

### comment:56 Changed 6 years ago by vbraun

• Status changed from positive_review to needs_work

Breaks pdf docs for me with

! LATEX ERROR: BAD MATH ENVIRONMENT DELIMITER.

SEE THE LATEX MANUAL OR LATEX COMPANION FOR EXPLANATION.
Type  H <return>  for immediate help.
...

l.222 ... index subgroups of $${\rm SL}_2(\ZZ)$$)}

?
! Emergency stop.
...

l.222 ... index subgroups of $${\rm SL}_2(\ZZ)$$)}

!  ==> Fatal error occurred, no output PDF file produced!
Transcript written on arithgroup.log.
Makefile:55: recipe for target 'arithgroup.pdf' failed
make[1]: *** [arithgroup.pdf] Error 1


I think the tex code has been around for a while, but the new sphinx doesn't allow these math delimiters. I would just change them to the usual dollar sign or backtick.

### comment:57 Changed 6 years ago by strogdon

This should have been taken care of with this commit:

However the name of the patch file, inline-latex doesn't have a .patch extension and thus it is never applied.

### comment:58 Changed 6 years ago by fbissey

Darn I missed that. I will take care of this shortly.

### comment:59 Changed 6 years ago by git

• Commit changed from 428dcc685e35ffee200792a307167084e6afdbf1 to de9630d5106c3336b4c8207c04a0a23f91f4db12

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

 ​de9630d Get the right extension for the inline-latex patch

### comment:60 Changed 6 years ago by kcrisman

• Authors changed from Ralph Stephan to Ralph Stephan, Steven Trogdon
• Reviewers changed from François Bissey, Julien Puydt to François Bissey, Julien Puydt, John Palmieri

New commits:

 ​de9630d Get the right extension for the inline-latex patch

### comment:61 Changed 6 years ago by fbissey

• Authors changed from Ralph Stephan, Steven Trogdon to Ralph Stephan
• Reviewers changed from François Bissey, Julien Puydt, John Palmieri to François Bissey, Julien Puydt
• Status changed from needs_work to needs_review

Fixed.

### comment:62 Changed 6 years ago by kcrisman

• Authors changed from Ralph Stephan to Ralph Stephan, Steven Trogdon
• Reviewers changed from François Bissey, Julien Puydt to François Bissey, Julien Puydt, John Palmieri

### comment:63 Changed 6 years ago by jhpalmieri

• Status changed from needs_review to positive_review

This works for me.

### comment:64 Changed 6 years ago by vbraun

• Branch changed from u/fbissey/sphinx-1.2.2 to de9630d5106c3336b4c8207c04a0a23f91f4db12
• Resolution set to fixed
• Status changed from positive_review to closed

### comment:65 follow-up: ↓ 67 Changed 6 years ago by slelievre

• Commit de9630d5106c3336b4c8207c04a0a23f91f4db12 deleted

The upstream sphinx ticket has a question from 2014-07-13, can someone add a comment there?

### comment:66 Changed 6 years ago by rws

• Authors changed from Ralph Stephan, Steven Trogdon to Ralf Stephan, Steven Trogdon
• Description modified (diff)

### comment:67 in reply to: ↑ 65 Changed 6 years ago by strogdon

The upstream sphinx ticket has a question from 2014-07-13, can someone add a comment there?

I'm the one that commented there. And the suggested patch is included in this ticket. What were you thinking of in terms of comment?