Opened 6 years ago

Closed 6 years ago

#17369 closed defect (fixed)

sphinx-build is broken

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.5
Component: build Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: 69337a5 (Commits) Commit: 69337a587a9eb7b2fa17fe4f870b987d0434fcdf
Dependencies: Stopgaps:

Description

The script $SAGE_LOCAL/bin/sphinx-build (which is copied over from build/pkgs/sphinx/patches/sphinx-build) is broken since it refers to the wrong version.

The Sage documentation build no longer uses the sphinx-build script, so this wasn't noticed immediately. Manually calling this script gives

Traceback (most recent call last):
  File "/usr/local/src/sage-git/local/bin/sphinx-build", line 5, in <module>
    from pkg_resources import load_entry_point
  File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 2817, in <module>
  File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 451, in _build_master
  File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 464, in _build_from_requirements
  File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 639, in resolve
pkg_resources.DistributionNotFound: Sphinx==1.1.2
make: *** [html] Error 1

Change History (20)

comment:1 Changed 6 years ago by kcrisman

I guess this could be fixed by

  • build/pkgs/sphinx/patches/sphinx-build

    diff --git a/build/pkgs/sphinx/patches/sphinx-build b/build/pkgs/sphinx/patches/sphinx-build
    index a20792f..46e7a1d 100755
    a b  
    11#!/usr/bin/env python
    2 # EASY-INSTALL-ENTRY-SCRIPT: 'Sphinx==1.1.2','console_scripts','sphinx-build'
     2# EASY-INSTALL-ENTRY-SCRIPT: 'Sphinx==1.2.2','console_scripts','sphinx-build'
    33__requires__ = 'Sphinx==1.1.2'
    44import sys
    55from pkg_resources import load_entry_point
    66import sage.all
    77sys.exit(
    8    load_entry_point('Sphinx==1.1.2', 'console_scripts', 'sphinx-build')()
     8   load_entry_point('Sphinx==1.2.2', 'console_scripts', 'sphinx-build')()
    99)
Last edited 6 years ago by kcrisman (previous) (diff)

comment:2 Changed 6 years ago by kcrisman

Is there any way to doctest this, or possibly at least fix SPKG.txt to say so?

Haha! It's ALREADY THERE!!!

 * Make sure to update the Sphinx version number in the file
   patches/sphinx-build.

Sigh.

comment:3 Changed 6 years ago by kcrisman

Or maybe the script could be modified to grab the number in build/pkgs/sphinx/package-version.txt automatically, before being copied? That would be more future-proof.

comment:4 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/17369
  • Created changed from 11/19/14 15:57:37 to 11/19/14 15:57:37
  • Modified changed from 11/19/14 16:09:10 to 11/19/14 16:09:10

comment:5 Changed 6 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to 69337a587a9eb7b2fa17fe4f870b987d0434fcdf
  • Status changed from new to needs_review

New commits:

69337a5Stop patching sphinx-build

comment:6 follow-up: Changed 6 years ago by kcrisman

?? But how will a brand-new Sage install now get sphinx-build? Is it part of setup.py install somehow?

comment:7 in reply to: ↑ 6 Changed 6 years ago by jdemeyer

Replying to kcrisman:

But how will a brand-new Sage install now get sphinx-build? Is it part of setup.py install somehow?

Yes indeed!

comment:8 follow-up: Changed 6 years ago by kcrisman

Huh.

But the salient features are rather different.

import sys 
from pkg_resources import load_entry_point 
import sage.all 

Will being in sage -sh be enough for that? As opposed to

from sphinx import main
import sys

which is the probable effect of sphinx-build? There must have been a reason to have the custom version of the script.

comment:9 Changed 6 years ago by jdemeyer

For the record, the patching of sphinx-build was introduced in #4737, which has very little info...

comment:10 in reply to: ↑ 8 Changed 6 years ago by jdemeyer

Replying to kcrisman:

There must have been a reason to have the custom version of the script.

