Opened 12 years ago

Closed 12 years ago

#6187 closed enhancement (fixed)

[with patch, positive review] Fix post-cloning docbuild problems and add new command-line options

Reported by: jhpalmieri Owned by: tba
Priority: major Milestone: sage-4.2
Component: documentation Keywords:
Cc: mhansen, mvngu, davidloeffler Merged in: sage-4.2.alpha0
Authors: John Palmieri, Mitesh Patel Reviewers: John Cremona, Mike Hansen
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mpatel)

Currently, sage -clone requires a rebuild of the Sage documentation. This ticket attempts to fix this and a few related problems with the "docbuild" system. It also includes some features added in the course of fixing the problems:

  • Lots of new command-line options. For a list and examples, try, e.g., sage -docbuild -H. The old options and syntax should still work.
  • Progress updates with Python's powerful logging framework, which we may wish to adopt later for the Sage library.

The only patches to apply are

All but one of the other patches are earlier versions of these. Please do not review the remaining, completely optional patch:

This adds a test document similar to the reference manual. It covers only algebras, so it builds much more quickly and may facilitate experimentation with Sphinx, say. To build it, try sage -docbuild testreference html -jv3.

Related tickets: #5350, #6605, #6614, #6653, #6673, #6488.

Please visit the "diff" links below to view earlier ticket descriptions.

Attachments (13)

cloning_scripts.patch (5.2 KB) - added by jhpalmieri 12 years ago.
cloning_scripts_short.patch (1.8 KB) - added by jhpalmieri 12 years ago.
use this patch or cloning_scripts.patch, but not both -- see my comments
cloning_6187_scripts.patch (2.1 KB) - added by jhpalmieri 12 years ago.
rebased against 4.1.alpha1, use instead of either of the other patches
trac_6187_mpatel.patch (1.7 KB) - added by jhpalmieri 12 years ago.
trac_6187_mpatel_v2.patch (2.6 KB) - added by mpatel 12 years ago.
use this and trac_6187_new_scripts.patch only
trac_6187_new_scripts.patch (2.0 KB) - added by jhpalmieri 12 years ago.
apply to scripts repo. use this and trac_6187_mpatel_v2.patch only.
trac_6187_new_scripts_v3.patch (2.9 KB) - added by mpatel 12 years ago.
Apply this to scripts repo and trac_6187_mpatel_v3.patch to sage repo.
trac_6187_mpatel_v3.patch (2.7 KB) - added by mpatel 12 years ago.
Apply this to sage repo and trac_6187_new_scripts_v3.patch to scripts repo.
trac_6187_testreference.patch (2.8 KB) - added by mpatel 12 years ago.
Adds a document called 'testreference' for testing the reference builder. It's non-essential and should be independent of the other patches.
trac_6187_new_scripts_v4.patch (2.9 KB) - added by mpatel 12 years ago.
Apply to scripts repo. Apply trac_6187_builder_v4.patch to the sage repo.
trac_6187_builder_v4.patch (25.3 KB) - added by mpatel 12 years ago.
Apply to sage repo. Apply trac_6187_new_scripts_v4.patch to the scripts repo.
trac_6187-new_scripts_v5.patch (2.9 KB) - added by mpatel 12 years ago.
Updated for #6673. Apply to scripts repo.
trac_6187-builder_v5.patch (25.3 KB) - added by mpatel 12 years ago.
Fixed sys.exit() code. Apply to sage repo.

Download all attachments as: .zip

Change History (44)

comment:1 Changed 12 years ago by mhansen

I think the file modification times need to be copied over as well with a "cp -a" instead of "cp -r". I'm not sure if there are any hardcoded paths in the Sphinx environment pickle that would need to be changed.

comment:2 Changed 12 years ago by jhpalmieri

I'm not having any luck with this. If in sage-clone I copy the library files with "cp -a" (or what I think is the equivalent, e.g. "cp -pr" on my mac, or shutil.copytree("sage/build","%s/build"%branch)), then when it gets to the sage -b part of things, it rebuilds all of the cython files, so cloning takes way too long.

