Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move runtime documentation python modules into src/sage #21732

Closed
infinity0 mannequin opened this issue Oct 19, 2016 · 63 comments
Closed

Move runtime documentation python modules into src/sage #21732

infinity0 mannequin opened this issue Oct 19, 2016 · 63 comments

Comments

@infinity0
Copy link
Mannequin

infinity0 mannequin commented Oct 19, 2016

Currently, (1) at runtime sage requires some files from under SAGE_DOC_SRC. For example:

$ git grep SAGE_DOC_SRC -- src/sage
[..]
src/sage/misc/sagedoc.py:    sys.path = [os.path.join(SAGE_DOC_SRC, 'common')] + oldpath
[..]
src/sage/misc/sphinxify.py:    confdir = os.path.join(SAGE_DOC_SRC, 'en', 'introspect')

However, this is awkward for binary distros such as Debian, who don't install source code onto end user machines by default. Indeed, even Sage's own Makefiles don't install these files into SAGE_LOCAL, even though they are a runtime necessity.

In addition to this, (2) there are many cases where Sage tracks SAGE_DOC_SRC and hacks it into sys.path via os.environ, which is not very clean.

This ticket proposes the following change (or something similar)

  • src/doc/common/* -> src/sage/docs/common/*
  • src/doc/en/introspect/* -> src/sage/docs/introspect/*

This would solve the above two issues - instead of (2) one can just do from sage.doc.common import conf or from sage.doc.introspect import conf, and (1) is taken care of automatically because everything under src/sage is installed as standard python modules.

One could also remove the OMIT = ["introspect"] exception in src/sage_setup/docbuild/build_options.py.

Does this sound sensible? If so I can do this for our Debian packaging already, test it, and send in an initial patch.

Depends on #22611
Depends on #22655

CC: @hivert @isuruf @kiwifb @jhpalmieri @antonio-rojas

Component: documentation

Keywords: days85

Author: Erik Bray

Branch/Commit: u/embray/ticket-21732 @ 77b5428

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/21732

@infinity0 infinity0 mannequin added this to the sage-7.5 milestone Oct 19, 2016
@jdemeyer
Copy link

comment:1

Putting the documentation as top-level directory of the project is the standard thing to do with Sphinx. We aren't quite using Sphinx in the standard way as there is a lot of Sage customization, but still... I see no reason to be even more different.

Just saying "it would make our lives easier to do X" is not a good explanation. You really need to justify better why you want to do X.

@jdemeyer jdemeyer changed the title Move runtime documentation python modules into sagelib Move documentation sources into src/sage Oct 20, 2016
@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Oct 20, 2016

comment:2

Why is reducing development cost not a good explanation? None of the files I mentioned are actual direct sphinx config files, they are all meant to be included or used by something else. So they are already not being used in a "standard Sphinx way".

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Oct 20, 2016

comment:3

Changing the title back because the ticket is not about moving all documentation sources.

@infinity0 infinity0 mannequin changed the title Move documentation sources into src/sage Move runtime documentation python modules into src/sage Oct 20, 2016
@jdemeyer
Copy link

comment:4

Replying to @infinity0:

Why is reducing development cost not a good explanation?

If you use an argument of the form "I want X because it helps Y" and we both agree that Y is a good thing, you still need to explain why X helps with Y. And I am totally missing that.

And this title is totally confusing me, I hardly see how it relates to moving src/doc/common. It might relate to moving the introspect doc, but then the title covers only a part of the ticket.

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Oct 20, 2016

comment:5

It reduces development cost because you no longer have to track SAGE_DOC_SRC in many places and hack it into sys.path from os.environ. You can just do from sage.doc.common import conf or from sage.doc.introspect import conf. This helps with tickets like #21495 which is what we ideally need in Debian (but currently need to hack the equivalent functionality in via patches).

In additional to reducing development cost, this change (or something similar) is necessary for binary distributions because we normally don't install things like SAGE_SRC or SAGE_DOC_SRC onto end user machines, yet the files I mentioned are needed at runtime - and only these files, not the other conf.py files or doc sources.

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Oct 20, 2016

comment:6
$ git grep SAGE_DOC_SRC -- src/sage
[..]
src/sage/misc/sagedoc.py:    sys.path = [os.path.join(SAGE_DOC_SRC, 'common')] + oldpath
[..]
src/sage/misc/sphinxify.py:    confdir = os.path.join(SAGE_DOC_SRC, 'en', 'introspect')

are the relevant lines.

@jdemeyer
Copy link

comment:7

Replying to @infinity0:

It reduces development cost because you no longer have to track SAGE_DOC_SRC in many places and hack it into sys.path from os.environ. You can just do from sage.doc.common import conf or from sage.doc.introspect import conf. This helps with tickets like #21495 which is what we ideally need in Debian (but currently need to hack the equivalent functionality in via patches).

This explanation should be in the ticket description. There is no way I could figure out the above paragraph from just reading the description.

@infinity0

This comment has been minimized.

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Oct 20, 2016

comment:8

Alright, sorry, I'll take some more time writing future tickets. I've edited the description now, hopefully it's OK.

@jdemeyer
Copy link

comment:9

At least I understand the problem now. Still, I don't know the best way to fix it.

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 23, 2016

comment:11

I agree that sage at runtime should not refer to anything in SAGE_DOC_SRC.
Whatever is needed from there should be installed in an appropriate place in SAGE_LOCAL.

@embray
Copy link
Contributor

embray commented Dec 7, 2016

comment:12

I noticed this recently too--glad to see there's already an issue for it. I agree, using a location in SAGE_LOCAL would be best.

@embray
Copy link
Contributor

embray commented Dec 12, 2016

comment:13

I'm starting to agree it would be good if this common doc stuff just went in a sage.misc.docs sub-package or something. As it is common/conf.py imports from sage so it already has a dependency on it anyways.

This would be a good move toward the previously-discussed ideal of making it easier for third-party packages to use Sage's documentation utilities.

Another options which I've brought up before is to move all docs-related utilities to a separate sage_documention package (to which sage_setup.docbuild would also be moved). This would necessarily be a runtime dependency of sage for docstring sphinxifying to work.

@embray
Copy link
Contributor

embray commented Dec 15, 2016

Dependencies: #22061

@embray
Copy link
Contributor

embray commented Dec 15, 2016

comment:14

I'm working on a possible solution to this, but I believe that any reasonable solution should depend on #22061 at a minimum.

@embray
Copy link
Contributor

embray commented Dec 16, 2016

Branch: u/embray/ticket-21732

@embray
Copy link
Contributor

embray commented Dec 16, 2016

Commit: 6194785

@embray
Copy link
Contributor

embray commented Dec 16, 2016

Author: Erik Bray

@embray
Copy link
Contributor

embray commented Dec 16, 2016

comment:15

Here's my attempt at solving this. It included a few other changes that I felt were needed (in addition to #22061), but that are not directly relevant either, so might be worth moving to another ticket.

I don't feel strongly about the choice of sage.misc.docs, but I am convinced that these files should be somewhere (for now) in the sage package.


New commits:

9175036Don't hard-code (in the form of a symlink) the path to thebe.js.
ba840c6A couple enhancements to find_extra_files:
9e1bdc3Adds a sage_build_py command in setup.py
334da54Fix some tests that broken in sage_setup
2983351Fully move src/doc/common and src/doc/en/introspect to subpackages of a new sage.misc.docs subpackage.
31a3989Fix no longer accurate reference to SAGE_DOC_SRC in a docstring
6194785As mentioned in #21732, 'introspect' is no longer needed in OMIT, as 'introspect' is no longer in the doc source tree.

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Dec 16, 2016

comment:17

In here https://github.com/sagemath/sagetrac-mirror/blob/2983351b1116ed29f08d5fc95c6c11dbcddeec64/src/sage/misc/sphinxify.py and possible some of the other changes, don't we want SAGE_LOCAL instead of SAGE_SRC? (Assuming we don't want to always install SAGE_SRC at runtime.)

@infinity0
Copy link
Mannequin Author

infinity0 mannequin commented Dec 16, 2016

comment:18

er, I mean SAGE_LIB instead of SAGE_LOCAL (or SAGE_SRC)

@embray
Copy link
Contributor

embray commented Dec 19, 2016

comment:19

Replying to @infinity0:

In here https://github.com/sagemath/sagetrac-mirror/blob/2983351b1116ed29f08d5fc95c6c11dbcddeec64/src/sage/misc/sphinxify.py and possible some of the other changes, don't we want SAGE_LOCAL instead of SAGE_SRC? (Assuming we don't want to always install SAGE_SRC at runtime.)

Yes, this is true. Actually, I have another branch where I'm trying to improve runtime dependency on SAGE_SRC (or, more specifically, SAGE_SRC is set to the same as SAGE_LIB when not running out of the source tree).

I guess this change is a holdover from that. But at the same time I did this work as a prerequisite for fixing the other runtime dependencies on SAGE_DOC_SRC and SAGE_SRC, so I guess there's an interdependency between the two.

Maybe, for the sake of making this ticket make sense on its own, I'll bring in some of the changes from my other branch too.

@kcrisman
Copy link
Member

comment:20

See also sagenb PR 416. Perhaps an update ticket for that is needed too.

@jdemeyer
Copy link

comment:43

Replying to @kiwifb:

I knew I had forgotten something. There is a lot of stuff in sage/misc I think stuff is coherent enough that it could go in sage/docs rather than sage/misc/docs

I agree but let's do that in a new ticket. There is already too much happening in this ticket. I suggest to move the files from src/doc to src/sage/docs but to keep the existing doc-related files in src/sage/misc for now.

@embray
Copy link
Contributor

embray commented Mar 15, 2017

comment:44

Okay, well, there were definitely good reasons for the other changes, but it was long enough ago now that I don't remember what they all were. Let me see if I can break it off to a separate ticket where I explain them better.

Moving module_list.py might not have been necessary, but I don't recall (eventually I think it should go away entirely but that's another subject).

@embray
Copy link
Contributor

embray commented Mar 20, 2017

comment:45

jdemeyer: You had mentioned that there was some other work you were doing that this ticket should depend on? Is there a branch for that posted yet, that I could base on? Or is mainly just a matter of naming the package in sage sage.doc (or is it sage.docs)?

@jdemeyer
Copy link

comment:46

Replying to @embray:

jdemeyer: You had mentioned that there was some other work you were doing that this ticket should depend on? Is there a branch for that posted yet, that I could base on? Or is mainly just a matter of naming the package in sage sage.doc (or is it sage.docs)?

There is at least #22611, which creates the sage.docs directory.

You should also be aware of #22252 which affects docbuilding. Depending on how well git can deal with renames, this will probably not actually conflict.

@jdemeyer
Copy link

Changed dependencies from #22061 to #22611

@embray
Copy link
Contributor

embray commented Mar 20, 2017

comment:47

Right, I think you had mentioned both of those. Thanks!

@embray

This comment has been minimized.

@embray
Copy link
Contributor

embray commented Mar 21, 2017

comment:48

New branch for this ticket based on #22655, which separates out the build changes to be considered separately. This now just makes the code moves suggested in the description of the ticket.

To be clear, nothing substantive changed in these files except to update some imports, and remove bits of code that were previously needed to manipulate sys.path. Now functions like sphinxify can work without SAGE_DOC_SRC needing to be installed somewhere on the system.


New commits:

e404a68A couple enhancements to find_extra_files:
5d30652Adds a sage_build_py command in setup.py
77b5428Move common documentation config and template files from `src/doc` to a new

@embray
Copy link
Contributor

embray commented Mar 21, 2017

Changed branch from u/hivert/ticket-21732 to u/embray/ticket-21732

@embray
Copy link
Contributor

embray commented Mar 21, 2017

Changed dependencies from #22611 to #22611 #22655

@embray
Copy link
Contributor

embray commented Mar 21, 2017

Changed commit from 1e372a8 to 77b5428

@embray
Copy link
Contributor

embray commented Mar 21, 2017

comment:49

Also, as already noted, this will likely conflict with #22611, but I'm fine waiting for that to be merged first.

@fchapoton
Copy link
Contributor

comment:51

does not apply

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 11, 2020

comment:52

This ticket appears to be outdated, as #25786 (Fix introspection with ? when doc source is not available) and other tickets seem to have done the described moves. I propose to close this ticket.

@mkoeppe mkoeppe removed this from the sage-7.6 milestone Feb 11, 2020
@jhpalmieri
Copy link
Member

comment:53

It looks okay to me to close this, but the distro people should take a look.

@kiwifb
Copy link
Member

kiwifb commented Feb 12, 2020

comment:54

I used to have to move python files in sage-on-gentoo to make the doc work. Not anymore. Although I still copy doc/common/themes in place so there may still be something to do. For some reason we seem to produce a lot of cruft and strange things in the doc on distros but that should be a different ticket.

@kiwifb
Copy link
Member

kiwifb commented Feb 12, 2020

Reviewer: François Bissey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants