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

Add support for autobuilding -M within Sphinx #151

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tim-nordell-nimbelink
Copy link

sphinx-build has a hiddenish entry point (it doesn't show up in sphinx-build --help) that is used for some build targets (see https://github.com/sphinx-doc/sphinx/blob/master/sphinx/cmd/build.py#L381-L384). It's useful to be able to autobuild these, like -M latexpdf, beyond autobuilding the HTML targets. There are PDF viewing programs that will autoreload when the PDF underneath changes, so this can be very useful for previewing the PDF output when one makes changes.

Additionally, the second patch allows multiple file changes to be consolidated into one build invocation of sphinx-build (at least while running with pyinotify installed which is automatically used by the livereload package if present). This can occur when changing branches, for instance. This is especially visible when doing a latexpdf build since it takes quite a bit longer than refreshing individual HTML files and it always fully rebuilds the project instead of individual files like a html build does.

When one multiple files within a repository at once (e.g. changing
branches for instance), the live view will cause multiple consecutive
rebuilds of the documentation set even though it already had built
against the new changes.  This is especially visible when building the
PDF variants of the documentation.

This change works well if pyinotify is installed; without pyinotify,
this behaves more or less like it did previously.  The livereload
integration with pyinotify will invoke __call__(self) multiple times in
a row with all of the changed files and then the top-level ioloop will
invoke our callback once for all of the changed files.
This is useful if one wants to use -M latexpdf for the build environment.
@tim-nordell-nimbelink
Copy link
Author

The first patch in this series is probably related to the intent behind the suggestions within #87 although solves it in a different manner that doesn't introduce delays.

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how -M is implemented I'd prefer to do this avoiding adding -M to argparse here.

I've just pushed an update which removes LiveReload // Tornado in favour of a simpler WebSocket based implementation, so please remove the tornado imports. They do seem a little out of place, though.

A

Comment on lines +33 to +49
self.files = []
self.ioloop = ioloop.IOLoop.instance()

def __call__(self):
"""Generate the documentation using ``sphinx``."""

if self.watcher.filepath:
show(context=f"Detected change: {self.watcher.filepath}")
self.files.append(self.watcher.filepath)
if len(self.files) == 1:
self.ioloop.add_callback(self.callback)

def callback(self):
if self.files:
for file in self.files:
if file:
show(context=f"Detected change: {file}")

self.files = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated?

It could be split into a separate PR if that'd be helpful. The commit explains it here: 0c2914f

Mainly I've encountered situations where multiple files got touched causing the livereload layer to call us back multiple times in a row each time causing a build. This is really annoying when building a longer build target, such as when using -M latexpdf. It's not as annoying with the -b html target since Sphinx doesn't rebuild the entire doctree for that target (but only the changed files). The -b latex (and thus also -M latexpdf) causes the entire doctree to be rebuilt and is terribly slow if that occurs.

When pyinotify is installed, the callbacks from tornado occur back to back in a row and if we install a callback handler via the tornado ioloop it'll get invoked after all of the callbacks that were generated at the same time. This doesn't work for the file polling method of livereload (e.g. when pyinotify is not installed) but it doesn't introduce any adverse behaviors by adding this callback hook at the top level.

@AA-Turner AA-Turner linked an issue Apr 9, 2024 that may be closed by this pull request
@tim-nordell-nimbelink
Copy link
Author

Given how -M is implemented I'd prefer to do this avoiding adding -M to argparse here.

I'll look at refactoring this sometime later this week if I get a chance.

I've just pushed an update which removes LiveReload // Tornado in favour of a simpler WebSocket based implementation, so please remove the tornado imports. They do seem a little out of place, though.

I'll take a look at this change.

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

Successfully merging this pull request may close these issues.

Support newly documented (but previously available) -M option
2 participants