Opened 4 years ago

Closed 3 years ago

#25786 closed defect (fixed)

Fix introspection with ? when doc source is not available

Reported by: Julian Rüth Owned by:
Priority: minor Milestone: sage-8.9
Component: documentation Keywords: conda, Arch Linux, introspection
Cc: Antonio Rojas, Jeroen Demeyer, Samuel Lelièvre Merged in:
Authors: Julian Rüth, Isuru Fernando Reviewers: Julian Rüth, Isuru Fernando, François Bissey
Report Upstream: N/A Work issues: is the patchbot happy?
Branch: a401a17 (Commits, GitHub, GitLab) Commit: a401a171fcfde7ed5e5a128bb6af0967e96b11a8
Dependencies: #25843 Stopgaps:

Status badges

Description (last modified by Antonio Rojas)

Currently, introspection does not work in conda and Arch Linux (if the sagemath-doc package is not installed):

sage: ZZ?
…
/usr/lib/python2.7/site-packages/sage/misc/sagedoc.pyc in process_extlinks(s, embedded)
    497     oldpath = sys.path
    498     sys.path = [os.path.join(SAGE_DOC_SRC, 'common')] + oldpath
--> 499     from conf import extlinks
    500     sys.path = oldpath
    501     for key in extlinks:

ImportError: cannot import name extlinks

The problem is that src/doc/common/conf.py is not in the place that Sage expects.

I don't think that we should require the documentation configuration to be installed for Sage to work.

A docker image to play with this is being built here: https://gitlab.com/sagemath/dev/sage/pipelines/25262253

Change History (59)

comment:1 Changed 4 years ago by Julian Rüth

Branch: u/saraedum/25786

comment:2 Changed 4 years ago by Jeroen Demeyer

Cc: Jeroen Demeyer added
Commit: 13958dbee41d1b3c7d4c74e2aacc8a4bdeba0216

Is this needs review?


New commits:

13958dbMove extlinks into the main Sage code

comment:3 Changed 4 years ago by Julian Rüth

I had a comment in #24655 that src/doc/en/introspect is required as well…let's see if that's still the case.

comment:4 Changed 4 years ago by Julian Rüth

jdemeyer: No, I have not tested this yet. But feel free to give feedback if you know something about this :)

comment:5 Changed 4 years ago by Julian Rüth

See #25787 for a followup regarding the docker images.

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

In any case, I agree with the idea behind the patch.

comment:7 Changed 4 years ago by Jeroen Demeyer

Authors: Julian Rüth
Reviewers: Jeroen Demeyer

comment:8 Changed 4 years ago by Julian Rüth

Status: newneeds_review
Work issues: is the patchbot happy?

comment:9 Changed 4 years ago by Julian Rüth

So, if the patchbot and my tests are Ok, then this is a positive review?

comment:10 Changed 4 years ago by Julian Rüth

Description: modified (diff)

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

Replying to saraedum:

So, if the patchbot and my tests are Ok, then this is a positive review?

Yes

comment:12 Changed 4 years ago by Julian Rüth

Description: modified (diff)

comment:13 Changed 4 years ago by Antonio Rojas

This still doesn't allow introspection on Arch without having the doc sources installed, due to

    # Sphinx constructor: Sphinx(srcdir, confdir, outdir, doctreedir,
    # buildername, confoverrides, status, warning, freshenv).
    confdir = os.path.join(SAGE_DOC_SRC, 'en', 'introspect')

    open(os.path.join(srcdir, 'docutils.conf'), 'w').write(r"""
[parsers]
smart_quotes = no
""")
    doctreedir = os.path.join(srcdir, 'doctrees')
    confoverrides = {'html_context': {}, 'master_doc': 'docstring'}

    import sys
    old_sys_path = list(sys.path)  # Sphinx modifies sys.path
    sphinx_app = Sphinx(srcdir, confdir, srcdir, doctreedir, format,
                        confoverrides, None, None, True)
    sphinx_app.build(None, [rst_name])
    sys.path = old_sys_path

in misc/sphinxify.py

comment:14 Changed 4 years ago by git

Commit: 13958dbee41d1b3c7d4c74e2aacc8a4bdeba02160e5b29a86c5d37f87a6ae9bf0b37c5bd3dd639a6

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

0e5b29aFix sphinxify for introspection

comment:15 Changed 4 years ago by Julian Rüth

arojas: Right, now it works for me. But probably the from common.conf import * is there for a reason and I should not simply delete it.

comment:16 Changed 4 years ago by Jeroen Demeyer

