boutproject
Move the manual `EXCLUDE_FROM_ALL` inwards
Original issue is that EXCLUDE_FROM_ALL conflicts with the install() command in that the COMPONENT cannot be used since the whole folder is excluded. This makes installing the documentation quite cumbersome because of the build directory manipulations.
On the other hand, the documentation building from CMake does not bring any advantages, and it is not documented. Instead the make could be used, and for doing the installation, on Fedora we use this
$ cd manual/html
$ find ./ -type f -not -path '*/.*' -not -path '.*/' -exec install -D "{}" "%{buildroot}%{_docdir}/bout++/{}" \;
See also: https://github.com/boutproject/BOUT-dev/pull/3111#issuecomment-2894474000
Edit: In the end, reverting to the original issue approach
Actually, this doesn't have the intended outcome because the EXCLUDE_FROM_ALL above completely negates any definitions inside here. It would be preferred if the top-level EXCLUDE_FROM_ALL can be removed and then individually it is added to the targets. That would break the cd build-dir/manual && make workflow, but to be fair, that workflow is broken if you use ninja as well. Thoughts?
As long as readthedocs still builds it fine, and it's documented how to build the docs locally, I think it's fine
As long as readthedocs still builds it fine, and it's documented how to build the docs locally, I think it's fine
On that front, RTD does not use the cmake build, and the documentation does not mention how to use the BOUT_BUILD_DOCS. Are you ok, with removing it completely from the cmake build? The only usecase for using sphinx/doxygen inside a cmake build is if you must document generated sources, which from the fact that the RTD does not do so, I am assuming it is not the case here.
Would it not be fine to just remove "EXCLUDE_FROM_ALL"?
If you explicitly enable it, you should be fine with building it? If not, disable it?
Would it not be fine to just remove "EXCLUDE_FROM_ALL"?
If you explicitly enable it, you should be fine with building it? If not, disable it?
Technically, yes. The question is if you wish to maintain this build process. I have not found any benefits for running sphinx inside CMake, and it is a cleaner workflow for RTD and downstreams to run sphinx directly. But if you wish to keep it, I will accommodate that change.
It does do more then just call sphinx, it also sets up the environment variables, so that python can pick up the python interface for BOUT++. I think that will be tricky to get right without cmake.
For fedora this is of course no issue, as I know how to do it, and only need to script it once, but it will make things more tricky for others, that want to change some documentation that involves the python interface.
@ZedThree What do you think?
It does do more then just call sphinx, it also sets up the environment variables, so that python can pick up the python interface for BOUT++. I think that will be tricky to get right without cmake.
But the thing is, RTD or the makefile do not use it. They call python and doxygen directly. For the makefile, it does set the PYTHONPATH
https://github.com/boutproject/BOUT-dev/blob/1cd0fa7cc9bd47f6bedf36ffd569a5c44227dfdb/manual/Makefile#L4
And once more in the sphinx conf.py
https://github.com/boutproject/BOUT-dev/blob/1cd0fa7cc9bd47f6bedf36ffd569a5c44227dfdb/manual/sphinx/conf.py#L34
For documenting compiled python bindings, that is a different story, but I don't see them being documented anyhow.
Ah, I remember. For RTD we do in-source-builds. There is even an extra option to allow that, but I do not want to recommend that to anybody that wants to keep working with the checkout ...
in the examples we also have also EXCLUDE_FROM_ALL.
I assume that also makes that non-installable with ninja.
@LecrisUT do you know how other projects deal with such targets that are excluded from all, but still should be installable?
@LecrisUT do you know how other projects deal with such targets that are excluded from all, but still should be installable?
Design wise it is indeed possible to make it work as optional build and install, the EXCLUDE_ALL just needs to be moved inward.
The thing that I want to highlight here is that sphinx is not benefiting from being run inside cmake and it is not an encouraged workflow. E.g. users and CI should be encouraged to use the sphinx executable directly so that they can add -W to catch warnings as errors, or -b linkcheck to check for link errors, or use sphinx-autobuild to continuously develop the documentation.
There is one usecase for sphinx inside the cmake for installing the html documentation for distro packages (even though it is not supported by sphinx), but even then the packager might choose to build a pdf or other formats which would need manual runs.
Given all that, I'm happy to reverse course on this and change it to the EXCLUDE_ALL (and review the other instances also), just wanted the state of the sphinx integration to be reviewed.
I see, so I do not add an extra global option like this: https://github.com/boutproject/BOUT-dev/compare/build-docs-all-option?expand=1
Yes, if you have the time to do that, that would be great. Currently it is quite hacky what we do in RTD, and I would like to recommend the cmake way to build the docs for developer, as we do not document generated source code, but part of the documentation is generated from compiled python binaries. So we probably need at least this bit: https://github.com/boutproject/BOUT-dev/compare/build-docs-all-option?expand=1#diff-914eec84582b5e30417e69e32a6f0badaf933890a9b79830c3632fa87f7b19a6
but part of the documentation is generated from compiled python binaries.
Ah ok, didn't check that part. In that case indeed it would make sense to include some integration. I have a fairly complex design in mind which would allow to integrate more seamlessly from both sphinx and cmake side, and avoid the hacky method. I should have some time next week to show that.
For now, I've changed this PR to just move the EXCLUDE_FROM_ALL inwards. I have another branch about some future improvements to the docs build, but for that one I am finding quite a lot of issues primarily due to the python bindings design, but there are some TODO comments there for what still needs to be done.
If someone wants to discuss how to simplify the python bindings, I can offer various recommendations, which would resolve #2814 as well.