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

Make sphinx build self-sufficient and in-tree-safe #2560

Merged
merged 39 commits into from
Apr 28, 2023

Conversation

Helveg
Copy link
Contributor

@Helveg Helveg commented Dec 1, 2022

The paths in conf.py were complicated and build-env dependent, different between ReadTheDocs, CMake builds and out-of-tree builds. Out-of-tree builds were required because NEST's sphinx configuration littered the source tree. All these problems have been resolved and the documentation can be built without CMake supplying any paths, without supplying NESTSRCDIR, and without polluting the NEST source directory:

cd doc/htmldoc
sphinx-build . <output-dir> -b html

Since these are the stock standard minimum required arguments to sphinx-build, from the conventional working directory of conf.py, this ought to work out of the box on ReadTheDocs.

The CMake sphinx process is also simplified, and files no longer need to be copied over, and apart from resolve_includes.py also doesn't need to run any more commands. The output is identical.

Most of the littering of the local doc build has been fixed by gitignoring the outputs. Several plugins used by NEST are preprocessors that generate RST files to be used by the build, these mostly don't conflict with any existing files, and are overwritten every time the build is ran. The only downside is that someone might try to change these files and see their changes overwritten.

PS: Moved KernelAttributes out of ll_api so that ll_api can be mocked by autodoc rather than needing pretty complicated in-house mocking.

@jessica-mitchell jessica-mitchell added this to In progress in Documentation via automation Dec 1, 2022
@jessica-mitchell jessica-mitchell added this to To do in Installation via automation Dec 1, 2022
@jessica-mitchell jessica-mitchell changed the title Made sphinx build self-sufficient and in-tree-safe Make sphinx build self-sufficient and in-tree-safe Dec 1, 2022
@jessica-mitchell jessica-mitchell added T: Enhancement New functionality, model or documentation S: Normal Handle this with default priority labels Dec 2, 2022
Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

@Helveg The build of the docs locally and on Read the Docs work fine. However, the docs that are generated at build (pynest examples( auto_examples/) and models) all end up in the source directory. Previously we had them in the build directory which kept the source uncluttered. Can we have these files end up in build? @jougs I otherwise approve this PR, if it's urgent to put in.

@Helveg
Copy link
Contributor Author

Helveg commented Feb 12, 2023

They end up in the source directory because they are part of the source that Sphinx needs to pick up during an in-tree build. Everything that ends up in the source directory as autogenerated pre-build files should be gitignored though. Are there any files that are not gitignored cluttering your git tree? Is it a problem that there are extra gitignored files in the repo?

@jessica-mitchell
Copy link
Contributor

@Helveg yah, adding them to .gitignore works. I didn't think about it when building since this hasnt been an issue for a long while. Should .gitigonore be updated in this PR then? Namely adding, doc/htmldoc/autoexamples doc/htmldoc/models/* !doc/htmldoc/models/models-main.rst and !doc/htmldoc/models/models-toc.rst

@Helveg
Copy link
Contributor Author

Helveg commented Feb 13, 2023

I was convinced I had done so! 😅 What sreps did you take to produce the not gitignored files?

@jougs
Copy link
Contributor

jougs commented Feb 13, 2023

From my perspective, the files created by Sphinx are definitely build artifacts, much like the .o files generated by the compiler. As such they should not end up in the source directory. Actually, in my opinion nothing should end up there unless I put it there myself. That's why I created the (complicated) solution that is currently in place in master.

In my workflow, I want to be able to delete the build directory and be sure nothing of the build configuration remains, as I spent way too much time in my life chasing after old tests, docs, and other generated files that should not have been there anymore. Putting files in .gitignore is not sufficient for achieving this goal, as this would still lead to outdated documentation being shown if I delete or rename files, but do not delete the old files manually.

@jessica-mitchell
Copy link
Contributor

To reproduce: I just built the user documentation - make docs in my build directory - the auto generated rst files then appear in source...however, the problem still persists. I thought I had it working but it appears that the files still show up in my source tree. I'm not that familiar with .gitignore so I'm probably doing something wrong.

@Helveg
Copy link
Contributor Author

Helveg commented Feb 13, 2023

Thx @jessica-mitchell I'll take a look.

I believe there's 2 sources of additional source files: the extract userdoc script, and sphinx-gallery. Neither tools are actually integrated into the sphinx build process: sphinx-gallery seems to by design just render files into RST source files for sphinx to pick up, and our in-house extract userdoc does the same when conf.py is imported.

The ideal solution is to turn these scripts into extensions that can create doctrees during the build phase for each of these, so that sphinx can render them, without the need of intermediate source files on disk.

For now, @jougs would it suffice to keep the gitignored source trees, but as part of the sphinx build to clear out any possibly stale files that these tools may have put there from previous builds?

@jougs
Copy link
Contributor

jougs commented Feb 13, 2023