I have no idea really. But for #17265 I tested this ticket: sphinx-build seems to work without the import sage.all...

comment:11 Changed 6 years ago by kcrisman

I was just about to ask you that question. Otherwise the script setup.py creates is indeed identical to the one here.

comment:12 Changed 6 years ago by kcrisman

And I can confirm what you say about #17265. Huh.

comment:13 follow-up: Changed 6 years ago by kcrisman

Can you think of anything else sphinx-build would be used for now? I do know that while building the documentation now, if a given module doesn't exist in Sage (say, some stale files from switching branches) that it complains that it can't find the module and one has to delete the generated .rst by hand. (That is probably a bug that those aren't destroyed after having been auto-generated and then the original file is gone, by the way, but not for this ticket.) So perhaps at some point this functionality is necessary, even if this script is no longer used for building the Sage doc per se. Why does Sphinx have to know that the module exists to build the whole documentation?

If we can resolve that, otherwise this is fine and has a couple other good cleanups.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 6 years ago by jdemeyer

Replying to kcrisman:

I do know that while building the documentation now, if a given module doesn't exist in Sage (say, some stale files from switching branches) that it complains that it can't find the module and one has to delete the generated .rst by hand.

What makes you think that this has to do with the sphinx-build script?

comment:15 in reply to: ↑ 14 Changed 6 years ago by kcrisman

I do know that while building the documentation now, if a given module doesn't exist in Sage (say, some stale files from switching branches) that it complains that it can't find the module and one has to delete the generated .rst by hand.

What makes you think that this has to do with the sphinx-build script?

I don't think it has to do with that, or at least I believe you when you say it doesn't (above). But it does mean that, at least at certain times, Sphinx wants to try to import Sage modules when it builds the documentation, and blows up if it can't. So I'm wondering whether there might be times when that type of behavior, via sphinx-build, would affect whatever remaining uses of sphinx-build there are. Perhaps there is no such behavior, but I'm trying to brainstorm.

comment:16 Changed 6 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

Okay, here is our src/doc/common/custom-sphinx-build.py

"""
This is Sage's version of the sphinx-build script

Enhancements are:

* import the Sage library to access the docstrings, otherwise doc
  buliding doesn't work.

* redirect stdout to our own logger, and remove some unwanted chatter.
"""

Although this is actually now in builder.py, and in any case the magic place this is needed is there too -

    def get_module_docstring_title(self, module_name):
        """
        Returns the title of the module from its docstring.
        """
        #Try to import the module
        try:
            __import__(module_name)
        except ImportError as err:
            logger.error("Warning: Could not import %s %s", module_name, err)
            return "UNABLE TO IMPORT MODULE"

So as long as we aren't trying to auto-generate stuff with autodoc in these we should be cool.

comment:17 Changed 6 years ago by jhpalmieri

  • Reviewers Karl-Dieter Crisman deleted

For what it's worth, this also seems to work with sage --docbuild file=... html. I can't think of any reasons why we would need to import sage.all in this file.

comment:18 Changed 6 years ago by kcrisman

? I think the point is that the sphinx-build which is in the patches directory of sphinx is no longer needed because we don't need to import sage.all any more for that, since it's not the one used to build Sage doc. The version in src/doc/common refers to it, but actually the import is in builder.py, and there it is indeed necessary so that autodoc can do its stuff - or if not, maybe to raise an error earlier?

Anyway, if the files in src/doc/common can be modified, let's make that a different ticket.

comment:19 Changed 6 years ago by jhpalmieri

  • Reviewers set to Karl-Dieter Crisman

I didn't mean to delete you as a reviewer. Sorry about that.

I meant that the changes in this ticket look good to me: there is no need to import sage.all in the sphinx-build script (that's what I meant by "this file"), so we should go back to using the standard version of that script.

comment:20 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/17369 to 69337a587a9eb7b2fa17fe4f870b987d0434fcdf
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.