No matter what, if after cloning, I delete the directory doc/output and then recopy it, sage -docbuild reference html rebuilds everything. This happens whether the modification times are preserved or not: even if the times are not preserved, so everything in the output directory is newer than everything in the build directory, Sphinx seems to think that the files have changed and so need to be rebuilt. Same thing if I recopy the entire "doc" directory. So I'm confused.

comment:3 Changed 12 years ago by burcin

#5350 might be relevant here. The script there could be adapted to create hard links for the documentation.

comment:4 Changed 12 years ago by jhpalmieri

This is kind of brutal, but we can replace the cloning part of sage-clone with a single line like

cmd = 'cp -pr sage %s'%branch

(This is with the BSD version of cp on Mac OS X; it might be more portable to use a python equivalent, like shutil.copytree.)

This has the disadvantage that some crap gets copied along with the good stuff. This is probably a bad idea from other points of view, too; what else goes wrong?

It has the advantage that modification times are preserved, while they are not (as far as I can tell) when you use 'hg clone' to copy the repository.

By the way, the 'clone' section of the hg man page says:

In some cases, you can clone repositories and checked out files
using full hardlinks with
$ cp -al REPO REPOCLONE
This is the fastest way to clone, but it is not always safe. The
operation is not atomic (making sure REPO is not modified during
the operation is up to you) and you have to make sure your editor
breaks hardlinks (Emacs and most Linux Kernel tools do so). Also,
this is not compatible with certain extensions that place their
metadata under the .hg directory, such as mq.

This is where I got the idea, although their version creates hard links, which I suppose is why it would be "the fastest way"...

comment:5 Changed 12 years ago by jhpalmieri

One issue to keep in mind when testing: if you do

sage -docbuild --jsmath reference html

as is done at the end of 'make', and then you do

sage -docbuild reference html

the entire reference manual gets rebuilt (and similarly for the other pieces of documentation). So if you're testing out an idea for how to fix this, make sure that you're consistent with the --jsmath setting before and after cloning.

comment:6 Changed 12 years ago by jhpalmieri

  • Summary changed from After making a clone, the reference manual (and other docs) should not have to be completely rebuilt. to [with patch, needs review] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt.

This patch might very well be a bad idea, but I don't know enough about the Sage cloning process to know any better. Please review it carefully.

(Actually, I'm posting two patches. 'cloning_scripts_short.patch' just copies over the whole Sage library. 'cloning_scripts.patch' uses the Python 2.6 version of shutil.copytree to allow us to skip certain files when copying, so I copied the source code for that into sage-clone and used that, thereby not copying absolutely everything. If I'm skipping too many things or not enough things, this is easy to adjust. Only apply one of these two patches.)

Changed 12 years ago by jhpalmieri

Changed 12 years ago by jhpalmieri

use this patch or cloning_scripts.patch, but not both -- see my comments

comment:7 Changed 12 years ago by cremona

I have read the comments here with interest, and could try out those patches, but I'm not qualified to say whether they work "well" or are robust enough to be released -- sorry!

comment:8 Changed 12 years ago by jhpalmieri

Here's a patch rebased against 4.1.alpha1.

Changed 12 years ago by jhpalmieri

rebased against 4.1.alpha1, use instead of either of the other patches

comment:9 Changed 12 years ago by cremona

  • Authors set to John Palmieri
  • Reviewers set to John Cremona
  • Summary changed from [with patch, needs review] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt. to [with patch, with positive review] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt.

Here's what I did. 1. Starting in main branch of a 4.1.alpha2 build, did "sage -docbuild all html" and waited until it finished. 2. Made a clone called test1. 3. In that clone, did "sage -docbuild again" and watched it build all over again. 4. In the clone, applied the third patch. 5. From that clone made a new clone test2. 6. In the new clone once again did "sage -docbuild all html" -- and nothing was rebuilt!

That looks like success to me. The new script also looks very much simpler than the old one, which is good.

This is on linux (ubuntu 32-bit); since you use a standard python utility for the copying I would hope that it would work anywhere, so I'm giving it a positive review and hope that others will try on other systems.

comment:10 Changed 12 years ago by mpatel

