Closed Sameeranjoshi closed 4 years ago
Does this docs/ folder need adding to the base CMakeLists.txt above? This doesn't look like it'll ever get built as is?
As regards the doxygen version, I think our minimum version should be the same as the rest of the project unless there's a really strong reason for that not to be the case.
Oh, I see, you're waiting for the CMake changes to land before adding that. Ignore me 😄
Does this docs/ folder need adding to the base CMakeLists.txt above?
Yes, FIR documentation(https://github.com/flang-compiler/f18/#how-to-generate-fir-documentation) as well generates in docs/ folder. But how about the current documentation/ folder are we moving the files in documentation/ folder to docs/?
This doesn't look like it'll ever get built as is?
Correct
As regards the doxygen version, I think our minimum version should be the same as the rest of the project unless there's a really strong reason for that not to be the case.
Will change to 1.8.6
Oh, I see, you're waiting for the CMake changes to land before adding that. Ignore me 😄
I see some work done here but I think need to resolve
add_subdirectory(documentation)
to
add_subdirectory(docs)
@sscalpone can the reviewers be assigned to this PR as CMake changes have been merged, so I can take this up ?
README.md needs to be updated to explain how to generate documentation. Please leave in a reference to the FIR docs.
@Sameeranjoshi Does this PR require that CMakeLists.txt be modified? Do I need to enable something besides LLVM_BUILD_DOCS or LLVM_ENABLE_DOXYGEN for cmake? How do I test this PR? What will be the result?
@Sameeranjoshi Does this PR require that CMakeLists.txt be modified?
Yes, I have modified for doxygen support.
Do I need to enable something besides LLVM_BUILD_DOCS or LLVM_ENABLE_DOXYGEN for cmake?
I have tried -DLLVM_ENABLE_DOXYGEN=ON -DFLANG_INCLUDE_DOCS=ON.
Haven't tried LLVM_BUILD_DOCS
as the doxygen seems to work without it, as well commented the dependency of the above flag for now, as I think it's a post merge into mono repo flag?
How do I test this PR?
Please build F18 with addition of -DLLVM_ENABLE_DOXYGEN=ON -DFLANG_INCLUDE_DOCS=ON
flags.
ninja install
ninja doxygen-flang #generates docs/ folder in build/ folder
ninja install doxygen-flang #copies to install location
What will be the result?
Inside install/docs/
folder, doxygen generated files should be present.
I usually open it as firefox install/docs/html/html/index.html
As regards our documentation/ folder; eventually we probably want to move our .md files to match the Sphinx infrastructure that LLVM uses for non-code documentation, but I think that's a separate issue to this. Potentially we can just rename it to docs/ though?
I added -DLLVM_ENABLE_DOXYGEN=ON -DFLANG_INCLUDE_DOCS=ON
to my cmake command and tried ninja install
and I get this error:
CMake Error at docs/cmake_install.cmake:36 (file):
file INSTALL cannot find
"<build-dir>/docs/doxygen/html".
Call Stack (most recent call first):
cmake_install.cmake:48 (include)
As regards our documentation/ folder; eventually we probably want to move our .md files
I suppose moving the documentation to docs would be reasonable, although not critical. These files have embedded references to other docs so its may not be a simple renaming. We should search for and replace embedded references to github/flang-compiler.
A bigger, but also straightforward, job will be to convert the markdown to the rst format that we'll need to make the docs available on llvm.org/flang.
If I run ninja doxygen
in an LLVM in-tree build, the flang docs don't get built as part of that target. The clang
and mlir
ones both do so I think flang should as well
@RichBarton-Arm @DavidTruby I'm no expert on doxygen. If you'd like, please review for content & approve when ready. Thanks
@Sameeranjoshi Thanks for the update. This commit now works as you described.
@Sameeranjoshi Thanks for the update. This commit now works as you described.
@sscalpone @DavidTruby Are you seeing similar issue as @tskeith mentions here
With the current review changes ca55c2e366b6621ab37fc9d467fe9fb2d011e2d2
I used FLANG_INCLUDE_DOCS=ON and LLVM_ENABLE_DOXYGEN=ON
during Cmake.
ninja doxygen-flang #builds documentation
ninja install # installs documentation in install/ folder
@DavidTruby @sscalpone @RichBarton-Arm which changes are still remaining to merge this PR, can you please point ?
I added
-DLLVM_ENABLE_DOXYGEN=ON -DFLANG_INCLUDE_DOCS=ON
to my cmake command and triedninja install
and I get this error:CMake Error at docs/cmake_install.cmake:36 (file): file INSTALL cannot find "<build-dir>/docs/doxygen/html". Call Stack (most recent call first): cmake_install.cmake:48 (include)
I was able to trace this issue.
When build with -DLLVM_ENABLE_DOXYGEN=ON -DFLANG_INCLUDE_DOCS=ON
flags
then
make -jx
now instead of make doxygen-flang
if I use make install
I see the error.
Is that the expected behavior, @RichBarton-Arm @DavidTruby @sscalpone ?
Thanks
I added
-DLLVM_ENABLE_DOXYGEN=ON -DFLANG_INCLUDE_DOCS=ON
to my cmake command and triedninja install
and I get this error:CMake Error at docs/cmake_install.cmake:36 (file): file INSTALL cannot find "<build-dir>/docs/doxygen/html". Call Stack (most recent call first): cmake_install.cmake:48 (include)
I was able to trace this issue. When build with
-DLLVM_ENABLE_DOXYGEN=ON -DFLANG_INCLUDE_DOCS=ON
flags thenmake -jx
now instead ofmake doxygen-flang
if I usemake install
I see the error. Is that the expected behavior, @RichBarton-Arm @DavidTruby @sscalpone ?Thanks
I think if those flags are on, we need to add a dependency on the install target to the doxygen-flang target. Something along the lines of:
if (LLVM_ENABLE_DOXYGEN AND FLANG_INCLUDE_DOCS)
add_dependencies(install doxygen-flang)
endif()
added just after the doxygen-flang target is created. (I haven't tested this so the syntax might be wrong)
Below commits add docs/ folder as per LLVM's directory structure and adds necessary doxygen supporting files in it. Other projects in LLVM infrastructure use 1.8.6 as doxygen version. Considering it's pretty older one(released on 2013-12-24 https://sourceforge.net/projects/doxygen/files/ ) used version 1.8.13 which shows few new variables introduced in it.
TODO: