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

Implement fragment-dependent cache invalidation in Sphinx for RST documents using the extension #1

Closed
webknjaz opened this issue Aug 1, 2020 · 8 comments · Fixed by #8
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@webknjaz
Copy link
Member

webknjaz commented Aug 1, 2020

To speed-up sequential builds, Sphinx caches documents as doctrees. This cache is based on whether the tracked RST files have changed and does not know anything about the changes in the directive output.
So including this directive into the document may result in it not picking up the new change fragments.

We need to explicitly document this and add some pointers on how to force rebuilds.

This idea is coming from ansible/pylibssh#120 as reported by @ewjoachim.

@webknjaz webknjaz added the documentation Improvements or additions to documentation label Aug 1, 2020
@ewjoachim
Copy link
Contributor

Note: if anyone finds a smart idea for this, I'll definitely steal it in https://github.com/peopledoc/sphinx-github-changelog and vice versa.

(and I'm convinced there's a technical way of handling this, not just reminding the user to make the job of the computer :) )

@webknjaz
Copy link
Member Author

webknjaz commented Aug 2, 2020

Ref: sphinx-doc/sphinx#8040

@ewjoachim
Copy link
Contributor

ewjoachim commented Aug 3, 2020

https://www.sphinx-doc.org/en/master/extdev/envapi.html?highlight=note_dependency#sphinx.environment.BuildEnvironment.note_dependency \o/

We should be calling this on all the rst files in the fragment folder.

@webknjaz
Copy link
Member Author

webknjaz commented Aug 3, 2020

@ewjoachim interesting... But how would sphinx know that this is connected to a directive in a certain file? I thought we need to point it to the document that uses the directive.

@ewjoachim
Copy link
Contributor

No, this is done in the directive code itself, so Sphinx knows where it is.

@ewjoachim
Copy link
Contributor

ewjoachim commented Aug 3, 2020

Fact is we could have multiple directives, each pointing to a different towncrier config and this would still work in theory

@webknjaz
Copy link
Member Author

webknjaz commented Aug 3, 2020

Nice! Although, the current implementation has one global setting for the towncrier config and I don't think that one project would need more (unless it's a monorepo, I guess, but then the docs would likely be separate anyway).

P.S. The main Towncrier CLI only supports custom config in their repo master, not in the released version so it's all only theoretical atm.

@webknjaz webknjaz changed the title Document ways of dealing with Sphinx not rebuilding cached RST documents 📝 Document ways of dealing with Sphinx not rebuilding cached RST documents Aug 5, 2020
@webknjaz webknjaz added the bug Something isn't working label Aug 7, 2020
webknjaz added a commit to webknjaz/sphinxcontrib-towncrier that referenced this issue Aug 10, 2020
This solution is suboptimal because it forces Sphinx to rebuild the
document regardless of whether the news fragments have changed in any
way.

OTOH, using `BuildEnvironment.note_dependency()` would be rather
useless in `TowncrierDraftEntriesDirective.run()` since it'd only make
it depend on the existing fragment changes (that almost never happens)
and would ignore any new fragments appearing on disk (because the only
dependencies declared would be the ones that existed at the time of
the first directive run).

The next step would be implementing tracking of the fragments outside
of the directive using 'env-get-outdated' combined with tracking of
the documents that use the directive via `BuildEnvironment` custom
attribute and `EnvironmentCollector` for the Sphinx parallel mode
support.

Refs:
* sphinx-contrib#1
* sphinx-contrib#1
* sphinx-doc/sphinx#8040
webknjaz added a commit to webknjaz/sphinxcontrib-towncrier that referenced this issue Aug 10, 2020
This change addresses sphinx-contrib#1 only partially and as such it needs a
follow-up.

The solution is suboptimal because it forces Sphinx to rebuild
documents regardless of whether the news fragments have changed in any
way.

OTOH, using `BuildEnvironment.note_dependency()` would be rather
useless in `TowncrierDraftEntriesDirective.run()` since it'd only make
it depend on the existing fragment changes (that almost never happens)
and would ignore any new fragments appearing on disk (because the only
dependencies declared would be the ones that existed at the time of
the first directive run).

The next step would be implementing tracking of the fragments outside
of the directive using 'env-get-outdated' combined with tracking of
the documents that use the directive via `BuildEnvironment` custom
attribute and `EnvironmentCollector` for the Sphinx parallel mode
support.

Refs:
* sphinx-contrib#1
* sphinx-doc/sphinx#8040
@webknjaz
Copy link
Member Author

FTR the approach with note_dependency() wouldn't work because we need to inject this outside of the directive from the new files that appear after the cache already exists. I've added some comments in #23.

webknjaz added a commit that referenced this issue Aug 11, 2020
This change adds a minimal workaround for #1 that results in the
documents using the directive will be always rebuilt unconditionally.
@webknjaz webknjaz changed the title 📝 Document ways of dealing with Sphinx not rebuilding cached RST documents Implement fragment-dependent cache invalidation in Sphinx for RST documents using the extension Sep 5, 2020
@webknjaz webknjaz self-assigned this Sep 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
2 participants