For now, @jougs would it suffice to keep the gitignored source trees, but as part of the sphinx build to clear out any possibly stale files that these tools may have put there from previous builds?

First of all: sorry that this slipped my attention when reviewing. I was under the impression that the new setup still works in the build directory, but does so without the manual steps and workarounds I have added back in the day.

Back to the topic. I just talked to @terhorstd and we agree that out-of-tree builds should be considered the default with building documentation on Read the Docs being the exception. As a consequence our policy is that no files are put in the source tree when building in a different directory.

For a development workflow with multiple active branches at a time and different configurations on the same source branch, a separation between an unmodified source tree (i.e. what comes from the Git repository) and build directory (i.e. the cmaked configuration together with the installation, the docs, etc.) has proven very useful.

The main benefits of such a setup are that

  • .gitignore files can stay minimal and don't have to be updated regularly
  • no build artifacts can be accidentally committed
  • build files for different configurations are persistent and cannot be overwritten by other configurations

How hard would it be to change your current setup so it does not leave any artifacts in the source tree?

@Helveg
Copy link
Contributor Author

Helveg commented Feb 14, 2023

The in-tree build can't be made any better than this without creating extensions. Sphinx has no separate build directory so the source tree needs to be modified in preparation of the build to transpile the model docs and gallery. I can add extra safeguards to make sure the stale files are cleaned up first.

I think I can run the same "in-tree" build but from the CMake build directory, rather than the source directory, simply by having CMake copy over all the source files to the build dir.. right?

@terhorstd
Copy link
Contributor

I think I can run the same "in-tree" build but from the CMake build directory, rather than the source directory, simply by having CMake copy over all the source files to the build dir.. right?

It's not a clean solution, but copy could work. I think we could go this way for now, and open a new issue for the proposed Sphinx extension. For the time being, for developers, the CMake options to not build docs could prevent this copying and would keep build dir clean. Additionally, copying with -l when possible would keep i-node counts low.

@terhorstd terhorstd requested a review from jougs February 24, 2023 12:41
Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

@Helveg I just have a couple of minor suggestions I spotted.

@terhorstd
Copy link
Contributor

@Helveg, any news on this?

@Helveg
Copy link
Contributor Author

Helveg commented Mar 7, 2023

I'm still very swamped, in the next month I'm unlikely to have time for this. I can polish it up to merge it with the gitignored source files, and a very pretty bowtie-promise wrapped around it that soon-ish I will create a Sphinx extension for it? :) If anyone else wants to give this a shot, it would boil down to copying the current _SPHINX_SOURCE_DIR into the build dir, and then running the commands from that new dir:

https://github.com/nest/nest-simulator/pull/2560/files#diff-652f011e4f45934b3684d4596670e3847750ec3289b5092d4f8847b1ca6a75edR71-R77

This may do the trick:

  set( _SPHINX_SOURCE_DIR "${PROJECT_SOURCE_DIR}/doc/htmldoc" )
  # new: source dir inside of the cmake build dir, for Sphinx to gratuitously pollute
  set( _SPHINX_BUILDSOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/_build_source" )
  set( _SPHINX_BUILD_DIR "${CMAKE_CURRENT_BINARY_DIR}/_build/html" )
  # new: copy the source source dir to the build source dir
  file(COPY ${_SPHINX_SOURCE_DIR} DESTINATION ${_SPHINX_BUILDSOURCE_DIR})
  # new: run the Sphinx build from the build source dir
  WORKING_DIRECTORY ${_SPHINX_BUILDSOURCE_DIR}
  COMMAND ${SPHINX_EXECUTABLE} -b html . ${_SPHINX_BUILD_DIR}
  COMMAND ${Python_EXECUTABLE} resolve_includes.py ${SPHINX_BUILD_DIR}/models

@Helveg
Copy link
Contributor Author

Helveg commented Apr 27, 2023

@jougs Can you review again?

Helveg and others added 2 commits April 27, 2023 18:19
…ser_documentation_workflow.rst

Co-authored-by: jessica-mitchell <mitchell20j@gmail.com>
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

I have some very minor suggestions, but am immediately approving to not delay this any further. Many thanks for cleaning this up and making it nicer 😄

doc/htmldoc/clean_source_dirs.py Outdated Show resolved Hide resolved
doc/htmldoc/conf.py Show resolved Hide resolved
doc/htmldoc/conf.py Outdated Show resolved Hide resolved
@Helveg Helveg requested a review from jougs April 28, 2023 07:35
Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

@Helveg @jougs I also approve - further discussions can be had later but this improves docs a lot, so thanks!

@jougs jougs merged commit 7bb7733 into nest:master Apr 28, 2023
6 checks passed
Documentation automation moved this from Review to Done Apr 28, 2023
Installation automation moved this from Review to Done Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Documentation
  
Done
Installation
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants