Skip to content

Add pre-commit-sphinx to list of hook repositories#362

Closed
thclark wants to merge 1 commit into
pre-commit:masterfrom
thclark:master
Closed

Add pre-commit-sphinx to list of hook repositories#362
thclark wants to merge 1 commit into
pre-commit:masterfrom
thclark:master

Conversation

@thclark
Copy link
Copy Markdown

@thclark thclark commented Jun 19, 2020

I've added a hook for building documentation with sphinx, so that commits fail if they break documentation builds.

@asottile
Copy link
Copy Markdown
Member

I don't think the hook will work as written in general. sphinx usually needs access to your dependencies and won't have that access when run in pre-commit

additionally, pre-commit hooks are intended to be as fast as possible, and sphinx-build is pretty slow

also you've got a security problem here (shell injection)

@thclark
Copy link
Copy Markdown
Author

thclark commented Jun 19, 2020

Thanks for the feedback @asottile :)

  1. I already have an issue open here to allow any sphinx dependencies to be defined (in this first version I've just used a very limited but pretty common subset which will work for a lot of python libraries out there).

  2. sphinx-build can be slow on the first build (similar to the first run of a new hook in pre-commit) but it caches the doctrees, so becomes much faster on any subsequent runs (rebuilding only changed files, so time doesn't increase at all linearly with docs size).
    To put some numbers on this, the first build with no cache for these reasonable-size docs takes 2.59 seconds on my 2013 macbook (a fraction of the time it takes pre-commit to fetch the repo and set up the environment), then of course accelerates significantly with sphinx having built a cache.
    So I don't feel like this speed is a dealbreaker - I haven't even noticed the pause (been committing regularly to that example repo with this hook installed all day now).

  3. Issue created to solve the security problem, thanks, well spotted, thank you

@asottile
Copy link
Copy Markdown
Member

I guess more specifically you need to install the project under test to the same sphinx-build executable, which isn't a thing that pre-commit can do (by design)

my guess is that a language: system local hook that calls entry: sphinx-build ... with pass_filenames: false may be better suited?

@thclark
Copy link
Copy Markdown
Author

thclark commented Jun 19, 2020

sphinx-build isn't a compiled executable, it's just a little shell script that tries to find the right python version then runs sphinx with that version of python. In this case in the hook, we have the right version of python on the path already, so we don't actually need it.

(I only had it in as I was copying and pasting over from an old build system.... better to get things working then gradually tidy).

I'll fix the security issue by bypassing sphinx-build and calling sphinx directly from python, which will remove the need for build-sphinx and keep language: python.

@asottile asottile closed this Jan 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants