Custom Doxygen executable to improve Sphinx builds #139

MathiasMagnus · 2023-07-24T16:09:25Z

This PR improves how Sphinx docs are built, specifically those using rocm-docs-core.

Changes

Updates the documentation of rocm_add_sphinx_doc (which even had it's name outdated.
Adds more validation of parameters and feedback to users who misuse the function.
Adds search of Doxygen executable
- Defines an opt-in flag USE_DOXYGEN which triggers a poor man's version of REQUIRED for the Doxygen executable. It relays the full path of Doxygen to rocm-docs-core as enabled by this PR.
Adds a simple positive test of this function while previously there was none.

Notes

The requirements.in will need a change once the dependent PR of rocm-docs-core goes live and receives a tagged version.

lawruble13

Overall this looks okay to me, pending the merge of the noted change to rocm-docs-core. My only concern at present is adding BUILDER as a mandatory argument, I worry that will cause issues for backwards compatibility. I would prefer that this variable have a sane default (perhaps html?), but am open to arguments.

share/rocm/cmake/ROCMSphinxDoc.cmake

lawruble13 · 2023-07-25T19:21:56Z

share/rocm/cmake/ROCMSphinxDoc.cmake

+        get_filename_component(OUTPUT_DIR "${PARSE_OUTPUT_DIR}" ABSOLUTE BASE_DIR "${CMAKE_CURRENT_BINARY_DIR}")
+    else()
+        set(OUTPUT_DIR "${CMAKE_CURRENT_BINARY_DIR}/sphinx/${PARSE_BUILDER}")
+    endif()


This also removes some functionality, namely that the cache variables for the output directories of different builders are removed, so that setting these cache variables manually can no longer alter the output directories. I don't know if this functionality is used by anything, but it's worth noting.

Maybe worth noting. I saw only 2 projects use this module. Hoping that will change in the future.

The cache variables in the long run have to go. While using ExternalProject excessively to drive the docs super build such that we could have multiple instances of these cache vars (one per project), the end game IMHO should be getting ROCm projects to a state where they are add_subdirectory friendly, at least among themselves and the entire thing (minus LLVM) can be built using one ninja process. If that's not something we're aspiring towards, I could restore the cache variables to how they were, but it will cause conflicts as soon as two documented projects share a build.

I dont know how much this is used, but it is nice to be able to set the OUTPUT_DIR without needing to modify the source files. Maybe you could just do if(DEFINED) check and then use the variable. This shouldn't cause problems for add_subdirectory.

I agree with the justification on removing the cache variables in the long-term. Paul's suggestion of just checking to see if a variable is defined seems reasonable to me to duplicate the functionality noted while not having collisions. I would also note that collisions could be avoided by adding the project name to the cache variable used, although that definitely leads to other potential usability issues of knowing exactly how that variable should be used.

Please take a look at the current solution driven by ROCM_CMAKE_DOCS_DIR. This was the closest to replicating a possibility of specifying externally an output directory while also keeping it idiomatic CMake (as far as command wrappers can be idiomatic). It tries to mimic globally defined variables that target-props take their default from, such as CMAKE_RUNTIME_OUTPUT_DIRECTORY.

MathiasMagnus · 2023-07-26T12:31:10Z

My only concern at present is adding BUILDER as a mandatory argument, I worry that will cause issues for backwards compatibility. I would prefer that this variable have a sane default (perhaps html?), but am open to arguments.

@lawruble13 BUILDER being mandatory hasn't changed. While the docs suggested it isn't mandatory, the internal logic just assumed it exists. I just clarified it instead of letting it blow up in the users' face.

pfultz2 · 2023-07-26T14:51:38Z

doc/src/ROCMSphinxDoc.rst

+  List of HTML template values passed to Sphinx. List items will be passed as
+  command-line args by prepending ``-A`` to each item.
+
+``USES_DOXYGEN``


I would prefer it to be imperative: USE_DOXYGEN.

saadrahim · 2023-08-22T13:19:07Z

test/test.cmake

@@ -1,5 +1,5 @@
 # ######################################################################################################################
-# Copyright (C) 2017 Advanced Micro Devices, Inc.
+# Copyright (C) 2023 Advanced Micro Devices, Inc.


Done. (been done for a while, just didn't mark here)

saadrahim · 2023-08-22T13:19:27Z

share/rocm/cmake/ROCMDocs.cmake

@@ -1,7 +1,9 @@
 # ######################################################################################################################
-# Copyright (C) 2021 Advanced Micro Devices, Inc.


Done. (been done for a while, just didn't mark here)

saadrahim

Minor copyright updates

MathiasMagnus · 2023-09-05T13:34:10Z

@saadrahim Should be okay.

pramenku · 2023-10-10T06:20:36Z

This change is causing MiGraphX failed to compile.

Do we need to install any pre-req @pfultz2 ?

11:18:25 -- MIGraphx is using Find-2.0 API of MIOpen
11:18:25 -- MIGraphx is using Find Mode API of MIOpen
11:18:25 �[91mCMake Error at /opt/rocm/share/rocm/cmake/ROCMSphinxDoc.cmake:10 (find_program):
11:18:25 Could not find SPHINX_EXECUTABLE using the following names: sphinx-build
11:18:25 Call Stack (most recent call first):
11:18:25 docs/CMakeLists.txt:27 (include)
11:18:25

Command to build MigraphX

pip3 install https://github.com/RadeonOpenCompute/rbuild/archive/master.tar.gz && git clone --depth=1 https://github.com/ROCmSoftwarePlatform/AMDMIGraphX --branch master && cd AMDMIGraphX && rbuild package --cxx /opt/rocm/llvm/bin/clang++ -DGPU_TARGETS="gfx906;gfx908;gfx90a;gfx1030;gfx1100;gfx1101;gfx1102" -d /workspace/AMDMIGraphX/deps -B /workspace/AMDMIGraphX/build'

pramenku · 2023-10-10T07:52:07Z

python3 -m pip install sphinx -> After installation, it's passing

But, @pfultz2 has patch #144 which will help us.
Can we get this PR merged in https://github.com/RadeonOpenCompute/rocm-cmake/tree/release-staging/rocm-rel-6.0 branch too so that it will come to 6.0 Mainline.

MathiasMagnus added 9 commits July 21, 2023 16:40

Add include guards to speed up multiple inclusion

6f5fc37

Update copyright date

a819894

Fix use of deprecated property

98e3e0d

Remove trailing space

00d0546

Update copyright date

fd75e39

Enable the use of custom Doxygen

2d676ff

Document options

81ab621

Add basic Sphinx with Doxygen unit test

ab9f989

Unit test doesn't rely on Doxygen being on PATH

b219ee4

MathiasMagnus requested review from pfultz2, lawruble13 and cgmb as code owners July 24, 2023 16:09

lawruble13 reviewed Jul 25, 2023

View reviewed changes

share/rocm/cmake/ROCMSphinxDoc.cmake Show resolved Hide resolved

lawruble13 reviewed Jul 25, 2023

View reviewed changes

pfultz2 reviewed Jul 26, 2023

View reviewed changes

MathiasMagnus added 7 commits July 31, 2023 14:49

Mark DOXYGEN_EXECUTABLE as advanced

2e0ceb4

USES_DOXYGEN -> USE_DOXYGEN

f9a957b

Introduce ROCM_CMAKE_DOCS_DIR

3be85d3

Bump rocm-docs-core to 0.20.0

02287ac

Install Sphinx doc deps

6bc9ff5

Set fake sphinx executable for linting

51fbf8f

C0301: Line too long

f8beaf8

saadrahim requested review from pfultz2 and lawruble13 August 22, 2023 13:17

saadrahim reviewed Aug 22, 2023

View reviewed changes

pfultz2 approved these changes Aug 22, 2023

View reviewed changes

lawruble13 approved these changes Aug 24, 2023

View reviewed changes

saadrahim changed the title ~~Sphinx improvement~~ Custom Doxygen Executable to improve Sphinx builds Sep 6, 2023

saadrahim changed the title ~~Custom Doxygen Executable to improve Sphinx builds~~ Custom Doxygen executable to improve Sphinx builds Sep 6, 2023

saadrahim merged commit 433d5cb into ROCm:develop Sep 6, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Doxygen executable to improve Sphinx builds #139

Custom Doxygen executable to improve Sphinx builds #139

MathiasMagnus commented Jul 24, 2023 •

edited

Loading

lawruble13 left a comment

lawruble13 Jul 25, 2023 •

edited

Loading

MathiasMagnus Jul 26, 2023

pfultz2 Jul 26, 2023

lawruble13 Jul 26, 2023

MathiasMagnus Aug 22, 2023

MathiasMagnus commented Jul 26, 2023

pfultz2 Jul 26, 2023

MathiasMagnus Jul 31, 2023

saadrahim Aug 22, 2023

MathiasMagnus Sep 5, 2023

saadrahim Aug 22, 2023

MathiasMagnus Sep 5, 2023

saadrahim left a comment

MathiasMagnus commented Sep 5, 2023

pramenku commented Oct 10, 2023

pramenku commented Oct 10, 2023

		@@ -1,7 +1,9 @@
		# ######################################################################################################################
		# Copyright (C) 2021 Advanced Micro Devices, Inc.

Custom Doxygen executable to improve Sphinx builds #139

Custom Doxygen executable to improve Sphinx builds #139

Conversation

MathiasMagnus commented Jul 24, 2023 • edited Loading

Changes

Notes

lawruble13 left a comment

Choose a reason for hiding this comment

lawruble13 Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathiasMagnus commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saadrahim left a comment

Choose a reason for hiding this comment

MathiasMagnus commented Sep 5, 2023

pramenku commented Oct 10, 2023

pramenku commented Oct 10, 2023

MathiasMagnus commented Jul 24, 2023 •

edited

Loading

lawruble13 Jul 25, 2023 •

edited

Loading