The following may help with rebuilding the reference manual after cloning:

  • Start with v4.0.2, or a vanilla sage-clone script, at least.
  • Apply
    diff --git a/doc/common/builder.py b/doc/common/builder.py
    --- a/doc/common/builder.py
    +++ b/doc/common/builder.py
    @@ -353,6 +353,16 @@ class ReferenceBuilder(DocBuilder):
             if os.path.exists(_sage):
                 copytree(_sage, os.path.join(self.dir, 'sage'))
                     
    +        # After "sage -clone", refresh the .rst file mtimes in
    +        # environment.pickle.
    +        if update_mtimes:
    +            import time
    +            env = self.get_sphinx_environment()
    +            for doc in env.all_docs:
    +                env.all_docs[doc] = time.time()
    +            env_pickle = os.path.join(self._doctrees_dir(), 'environment.pickle')
    +            env.topickle(env_pickle)
    +
             getattr(DocBuilder, build_type)(self, *args, **kwds)
         
         def cache_filename(self):
    @@ -645,6 +655,8 @@ def help_message():
     parser = optparse.OptionParser(usage="usage: sage -docbuild [options] name type")
     parser.add_option("--jsmath", action="store_true",
                       help="render math using jsMath")
    +parser.add_option("--update_mtimes", action="store_true",
    +                  help='update .rst file mtimes (e.g., after "sage -clone")')
     parser.print_help = help_message
     
     if __name__ == '__main__':
    @@ -653,6 +665,11 @@ if __name__ == '__main__':
         if options.jsmath:
             os.environ['SAGE_DOC_JSMATH'] = "True"
     
    +    if options.update_mtimes:
    +        update_mtimes = True
    +    else:
    +        update_mtimes = False
    +
         #Get the name of the document we are trying to build
         try:
             name, type = args
    
  • Do sage -clone foo or, e.g., cd $SAGE_ROOT/devel/sage-foo; cp -pr ../sage-bar/doc .
  • Update builder.py, if necessary.
  • Do sage -docbuild reference html --update_mtimes --jsmath
  • Do sage -docbuild reference html --jsmath

Note: I haven't tested this idea with #5350, other documents, other builders, etc.

comment:11 Changed 12 years ago by jhpalmieri

  • Summary changed from [with patch, with positive review] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt. to [with patch, needs review] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt.

So we could add sage -docbuild reference html --update_mtimes --jsmath to the sage-clone script, right?

The only issue I see is that if someone has made some changes and then clones before rebuilding the reference manual, the -update_mtimes option tells Sage/Sphinx? that the ref manual page doesn't need to be changed. I guess we can add a warning to sage-clone like the warning already there.

This works with or without the patch at #5350, and it works appropriately with or without the --jsmath switch. It has no effect on other documents, as one would expect, but I think that's fine. Same for the other builders.

Here are two new patches, one which is just mpatel's patch to builder.py (and which receives a positive review from me). The other adds some code to sage-clone to do the update_mtimes thing to the reference manual; it tries to figure out whether to use --jsmath or not; is there a better way to do this than what I have? It also prints a brief message about the reference manual. The only thing that needs to be reviewed is this second patch (to the scripts repository).

I like this version better than my old patch: it keeps the old cloning process, so it seems safer to me. So I'm changing this from "positive review" (for the old patch) to "needs review" (for the new scripts patch).

Apply "trac_6187_mpatel.patch" to the sage repository, and apply "trac_6187_new_scripts.patch" to the scripts repository.

Changed 12 years ago by jhpalmieri

comment:12 follow-up: Changed 12 years ago by mpatel

I noticed an error occurs during cloning if environment.pickle doesn't already exist.

I'll try to make a new patch for builder.py, after I build 4.1.alpha2.

Changed 12 years ago by mpatel

use this and trac_6187_new_scripts.patch only

comment:13 in reply to: ↑ 12 Changed 12 years ago by mpatel

Replying to mpatel:

I noticed an error occurs during cloning if environment.pickle doesn't already exist.

The new patch should now work in this case.

The pickle's srcdir attribute contains what appears to be the only trace of the name of the cloned (i.e., previous) branch. This can become an issue when branches are renamed or deleted, as Sphinx will throw an OSError. The new patch now tells Sphinx to use the sym-linked '.../devel/sage/...'

I'll try to make a new patch for builder.py, after I build 4.1.alpha2.

