-
Notifications
You must be signed in to change notification settings - Fork 46
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
Build and deploy TriBITS documentation with Sphinx (#386) #387
Build and deploy TriBITS documentation with Sphinx (#386) #387
Conversation
@marcinwrobel1986, it seems it may be possible to get Sphinx to run a script to generate the input files so that we don't need commit them to the repo to any branch. That way, we can also use readthedocs.com to deploy the documentation. Some details are in:
It seems like we might be able to put code in the bottom of SIDE-NOTE: A funny quote from readthedocs/readthedocs.org#8133 (comment):
|
FYI: As per the instructions at: I installed the latest Anaconda3-2021.05-Linux-x86_64 on my SNL RHEL 7 machine using:
which installed it under:
and tried to install an updated Sphinx with that 'conda' and I got:
Seems like a proxy issue or something. I could spend some time trying to debug this to at least do the downloads. Otherwise, it may be very hard for me to build this documentation on a Linux RHEL 7 machine at SNL if it requires a newer Sphinx install. (Which is one reason why I have never been able to use Sphinx in the past. I could never get it to install on SNL machines.) Next, I could try installing everything I need on my Windows 10 machine and running under cygwin? I just don't have the luxury of an up-to-date Linux distribution at SNL. |
Hello Ross, that's all really too much. Anaconda is a huge platform. First some checking:
In my case, it's telling me I have two Python interpreters installed in my OS. I have other built from source and installed as well, but it doesn't matter now. If one has no Python or Python3 one could install it. Easiest way would be from package manager. It could be
The above is regarding Python installation. One needs to install python packages, so:
|
So I will try the following in install Sphinx. On my Windows 10 machine off the SNL network:
Then copy these files to RHEL7 machine and run:
|
Next action items:
Then we will touch base again on Thursday and try to wrap up this documentation effort quickly after that. |
@marcinwrobel1986, okay, with some local help, I figured out how to get pip3 to communicate and download files on my RHEL 7 machine and install Sphinx and I was able to build the Sphinx documents locally and view them in the local browser (see detailed notes below). The state of the repo after doing this build was:
The biggest issue I see is that this process modifies the original source files:
That will make maintenance of this documentation very difficult. (A simple rule is that a build process should never change a manually maintained file.) To avoid this, can we just put the Sphinx base
? The document I will add a "Task" section above to keep track of everything we need to do to get this merged and deployed. Thanks again for all of your help on this! DETAILED NOTES: (click to expand). After getting the proxy set up to work with pip in my local SNL RHEL 7 machine, I was able to install Sphinx and the RTD theme with:
Now I need to give this a try! I checked out the branch:
I then ran it on 'crf450' as:
and it did:
This unfortunately modified the orginal
It changed the paths like:
That is not a good workflow. Instead, the Sphinx documents should be generated right there in the same directories as:
and not require any modifications to the original source files. Now to build the Sphinx documentation:
That generated:
And looking at this in firefox on that machine I can see the build documentation. So that worked! The final state of the git repo is:
|
Ross, I would said that a simple rule is when you have hardcoded relative paths which does not work when run from other directory, you make a generic script which change them, changed files are not commited anywhere. If you take a look at the problem in very generic way, you have some output and you want this output to work with other tool. Other tool expects diffent input then your output. What to do? Natural way would be to modified the output to fulfill requirements for the input of the other tool. Like adapter, right?. This is exactly how it's done now. It has nothing to do with the maintenance of the documentation. One makes any changes one wants to the documentation, files are generated with |
@marcinwrobel1986, see the list of "Tasks" above and see if this is consistent with what we discussed yesterday.
The problem is that reStructuredText does not really give you any other option that I can see. You can't use variables to specify the paths and the paths are relative the the file that includes them. It is very ridged. I was able to play some tricks with symlinks and avoid needing to edit or copy files. But if you do need to modify the files, then you copy then to a different location and then modify them. You never modify them in place. Does that make sense? |
@bartlettroscoe new landing page is deployed, please check whenever you have a moment |
@marcinwrobel1986, thanks! I will give that a try right now and work to get rid of to the Python2 and DocUtils requirement (in a separate PR branch that can be merged to this PR). |
@bartlettroscoe please mind that now Sphnix is runnig from Python script, you can check the changes in the CI job |
@marcinwrobel1986, yup, I saw what from inspecting the updated |
@marcinwrobel1986, I pulled the updated branch at the commit 1b87c9c:
and built the updated documentation (see details below) and I was able to view the updated documentation with three separate Sphinx documents. Thanks for doing that! However, it looks like the build process is still modifying manually maintained source files:
Can we meet soon to discuss how to adjust things to avoid this? A quick meeting to discuss the issues and the different options would be best. NOTE: There are several issues with this Sphinx generated documentation including formatting issues (like not showing bulleted lists of links correctly), relative links between the documents are broken (because they assume the documents are side-by-side), the sphinx search results is only shows the "Introduction" section, the page title in the browser starts with "Introduction" and the messy TriBITS version but at least this is a good start. We can do that tweaking later. To merge this branch, we just need to not break the existing DoUtils documentation and we need to not have the build process modify any original source files. DETAILED NOTES (click to expand)(7/15/2021) @marcinwrobel1986 has an updated Sphinx documentation build. I need to try this out and give feedback. Let's give it a try. Updated branch to:
Now to figure out how to build the updated documentation from looking at
The updated repo state is:
It is still modifying the original source files. The new landing page is:
I was able to view all three separate documents in the local browser. |
Ross, this is just done when documentation is generated, it does not stay in the repository. This is needed for documentation to be correctly generated. It should not be a problem at all. The other way would be to change paths (include directive) in Regarding meeting, sure I am available, but after last night storm I don't have my fiber working and I am on my mobile internet. This should be solved hopefully tonight.
As we discussed, regarding search, there is nothing that could be done as each documentation is a single html page. Regarding other issues I can't tell for now. You have to show me the current and desired behaviour and then maybe I could help. |
@marcinwrobel1986, the problem is that when you are iteratively building and tweaking the documentation this becomes a major problem. I can give you some concreate development and debugging workflows where this becomes a major problem and is asking for trouble. Why not just generate the |
@marcinwrobel1986, let's just get the basic mechanics of the build and deployment process done so we can merge this branch, deploy a process to automatically deploy updated versions of these documents, and link to them from tribits.org as alterative forms of these documents. Then we can do a more careful review of all of the issues and address them later. We then really need to get back to refactoring the guts of TriBITS to complete the bulk of epic #367 (and #63 and #299). |
Ross as I wrote:
Once you want to go with Sphinx, you can change |
@marcinwrobel1986, I don't want to abandon the flat DocUtils-generated HTML files just yet because there are still a number of issues with the Sphinx generated documentation and I/we don't have the time right now to address all of them. At the same time, I don't want leave this great work sitting on a branch and I want people to have access to the (imperfect but still valuable) Sphinx generated documentation. Given all of that, I think the best course of action is to address the problem of modifying manually maintained |
@marcinwrobel1986, let me scope that out locally and see how that goes. Seems like that should be pretty easy to do. |
…#386) This is not really the build reference for the TriBITS project per-say. It the build reference for an arbitrary TriBITS project <Project>. It does not make sense, I don't think, to produce this document for the TriBITS project itself since it only has one package. And people who are building the TriBITS package and running TriBITS test will know quite a lot about TriBITS so I think they can live with this generic document.
…st files (TriBITSPub#386) You have to actually call the tool create-project-build-ref.py to generate the final *.rst files. You just need to call it and tell it to not generate any of the other output (which is easy to do).
I just had to fix a minor defect in tribits/doc/build_ref/create-build-ref.sh and now this works fine with building the Sphinx documentation.
There is no need to cd into that directory anymore. It knows where it is and how to build the docs in-source.
I also make sure this does not run over top of the existing test build_docs (see comments in file).
dd385c9
to
8d28c1b
Compare
@marcinwrobel1986, I did the following:
My more detailed review notes are below. The changes for my commits that I pushed are shown in marcinwrobel1986#1. I think this is ready to merge and deploy! DETAILED NOTES: (click to expand). I pulled the updated copy of the branch 'marcinwrobel1986/386-Build-TriBITS-Documentation-with-Sphinx-and-ReadTheDocs' at the commit:
I went in and build the documentation locally on my RHEL 7 machine 'crf450' with:
After that, the state of the repo was:
Yea, no modified source files! Looking at the generated documentation starting at:
I was able to view the three updated documents. To rebuild the documentation, I had to run:
That seemed to work. I rebased and cleaned up the commits and pushed. I created the stacked PR: to show the changes and pushed all commits. The files that were added are tiny:
Very nice! Now the tests are TriBITS GHA running ... They all passed! |
You can't just run 'python3 sphinx_rst_generator.py' if you have already built the documentation locally. It will crash. But note the warning at the top about committing your work (or at least working in tracked non-ignored files).
It is important to be able to rebuild the documentation iteratively locally in a robust way. The rebuild makes this an expensive tests by TriBITS standards (over 35s on my super fast Linux machine) but this test is disabled by default and it only takes one core to run so hopefully this is not too bad.
I changed from 4-char to 2-char indents and wrapped lines to fit in 90 chars. This really should be reformatted a little closer to 85 chars or so but at least now I can do side-by-side editing on a reasonable side screen. I know the Python standard is 4 char indents but if you do that you need a lot more functions and a lot more wrapped lines to fit in 85 chars or so.
@marcinwrobel1986, I know why the Sphinx documentation build generates the error:
The reason is because when script
and it replaces it in the copied file
Note that it lost the indentation of two spaces before the includes. It turns out that you need that indentation. Fixing this is likely easiest with a unit test for that code to highlight the problem and then to fix it. I will see if I can do that quickly with your code. NOTE: I pushed the commit bafff87 to reformat that script to 90-chars wide so that I could view it in split view on a reasonable sized monitor (and with my bad eyes). Hopefully that is not an issue. I felt like I needed to do this to be able to effectively work with this code. |
@marcinwrobel1986, so I looked at writing a unit test for the function |
…it tested (TriBITSPub#386) First, the function is_rst_file() no benefit from being a member of the class. This is not Java. Every function does not have to belong to a class. Second, the function change_paths_and_get_includes() has a very clear purpose and is unit testable but not when it was inside of the class. Removing it from the clas will allow it to be unit tested which will allow us to find and fix a defect quickly.
…#386) This shows the problem. Now debuging the code and fixing it and ensuring it stays fixed will be easy.
…SPub#386) This fixes a defect where loosing the indent was causing an error in the converted *.rst file.
@marcinwrobel1986, it is fixed in commit 1d66677 with unit test to demonstrate and protect it. Now this builds with zero errors. |
…ITSPub#386) These includes got copies and pasted from another unit test driver.
…riBITSPub#386) When unit testing, you can't copy these files. I have no idea how this unit test passed locally when I ran it. In production mode, the default is to copy the files. This is actually a pretty nice algorithm. I also moved the unit test higher in the CMakeLists.txt file so this might make it more likely that this unit test runs before the actual build of the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we could simplify this some after looking at this in some more detail after having to debug the include indentation issue. I will take a look at removing the field and see if my hunch is right.
'maintainers_guide': { | ||
'src': os.path.join(doc_path, 'guides', 'maintainers_guide', | ||
'TribitsMaintainersGuide.rst'), | ||
'src_path': os.path.join(doc_path, 'guides', 'maintainers_guide'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this path duplicated in the 'src_path' field? This is the base path in the of the 'src' field in every case, no? Can't you just call os.path.dirname()
on the src
field and get this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, there are more pressing things than to refine this code. It works. Time to merge and move on.
@bartlettroscoe Hello Ross, this is not hard, you just need to know how to do that. There are no private methods, so you can test every method without any problems. If these functions would be use somwhere else, they would be outside a a class, but they are not. One can test |
@marcinwrobel1986 that is essentially what I wrote on a couple of more readable/maintainable lines. After that function was isolated and run in a unit test harness (which was 90% of of the work), it was easy to fix. A good bit of that code likely could have been written faster with TDD and then there would have been unit tests left over at the end to support any needed future maintenance (win-win). All of this code does not meet the 5 criteria in when not to write automated tests. |
Brings in numerous refactorings to TriBITS over the last 3 months, but there should be no breaks in backward compatibility. Almost every file in TriBITS is changed due to the lower-casing of command, macro and function names in PR TriBITSPub/TriBITS#379. But the main driver for this snapshot is to bring in the change in PR TriBITSPub/TriBITS#413 that should make it so that Kokkos INTERFACE_COMPILE_OPTIONS get propagated to downstream targets in TriBITS and therefore to external customers through installed <Package>Config.cmake files and IMPORTED targets. I should have done several snapshots in the last few months and not done a big snapshot like this (but I have been testing with Trilinos locally along the way). Overall, this merge brings in changes from a bunch of TriBITS PRs including (from most recent): * TriBITSPub/TriBITS#413: Change internal TriBITS target_link_libraries() to PUBLIC (TriBITSPub/TriBITS#299) component: core type: enhancement * TriBITSPub/TriBITS#410: Upgrade from cmake 3.21.0 to 3.21.2 (TriBITSPub/TriBITS#363, TriBITSPub/TriBITS#394) * TriBITSPub/TriBITS#394: DO NOT MERGE: Show TriBITS test failures with CMake 3.21.0 that don't occur with CMake 3.17.5 (TriBITSPub/TriBITS#363) * TriBITSPub/TriBITS#409: Add getTestDictStatusField() to handle empty 'status' field (SESW-383) component: ci_support type: enhancement * TriBITSPub/TriBITS#408: Deal with spaces in CDash url parts (SESW-383) component: ci_support type: enhancement * TriBITSPub/TriBITS#403: Spelling fixes * TriBITSPub/TriBITS#407: Move tribits_get_build_url_and_write_to_file() to stand-alone module (TriBITSPub/TriBITS#154) component: ctest_driver type: enhancement * TriBITSPub/TriBITS#388: Fixing typos (TriBITSPub/TriBITS#377) * TriBITSPub/TriBITS#406: Fix tribits_ctest_driver() package-by-package mode for CMake 3.19+ (TriBITSPub/TriBITS#363, TriBITSPub/TriBITS#394) component: ctest_driver type: bug * TriBITSPub/TriBITS#401: Improve GitHub Actions and CDash integration (TriBITSPub/TriBITS#154) type: enhancement * TriBITSPub/TriBITS#366: CI: draft action yaml * TriBITSPub/TriBITS#402: Revert some incorrect uppercase->lowercase changes * TriBITSPub/TriBITS#387: Build and deploy TriBITS documentation with Sphinx (TriBITSPub/TriBITS#386) component: documentation type: enhancement * TriBITSPub/TriBITS#398: Deal with pr null in not preprending build name (TriBITSPub/TriBITS#363) type: bug * TriBITSPub/TriBITS#396: Send PR results to 'Pull Request' CDash group and add PR ID to build names (TriBITSPub/TriBITS#363) type: enhancement * TriBITSPub/TriBITS#397: Print the ninja path and version (TriBITSPub/TriBITS#363) * TriBITSPub/TriBITS#393: GitHub Actions based testing for TriBITS (TriBITSPub/TriBITS#363) type: enhancement * TriBITSPub/TriBITS#389: TriBITS CI testing with GitHub Actions (TriBITSPub/TriBITS#363) type: enhancement * TriBITSPub/TriBITS#392: Fix broken tests for non-Fortran and CMake 3.21 builds (TriBITSPub/TriBITS#363) component: core * TriBITSPub/TriBITS#391: Fix up build_docs.sh for Sphinx doc generation (TriBITSPub/TriBITS#386) component: documentation type: enhancement * TriBITSPub/TriBITS#390: Add test for doc generation and fix usage of Python3 component: documentation type: bug * TriBITSPub/TriBITS#385: Replace last few references to TribitsDevelopersGuide.html (TriBITSPub/TriBITS#381) component: documentation type: enhancement * TriBITSPub/TriBITS#384: Split TribitsDevelopersGuide.* into TribitsUsersGuide.* and TribitsMaintainersGuide.* (TriBITSPub/TriBITS#381) component: documentation type: enhancement * TriBITSPub/TriBITS#383: Remove endfunction(<string>) and endmacro(<string>) (TriBITSPub/TriBITS#274, TriBITSPub/TriBITS#382) component: common_tpls type: bug * TriBITSPub/TriBITS#380: More package-arch data-structure documentation updates (TriBITSPub/TriBITS#63) component: documentation type: enhancement * TriBITSPub/TriBITS#379: Convert TriBITS to lower-case CMake command, macro, and function names (TriBITSPub/TriBITS#274) The following files were conflicting where I went with what is on the Trilinos 'develop' branch: * cmake/tribits/common_tpls/FindTPLBLAS.cmake * cmake/tribits/common_tpls/FindTPLLAPACK.cmake * cmake/tribits/common_tpls/FindTPLNetcdf.cmake (It looks like the above changes never made it back into TriBITS proper. The conflicts were due to the case changes in cmake command calls in these files due to TriBITSPub/TriBITS#379.) There was also a conflict in the file: * cmake/tribits/core/installation/TribitsProjectConfigTemplate.cmake.in I looked at that one carefully and I think that may have been due to fixes on both sides and then the case changes from TriBITSPub/TriBITS#379.
Implements #386
This PR
Read The Docs
templaterst
files:include::
directive inrst
filesThe documentation is currently being deployed at:
Tasks
deploy-doc-site
that can be removed later if neededtribits/doc/build_docs.sh
andtribits/doc/guides/generate-guide.sh
(and don't require thepython
hack) [@bartlettroscoe]rst2html.py
to build the flat HTML files with docutils when extracting the documentation from the CMake files to create the intermediate*.rst
files withtribits/doc/build_docs.sh
andtribits/doc/guides/generate-guide.sh
when called when building Sphinx documentation (that is a waste in that case) [@bartlettroscoe]