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
Fix HTML docs generation #930
Fix HTML docs generation #930
Conversation
@gklimowicz, @bryanpkc, @shivaramaarao : Please help with review/suggestions. Note: In general we are interested in improving the documentation of Classic Flang upstream. We will try to share what we have. |
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.
I needed to apply this patch before I could configure flang with the additional -DLLVM_INCLUDE_DOCS=on -DLLVM_BUILD_DOCS=on -DLLVM_ENABLE_DOXYGEN=on
options:
diff --git a/docs/CMakeLists.txt b/docs/CMakeLists.txt
index ec2bfd84..e1a4ef3e 100644
--- a/docs/CMakeLists.txt
+++ b/docs/CMakeLists.txt
@@ -9,6 +9,8 @@ find_package(Sphinx)
if (DOXYGEN_FOUND)
if (LLVM_ENABLE_DOXYGEN)
+ add_custom_target(doxygen ALL)
+
set(abs_srcdir ${CMAKE_CURRENT_SOURCE_DIR})
set(abs_builddir ${CMAKE_CURRENT_BINARY_DIR})
Update: I found out that -DLLVM_BUILD_DOCS=on and -DLLVM_ENABLE_DOXYGEN=on
were probably intended for an in-tree build. They are not needed for an out-of-tree build of Flang, and I got both Doxygen and Sphinx documentation to build properly with only -DLLVM_INCLUDE_DOCS=on -DLLVM_ENABLE_DOXYGEN=on
, followed by make install doxygen-flang
. Please ignore the above patch.
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.
LGTM now. Thanks @michalpasztamobica!
@michalpasztamobica I noticed that xflag.n and xref.n are not in the documentation index, and that there are two versions of dinit.n. Would you consider merging the following patch into the PR? |
Nice catch @bryanpkc , I cherry-picked and pushed your patch to this PR. |
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.
Works fine on OpenPower LLVM 9.0 and 10.0.
Thanks @michalpasztamobica for this patch. Do you know why
|
Ad 1)
Then Ad 2) This limitation might not make much sense and it does make the generated html look a bit ugly. I think this could be taken care of by some clever |
@shivaramaarao is the patch OK with you? |
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.
looks good.
@michalpasztamobica please squash. |
* The ast.n file was moved to a different location causing cmake to fail to generate most of the existing documentation. * The unused rst files are now added to the list, so they are now also visible in the generated HTML. * Docs CMake rename gen_sphinx_docs to flang2_gen_sphinx_docs and add missing dependencies * Sphinx build depends on generating all necessary rst files * Directories must be created before putting files into them * gram.lis file dependency added (thanks @bryanpkc!)
7dee58e
to
c54e7ab
Compare
@kiranchandramohan , I squashed, just leaving @bryanpkc 's commit separate to keep the authorship in order. I hope this is OK? |
ast.n
file was moved to a different location causing cmake to fail to generate most of the existing documentation.gram.lis
file dependency added (thanks @bryanpkc!).gen_sphinx_docs
toflang2_gen_sphinx_docs
(to match existingflang1_gen_sphinx_docs
)@RichBarton-Arm , @kiranchandramohan , please review.