Apply [trac_6187_new_scripts.patch] to the scripts repository and [trac_6187_mpatel_v2.patch] to the sage repository.

comment:14 Changed 12 years ago by jhpalmieri

  • Authors changed from John Palmieri to John Palmieri, Mitesh Patel

What should the output from 'sage -docbuild --update_mtimes reference html' look like? Right now I see

sphinx-build -b html -d /Applications/sage/devel/sage/doc/output/doctrees/en/reference    /Applications/sage/devel/sage/doc/en/reference /Applications/sage/devel/sage/doc/output/html/en/reference
Sphinx v0.5.1, building html
loading pickled environment... done
building [html]: targets for 0 source files that are out of date
updating environment: 0 added, 0 changed, 0 removed
no targets are out of date.
Build finished.  The built documents can be found in /Applications/sage/devel/sage/doc/output/html/en/reference

Since it's not actually building anything (is it?), I think this is misleading. In the new scripts patch, I've changed sage-clone so that it suppresses the standard output from this command.

Changed 12 years ago by jhpalmieri

apply to scripts repo. use this and trac_6187_mpatel_v2.patch only.

Changed 12 years ago by mpatel

Apply this to scripts repo and trac_6187_mpatel_v3.patch to sage repo.

Changed 12 years ago by mpatel

Apply this to sage repo and trac_6187_new_scripts_v3.patch to scripts repo.

comment:15 Changed 12 years ago by mpatel

I've attached new versions of both patches. Apply only the v3 pair to the indicated repositories. Changes:

  • sage-clone now copies any existing auto-generated .rst files to the new branch, preserving modification times.
  • builder.py now updates the pickle first, since otherwise it always regenerates existing .rst files after cloning.
  • jsMath --> jsmath in sage-clone.

It's OK to delete existing auto-generated .rst files prior to cloning, as long as doc/output/doctrees/en/reference/reference.pickle disappears, too. The patches don't cover this case, since I think it occurs rarely in practice. I conjecture that most of the time, the update_mtimes line doesn't actually build anything, but I've been wrong before.

comment:16 Changed 12 years ago by mpatel

  • Summary changed from [with patch, needs review] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt. to [with patch, needs work] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt.

Changed 12 years ago by mpatel

Adds a document called 'testreference' for testing the reference builder. It's non-essential and should be independent of the other patches.

Changed 12 years ago by mpatel

Apply to scripts repo. Apply trac_6187_builder_v4.patch to the sage repo.

Changed 12 years ago by mpatel

Apply to sage repo. Apply trac_6187_new_scripts_v4.patch to the scripts repo.

comment:17 Changed 12 years ago by mpatel

  • Cc mhansen mvngu davidloeffler added

Apply only the v4 pair to the appropriate repositories. The testreference patch just adds a non-trivial test document that should build quickly. It may give an indication, at least, of the behavior to expect when building the full reference manual.

Changes in v4:

  • Reordering of some code and somewhat better case / exception handling in the reference builder.
  • First steps with Python's logging framework. It's powerful and easy to use, at least inside one module. One of Sphinx's utility modules colorizes some of the output.
  • More aggressive use of Python's optparse module, in order to provide several new command-line options (cf. #6488). The previous syntax should still work, however.
  • sage-clone should now be compatible with #6512. --update_mtimes is now --update-mtimes.

I've focused on the reference manual, but I hope the new pieces make it easier to improve the docbuild system for the other documents, too. Feel free to make constructive comments, as well as changes. In particular, we should settle on the option names.

Anyway, if possible, please test the patches in multiple real-world scenarios, including those mentioned on sage-devel. I'd like to take care of all known, relevant "ref-cloning" issues (cf. #5350) before this ticket closes. It is likely we're not there yet.

I've added a few recipients to the CC list. I hope that's OK.

comment:18 Changed 12 years ago by mpatel

I should add that I selected capital letters for listing commands (-C), formats (-F), and documents (-D), so that we could use the corresponding lower-case letters, if it's desired, to carry out multiple actions, e.g.,

sage -docbuild -d reference,tutorial -f pdf,html -jv2