I really prefer one ticket, one issue.

comment:17 Changed 4 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work
Work issues: is the patchbot happy?split ticket

comment:18 in reply to:  16 Changed 4 years ago by Julian Rüth

Work issues: split ticketis the patchbot happy?

Replying to jdemeyer:

I really prefer one ticket, one issue.

I believe that this is one issue: "Fix introspection with ? in conda and ArchLinux?". And even the solution can be seen as one thing: "Make Sage runtime not depend on documentation configuration."

comment:19 Changed 4 years ago by Julian Rüth

Status: needs_workneeds_review

The patchbots report weird errors that seem unrelated to the changes here…let me try this out locally.

comment:20 Changed 4 years ago by Jeroen Demeyer

Dependencies: #25843
Reviewers: Jeroen Demeyer

I moved the first part to #25843 and gave positive review to that.

comment:21 Changed 4 years ago by Samuel Lelièvre

Cc: Samuel Lelièvre added
Description: modified (diff)
Keywords: conda Arch Linux introspection added
Summary: Fix introspection with ? in conda and ArchLinuxFix introspection with ? in conda and Arch Linux

comment:22 Changed 4 years ago by Antonio Rojas

Description: modified (diff)
Summary: Fix introspection with ? in conda and Arch LinuxFix introspection with ? when doc source is not available

Updating slightly misleading description - introspection works fine in Arch if the sagemath-doc package is installed.

comment:23 Changed 4 years ago by Jeroen Demeyer

One thing I don't like is the style of the long """-quoted strings in between code. This could be made more readable by moving the strings outside of the function. Something like

# At module level
layout_html = r"""
<div class="docstring">
    {% block body %} {% endblock %}
</div>"""
# Inside the function
    with open(os.path.join(templatesdir, 'layout.html'), 'w') as filed:
        filed.write(layout_html)

comment:24 Changed 4 years ago by git

Commit: 0e5b29a86c5d37f87a6ae9bf0b37c5bd3dd639a6cb997d377324a3f25d61a109d6c07493e8dd903c

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

cb997d3Merge branch '25786' into HEAD

comment:25 Changed 4 years ago by git

Commit: cb997d377324a3f25d61a109d6c07493e8dd903c8b519f5f344cc2256f0e94f5ad1092e49380964e

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

5a249ccMerge remote-tracking branch 'origin/develop' into 25786
8b519f5fix pyflakes error

comment:26 Changed 4 years ago by Isuru Fernando

Status: needs_reviewneeds_work

OMIT is removed from src/sage_setup/docbuild/build_options.py, but it is needed in src/sage_setup/docbuild/__init__.py

comment:27 Changed 3 years ago by git

Commit: 8b519f5f344cc2256f0e94f5ad1092e49380964eedc96e3b517d617b6068af7df932aba8e92cf2e0

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

edc96e3Merge remote-tracking branch 'trac/develop' into u/saraedum/25786

comment:28 Changed 3 years ago by git

Commit: edc96e3b517d617b6068af7df932aba8e92cf2e08c770a707bc9b3e050a9868dccc559948cb86339

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

8c770a7Restore OMIT that is needed by the build

comment:29 Changed 3 years ago by Julian Rüth

Status: needs_workneeds_review

comment:30 Changed 3 years ago by François Bissey

I like it. It helps me remove some manual installs in sage-on-gentoo. In fact I may not have to install the common folder and all the stuff I put in it anymore. This may be a bigger win than it looks like.

comment:31 Changed 3 years ago by François Bissey

The patchbot reports a few broken doctests over formatting.

File "src/sage/misc/sagedoc.py", line 196, in sage.misc.sagedoc.detex
Failed example:
    detex(r'Some math: `n \geq k`.  A website: \url{sagemath.org}.')
Expected:
    'Some math: n >= k.  A website: sagemath.org.\n'
Got:
    'Some math: *n geq k*.  A website: sagemath.org.\n'

as an example.

comment:32 Changed 3 years ago by Samuel Lelièvre

Milestone: sage-8.3sage-8.9

comment:33 Changed 3 years ago by Julian Rüth

Status: needs_reviewneeds_work

comment:34 Changed 3 years ago by Julian Rüth

Work issues: is the patchbot happy?failing doctests

comment:35 Changed 3 years ago by git

Commit: 8c770a707bc9b3e050a9868dccc559948cb86339118b96416fcd593a24650237808fcd558324605f

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

118b964Attempt to fix failing doctests

comment:36 Changed 3 years ago by Julian Rüth

