Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#16396 closed enhancement (fixed)

upgrade Sphinx to 1.2

Reported by: rws Owned by:
Priority: major Milestone: sage-6.4
Component: documentation Keywords: speed, slow, build, rst
Cc: strogdon, Snark Merged in:
Authors: Ralf Stephan, Steven Trogdon Reviewers: François Bissey, Julien Puydt, John Palmieri
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: de9630d (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by rws)

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:

https://pypi.python.org/packages/source/S/Sphinx/Sphinx-1.2.2.tar.gz

Change History (69)

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:2 Changed 6 years ago by rws

  • Branch set to u/rws/upgrade_sphinx_to_1_2

comment:3 Changed 6 years ago by rws

  • Commit set to cd12a2bfbea0cb98f48d69b009e7d3c8ccdae885

New commits:

cd12a2b16396: 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:

50e24c216396: 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.

I think you can build the document with Sphinx-1.1.3. Alternatively, please contact to sage documentation maintainer.

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:

f81c55d16396: fixing sage-doc-9999-r1 with Steve's help
53bf86e16396: 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: 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?

Tarball link is added.

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

Replying to rws:

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?

Tarball link is added.

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

Replying to 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?

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:19 Changed 6 years ago by strogdon

  • Cc strogdon added

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

86ba11cMerge branch 'develop' into t/16396/upgrade_sphinx_to_1_2
761be3016396: latex math output is broken for inline math in chapters

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

Replying to 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

Thanks, I have added your patch with you as author.

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:27 Changed 6 years ago by Snark

  • Cc Snark added

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

Replying to 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.

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: 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**\n`monospace`')
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>'
**********************************************************************
1 item had failures:
   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

Replying to Snark:

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

Replying to Snark:

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 b  
    11tarball=sphinx-VERSION.tar.bz2
    2 sha1=2a6f238185a58cbb5c597c985d715ab9653e0fbe
    3 md5=9ea0a6f218779ddbe3222c5ef8c0f6e6
    4 cksum=1040528499
     2sha1=deb2a9308d50a508ed9c26b02373a3965c2a5500
     3md5=a81ab4ca80eec8fa1cc72cb5858f7f5a
     4cksum=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: 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:

428dcc6Correct checksum, refresh patch to eliminate the fuzz.

comment:52 follow-up: 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

Replying to kcrisman:

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:

http://git.sagemath.org/sage.git/commit/?id=761be309a895a85c91ec24af8acc69eca0126faf

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:

de9630dGet 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:

de9630dGet 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: 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

Replying to slelievre:

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?

comment:68 in reply to: ↑ 50 Changed 6 years ago by Snark

Replying to 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.

Sorry, I've been away for two weeks, then my browser didn't let me connect to trac (I just found the time to dig and behead the beast). Great work, thanks!

comment:69 Changed 6 years ago by kcrisman

<psa>For future reference, folks, please do use/read SPKG.txt. This caused some annoyance in #17265 because of #17369.</psa>

Note: See TracTickets for help on using tickets.