Is the command-format distinction useful? We could add commands to "pre-process" groups of docstrings, e.g., to check for and enforce style conventions or to search for cross-references that point to a given object. However, I'm not very familiar with actually writing ReST docstrings.

comment:19 Changed 12 years ago by mpatel

  • Summary changed from [with patch, needs work] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt. to [with patch, needs review] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt.

Run, e.g., sage -docbuild to see a "short" help message and sage -docbuild -H to see an extended message, including several examples. Please feel free to edit any parts to make the system easier to use. Should I post to sage-devel about the new options?

I'm changing the status of this ticket to WPNR to signal that the patches are [again] ready for testing, comment, and perhaps even review.

comment:20 Changed 12 years ago by mpatel

To see colored logging output, try sage -docbuild testreference html -jv3 -S -E, say. I haven't colorized the help messages, since the user's terminal may not support ANSI color escape sequences.

comment:21 Changed 12 years ago by jhpalmieri

Reminder: Make sure to include something like the patch at #6488 (documenting "--jsmath").

When this ticket is closed, close #6488 as well.

comment:22 Changed 12 years ago by jhpalmieri

Overall, this works as advertised. I like the new options, the new option parsing, and the help messages. We might consider eventually updating mtimes on all of the docs, not just the reference manual, but everything else is quick enoug to build that it's not a big deal. (To test: I installed the two patches here and made sure the docs were built. Then I cloned the current repository and rebuilt the docs. The reference manual built almost instantly, because it was using the version from the clone.)

Someone else should take a good look at it, though, since I am an author of part of this (the scripts part). Also, there are a number of changes to builder.py, and other people should look carefully at them, more carefully than I have so far.

Other comments: if I do something like sage -docbuild hello html, then it says

sphinx-build -b html -d /Applications/sage/devel/sage/doc/output/doctrees/en/hello    /Applications/sage/devel/sage/doc/en/hello /Applications/sage/devel/sage/doc/output/html/en/hello
Error: Source directory doesn't contain conf.py file.
Build finished.  The built documents can be found in /Applications/sage/devel/sage/doc/output/html/en/hello

The problem is, it creates a directory SAGE_ROOT/devel/sage/doc/en/hello, and now "hello" appears in the list of documents. Should there be better error checking to prevent this from happening?

Actually, I think this belongs on another ticket: it's a "pre-existing condition", and can be dealt with separately. See #6605.

comment:23 follow-up: Changed 12 years ago by mpatel

A reminder to myself: Change sys.exit(0) to sys.exit(1).

comment:24 in reply to: ↑ 23 Changed 12 years ago by mpatel

Replying to mpatel:

A reminder to myself: Change sys.exit(0) to sys.exit(1).

Also: Update the scripts patch, if necessary, for consistency with #6614.

Changed 12 years ago by mpatel

Updated for #6673. Apply to scripts repo.

Changed 12 years ago by mpatel

Fixed sys.exit() code. Apply to sage repo.

comment:25 Changed 12 years ago by mpatel

V5 just squares sage-clone with #6673 and changes sys.exit(0) to sys.exit(1) in one instance in builder.py.

comment:26 Changed 12 years ago by cremona

It's asking a lot of a reviewer to apply over a dozen patches! Any chance of combining them all into one?

comment:27 Changed 12 years ago by mpatel

I apologize for not being explicit. Please apply just the v5 pair: the "new_scripts" patch to the scripts repository and the "builder" patch to the sage repository.

comment:28 Changed 12 years ago by mpatel

  • Description modified (diff)
  • Summary changed from [with patch, needs review] After making a clone, the reference manual (and other docs) should not have to be completely rebuilt. to [with patch, needs review] Fix post-cloning docbuild problems and add new command-line options

comment:29 Changed 12 years ago by mpatel

  • Description modified (diff)

comment:30 Changed 12 years ago by mhansen

  • Reviewers changed from John Cremona to John Cremona, Mike Hansen
  • Status changed from needs_review to positive_review
  • Summary changed from [with patch, needs review] Fix post-cloning docbuild problems and add new command-line options to [with patch, positive review] Fix post-cloning docbuild problems and add new command-line options

After look at this and testing it out, I think that it can go in.

comment:31 Changed 12 years ago by mhansen

  • Merged in set to sage-4.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.