isuruf commented that following seems to be missing.

from sage.env import SAGE_DOC_SRC
sys.path.append(SAGE_DOC_SRC)
from common.conf import *
Last edited 3 years ago by Julian Rüth (previous) (diff)

comment:37 Changed 3 years ago by Isuru Fernando

Maybe move src/doc/common/conf.py to src/sage/docs/conf.py and update the imports?

comment:38 Changed 3 years ago by Isuru Fernando

What do you think about the above suggestion? I can update this branch if you agree

comment:39 Changed 3 years ago by Julian Rüth

Sorry, yes please go ahead :)

comment:40 Changed 3 years ago by François Bissey

It would be nice to finish this. I would add that the clean up should probably also include this line https://github.com/sagemath/sage/blob/master/src/sage_setup/docbuild/build_options.py#L12

OMIT = ["introspect"]  # docs/dirs to omit when listing and building 'all'

This ticket is removing that folder altogether. Do we even need OMIT? If we don't, I think we could remove that logic in a follow up ticket.

comment:42 Changed 3 years ago by Julian Rüth

Sure. You can also just push to this ticket, there is no ownership as with GitHub PRs here.

comment:43 Changed 3 years ago by git

Commit: 118b96416fcd593a24650237808fcd558324605f863d433d402c9257768929f7d62c4bd478eabbc8

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

f2f53b0Merge branch 'u/saraedum/25786' of git://trac.sagemath.org/sage into develop
863d433mv src/doc/common/*.py to src/sage/docs/conf

comment:44 Changed 3 years ago by Isuru Fernando

Status: needs_workneeds_review

comment:45 Changed 3 years ago by Julian Rüth

Is there a reason why we should not squash one layer here, i.e., move conf.py one level up so that we can write from sage.docs.conf import *?

comment:46 Changed 3 years ago by Isuru Fernando

No reason. I previously had some other files in sage/docs/conf and were removed later.

comment:47 Changed 3 years ago by git

Commit: 863d433d402c9257768929f7d62c4bd478eabbc85c63a8079c96a2ca87d8cea1cace10c7e364e831

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

5c63a80Fix mention of common/conf.py

comment:48 Changed 3 years ago by git

Commit: 5c63a8079c96a2ca87d8cea1cace10c7e364e831edd3ab4efc393d639bb83fe6bd176557c8386fef

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

edd3ab4Replace conf.conf with conf

comment:49 Changed 3 years ago by Julian Rüth

Authors: Julian RüthJulian Rüth, Isuru Fernando
Reviewers: Julian Rüth, Isuru Fernando
Work issues: failing doctestsis the patchbot happy?

@isuruf: Could you have a look at my latest changes? (That I did not test yet, because I have not built recent Sage…)

comment:50 Changed 3 years ago by Isuru Fernando

Looks good to me. Thanks

comment:51 Changed 3 years ago by François Bissey

There is strange stuff about lcalc in the branch. May be some rebasing is needed?

comment:52 Changed 3 years ago by git

Commit: edd3ab4efc393d639bb83fe6bd176557c8386fef2f528be198c8362e06a0f9b7cf547913ceb860c2

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

2f528beRevert lcalc changes

comment:53 Changed 3 years ago by Julian Rüth

Thanks fbissey. That should not have been here.

comment:54 Changed 3 years ago by Isuru Fernando

Branch: u/saraedum/25786u/isuruf/25786
Commit: 2f528be198c8362e06a0f9b7cf547913ceb860c25eec95a03774ea9f2b8e5f4075554f9a3e1a0a05

New commits:

5eec95aFix some pyflakes issues

comment:55 Changed 3 years ago by git

Commit: 5eec95a03774ea9f2b8e5f4075554f9a3e1a0a05a401a171fcfde7ed5e5a128bb6af0967e96b11a8

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

a401a17More pyflakes fixes

comment:56 Changed 3 years ago by François Bissey

I think the patchbot is happy with it (the last run had a failure that should be unrelated). Anything to add or is it completely ready for review?

comment:57 Changed 3 years ago by Isuru Fernando

Anything to add or is it completely ready for review?

Nothing to add. Ready for review

comment:58 Changed 3 years ago by François Bissey

Reviewers: Julian Rüth, Isuru FernandoJulian Rüth, Isuru Fernando, François Bissey
Status: needs_reviewpositive_review

Looks good to me :)

comment:59 Changed 3 years ago by Volker Braun

Branch: u/isuruf/25786a401a171fcfde7ed5e5a128bb6af0967e96b11a8
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.