#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, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
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 7 years ago by
comment:2 Changed 7 years ago by
- Branch set to u/rws/upgrade_sphinx_to_1_2
comment:3 Changed 7 years ago by
- Commit set to cd12a2bfbea0cb98f48d69b009e7d3c8ccdae885
New commits:
cd12a2b | 16396: sphinx-1.2.2 install: fix grammar script; bump version
|
Lots of SEEALSO
problems apparently.
comment:4 Changed 7 years ago by
- 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 7 years ago by
- Report Upstream changed from N/A to Reported upstream. No feedback yet.
- Status changed from new to needs_info
Further, I get a crash which is reported at https://bitbucket.org/birkenfeld/sphinx/issue/1475/typeerror-in-get_js_index
comment:6 Changed 7 years ago by
- 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 7 years ago by
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 7 years ago by
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 7 years ago by
- Commit changed from 50e24c23c2d80d64bf50fc288f328f56fe022003 to 53bf86e6d78f000e728cf3d57efa2a62d1664412
comment:10 Changed 7 years ago by
- 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 7 years ago by
Can you provide a link to the new tarball?
comment:12 Changed 7 years ago by
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 7 years ago by
- 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 7 years ago by
I should say that I manually untarred, renamed the Sphinx
directory to sphinx
and rebzip
ped. 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 7 years ago by
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: ↓ 18 Changed 7 years ago by
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 7 years ago by
- 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 7 years ago by
Replying to jhpalmieri:
For what it's worth, I did several iterations of
make doc-clean
followed bymake
. 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 7 years ago by
- Cc strogdon added
comment:20 Changed 7 years ago by
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 7 years ago by
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 7 years ago by
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 7 years ago by
- Commit changed from 53bf86e6d78f000e728cf3d57efa2a62d1664412 to 761be309a895a85c91ec24af8acc69eca0126faf
comment:24 in reply to: ↑ 22 Changed 7 years ago by
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 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:26 Changed 7 years ago by
Anything left to do with this ticket?
comment:27 Changed 7 years ago by
- Cc Snark added
comment:28 Changed 7 years ago by
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 7 years ago by
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.
comment:30 follow-up: ↓ 31 Changed 7 years ago by
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 7 years ago by
- 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 7 years ago by
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 7 years ago by
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 7 years ago by
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 7 years ago by
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 7 years ago by
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 7 years ago by
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 7 years ago by
comment:39 Changed 7 years ago by
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 1 1 tarball=sphinx-VERSION.tar.bz2 2 sha1= 2a6f238185a58cbb5c597c985d715ab9653e0fbe3 md5= 9ea0a6f218779ddbe3222c5ef8c0f6e64 cksum= 10405284992 sha1=deb2a9308d50a508ed9c26b02373a3965c2a5500 3 md5=a81ab4ca80eec8fa1cc72cb5858f7f5a 4 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 7 years ago by
- Description modified (diff)
comment:41 Changed 7 years ago by
I checked that when the sagenb upgrade #16911 will be done, this ticket will go well.
comment:42 Changed 7 years ago by
What's the status?
comment:43 Changed 7 years ago by
Are we getting a new sagenb in that can take this and a new docutils?
comment:44 Changed 7 years ago by
comment:45 Changed 7 years ago by
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
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
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
I'm actually reviewing the ticket...
comment:49 Changed 6 years ago by
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
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
- 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
comment:52 follow-up: ↓ 53 Changed 6 years ago by
Is the change from sphinx
to Sphinx
going to cause any upgrading troubles?
comment:53 in reply to: ↑ 52 Changed 6 years ago by
Replying to kcrisman:
Is the change from
sphinx
toSphinx
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
- 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
comment:56 Changed 6 years ago by
- 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
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
Darn I missed that. I will take care of this shortly.
comment:59 Changed 6 years ago by
- 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
- 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
- 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
- Reviewers changed from François Bissey, Julien Puydt to François Bissey, Julien Puydt, John Palmieri
comment:63 Changed 6 years ago by
- Status changed from needs_review to positive_review
This works for me.
comment:64 Changed 6 years ago by
- 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
- 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
- Description modified (diff)
comment:67 in reply to: ↑ 65 Changed 6 years ago by
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
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!
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.