[clang-format] Add cmake target clang-format-style-options for updating ClangFormatStyleOptions.rst

[ichaer]

• Create a new clang-format-style-options build target which re-generates ClangFormatStyleOptions.rst from its source header files.

As discussed in #96804 (comment)

[ichaer]

Sample failure: https://github.com/llvm/llvm-project/actions/runs/11233429933/job/31227131778?pr=111513#step:10:127

[ichaer]

@owenca, see what you think.

[llvmbot]

@llvm/pr-subscribers-clang-format
@llvm/pr-subscribers-github-workflow

@llvm/pr-subscribers-clang

Author: Iuri Chaer (ichaer)

Changes

• Create a new clang-format-style-options build target which re-generates ClangFormatStyleOptions.rst from its source header files.
• Add CI check confirming that the repository's ClangFormatStyleOptions.rst is up-to-date, so that it doesn't have to be done by reviewers.

As discussed in #96804 (comment)

---

Full diff: https://github.com/llvm/llvm-project/pull/111513.diff

2 Files Affected:

• (modified) .github/workflows/docs.yml (+13)
• (modified) clang/docs/CMakeLists.txt (+6)

diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml
index 800e9291573533..f559fbd237a99d 100644
--- a/.github/workflows/docs.yml
+++ b/.github/workflows/docs.yml
@@ -75,6 +75,9 @@ jobs:
               - 'clang/include/clang/Basic/AttrDocs.td'
               - 'clang/include/clang/Driver/ClangOptionDocs.td'
               - 'clang/include/clang/Basic/DiagnosticDocs.td'
+            clang-format_style-headers:
+              - 'clang/include/clang/Format/Format.h'
+              - 'clang/include/clang/Tooling/Inclusions/IncludeStyle.h'
             clang-tools-extra:
               - 'clang-tools-extra/docs/**'
             lldb:
@@ -122,6 +125,16 @@ jobs:
         run: |
           cmake -B clang-build -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang" -DLLVM_ENABLE_SPHINX=ON ./llvm
           TZ=UTC ninja -C clang-build docs-clang-html docs-clang-man
+      - name: Confirm ClangFormatStyleOptions.rst is up-to-date
+        if: steps.docs-changed-subprojects.outputs.clang-format_style-headers_any_changed == 'true'
+        run: |
+          cmake -B clang-build -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang" -DLLVM_ENABLE_SPHINX=ON ./llvm
+          TZ=UTC ninja -C clang-build clang-format-style-options
+          GIT_STATUS="$(git status --porcelain=v1 clang/docs/ClangFormatStyleOptions.rst)"
+          if [ ! -z "${GIT_STATUS}" ]; then
+            echo "Error: you must build the 'clang-format-style-options' target and commit any changes to 'clang/docs/ClangFormatStyleOptions.rst'"
+            exit 1
+          fi
       - name: Build clang-tools-extra docs
         if: steps.docs-changed-subprojects.outputs.clang-tools-extra_any_changed == 'true'
         run: |
diff --git a/clang/docs/CMakeLists.txt b/clang/docs/CMakeLists.txt
index 4fecc007f59954..93a4356c1d53bb 100644
--- a/clang/docs/CMakeLists.txt
+++ b/clang/docs/CMakeLists.txt
@@ -1,3 +1,9 @@
+add_custom_target(clang-format-style-options
+    COMMAND "${Python3_EXECUTABLE}" dump_format_style.py
+    WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/tools"
+    DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/../include/clang/Format/Format.h"
+            "${CMAKE_CURRENT_SOURCE_DIR}/../include/clang/Tooling/Inclusions/IncludeStyle.h")
+add_dependencies(clang-format clang-format-style-options)
 
 if (DOXYGEN_FOUND)
 if (LLVM_ENABLE_DOXYGEN)

[boomanaiden154]

The documentation build workflow is not really the correct place to put this.

Also, why can't we just run the python script as part of building the docs instead of committing the result of the script to the repository?

[ichaer]

The documentation build workflow is not really the correct place to put this.

Yeah, I agree, but I couldn't find a place which looked correct 😅. This thing of adding built artifacts to the repo always introduces sadness... the documentation build workflow looked like the least bad place. I'll gladly take input on this!

Also, why can't we just run the python script as part of building the docs instead of committing the result of the script to the repository?

That's an excellent question, maybe @owenca will know! I should have asked in #96804 (comment); what I did instead was try to reason out an explanation based on what I could see, and I came up with two theories. The first one is that we want for ClangFormatStyleOptions.rst to be available to anyone browsing the repo... it's not very convincing, but it doesn't sound absurd. I could definitely get behind my second theory, though: we want to force everyone to review ClangFormatStyleOptions.rst before merging changes to the header files from which it is produced, and putting it in the repo is a good way to inject that into the code review process.

Anyhow, I'm proposing this because in #96804 (comment) I thought that it was an unreasonable burden for project maintainers having to manually gate on changes which may affect ClangFormatStyleOptions.rst, and also an unnecessary source of friction during code reviews, so I volunteered to automate that part.

[boomanaiden154]

That's an excellent question, maybe @owenca will know! I should have asked in #96804 (comment); what I did instead was try to reason out an explanation based on what I could see, and I came up with two theories. The first one is that we want for ClangFormatStyleOptions.rst to be available to anyone browsing the repo... it's not very convincing, but it doesn't sound absurd. I could definitely get behind my second theory, though: we want to force everyone to review ClangFormatStyleOptions.rst before merging changes to the header files from which it is produced, and putting it in the repo is a good way to inject that into the code review process.

Based on the commit history (d278e0e, https://reviews.llvm.org/D1597), it seems like the real answer might be that it just never got implemented. That looks like it was from the time where there were two build systems that both needed to be maintained at the same time, so maybe somewhat understandable it wasn't implemented that way. There is precedent for generating docs from other files (several clang docs pages are generated through tablegen. So I'd prefer to see a patch doing that rather than just asserting that it is the same. I don't think there should be issues with the Python requirement either. Most importantly, we already require a relatively recent Python in CMake, so its guaranteed to be available, and secondly building clang-format as part of a bootstrapping process doesn't make much sense.

That is my read on it, although I'm not a clang-format maintainer, so they might have more context there that I am not aware of.

Anyhow, I'm proposing this because in #96804 (comment) I thought that it was an unreasonable burden for project maintainers having to manually gate on changes which may affect ClangFormatStyleOptions.rst, and also an unnecessary source of friction during code reviews, so I volunteered to automate that part.

Yeah, I saw the thread. Thanks for looking into process improvements here.

[ichaer]

So I'd prefer to see a patch doing that rather than just asserting that it is the same.

@boomanaiden154, I'm willing to do that, but could we confirm with a clang-format maintainer first? In the course of that code review I referenced I did introduce an error in the documentation through a valid change to the header files, and @mydeveloperday was able to catch it by looking at the file generated. The error I introduced would have been hard to catch through automation. I suppose the same could be said for all of the human-consumable artifacts, all the docs, but my impression is that this is a special case, user-facing documentation produced from comments in the source code.

My instinct is to agree with you and not version-control artifacts, I just want to make sure that the folks who are dealing with that part of the codebase more frequently agree that change would be net positive.

[owenca]

Also, why can't we just run the python script as part of building the docs instead of committing the result of the script to the repository?

Because only part of the doc is generated by dump_format_style.py. See

llvm-project/clang/docs/ClangFormatStyleOptions.rst

Line 3 in 1809d0f

This file is automatically generated, in part. Do not edit the style options

.

[owenca]

@owenca, see what you think.

IMO it would be more helpful to add the build target to lib/Format/CMakeLists.txt instead. For example,

$ ninja FormatTests
[1/1] cd /Users/Owen/llvm-project/clang....12/bin/python3.12 dump_format_style.p
$ 

I'm ok with not adding the CI check though.

[ichaer]

IMO it would be more helpful to add the build target to lib/Format/CMakeLists.txt instead. For example,

$ ninja FormatTests

Moving the target from clang/docs/CMakeLists.txt to lib/Format/CMakeLists.txt sounds great to me, but the testing part of the thing relies on us being in a git clone, it's a very CI-oriented thing, my impression is that it's more appropriate for it to live in the workflow yaml files. I'm saying this because you suggested FormatTests as the name... I'm not sure if clang-format-style-options is a good name, but I think it's closer to what we can do there. What do you think?

I'm ok with not adding the CI check though.

IMO that would be sad, I would really like to free maintainers up from having to remember this dependency themselves. Is this because you feel .github/workflows/docs.yml is the wrong place for that validation? I see that step more as a pre-build validation for docs than as a test, that's why docs.yml looks like a good home for it from my point of view, but we could move it elsewhere if you're uncomfortable with that.

[owenca]

IMO it would be more helpful to add the build target to lib/Format/CMakeLists.txt instead. For example,

$ ninja FormatTests

Moving the target from clang/docs/CMakeLists.txt to lib/Format/CMakeLists.txt sounds great to me, but the testing part of the thing relies on us being in a git clone, it's a very CI-oriented thing, my impression is that it's more appropriate for it to live in the workflow yaml files. I'm saying this because you suggested FormatTests as the name... I'm not sure if clang-format-style-options is a good name, but I think it's closer to what we can do there. What do you think?

I wasn't suggesting a different name as FormatTests is an existing build target. See

llvm-project/clang/unittests/Format/CMakeLists.txt

Line 41 in 0fba838

clang_target_link_libraries(FormatTests

Actually, I like the clang-format-style-options name.

Anyway, my current workflow is:

1. Edit files in clang/lib/Format and clang/unittests/Format.
2. Edit clang/include/clang/Format/Format.h if needed.
3. Run ninja FormatTests.
4. Run cd clang/docs/tools && dump_format_style.py if Step 2 above wasn't skipped.
5. Run the unit tests.
6. Run git commit -a.

It would be nice if I didn't have to run step 4 manually.

I'm ok with not adding the CI check though.

IMO that would be sad, I would really like to free maintainers up from having to remember this dependency themselves. Is this because you feel .github/workflows/docs.yml is the wrong place for that validation? I see that step more as a pre-build validation for docs than as a test, that's why docs.yml looks like a good home for it from my point of view, but we could move it elsewhere if you're uncomfortable with that.

I'm not opposed to adding the CI check either.

[ichaer]

It would be nice if I didn't have to run step 4 manually.

Got it! So ideally we'd move clang-format-style-options lower down the stack, to a target shared between clang-format and FormatTests.

I've made it a dependency of the clangFormat library, let me know what you think :).

[boomanaiden154]

Because only part of the doc is generated by dump_format_style.py.

It should still be possible to only include the non-generated parts in the repo and then modify the script to add all the generated components on afterwards as a build step? Not sure if I'm missing something here.

[owenca]

Because only part of the doc is generated by dump_format_style.py.

It should still be possible to only include the non-generated parts in the repo and then modify the script to add all the generated components on afterwards as a build step? Not sure if I'm missing something here.

Sometimes we need to edit the non-generated parts, and it would be easier to have the entire contexts. Anyway, don't we need the generated RST file in the repo so that the corresponding HTML file (e.g. https://clang.llvm.org/docs/ClangFormatStyleOptions.html) will be readily available?

[owenca]

@ichaer I suggest that you split the workflow part to another patch so that we can get the cmake part approved.

[boomanaiden154]

#113739 should fix this problem by just adding the options as part of the build and omitting them from the documentation source.

[ichaer]

@ichaer I suggest that you split the workflow part to another patch so that we can get the cmake part approved.

It's a bit sad, but I get your point, let's make it more focused.

[Andarwinux]

If CLANG_INCLUDE_DOCS=OFF then this should be skipped, otherwise the build will fail if llvm/clang/docs doesn't exist.

[nikic]

Reverted this because it breaks the build. I initially assumed this is due to disabled docs per the previous comment, but I think in my case the problem is actually that this seems to be trying to write to the source directory, which is forbidden. If you need to write out files, that has to happen in the build directory.

FAILED: /var/llvm-compile-time-tracker/llvm-project/clang/docs/ClangFormatStyleOptions.rst 
cd /var/llvm-compile-time-tracker/llvm-project/clang/docs/tools && /usr/bin/python3.10 dump_format_style.py
Traceback (most recent call last):
  File "/var/llvm-compile-time-tracker/llvm-project/clang/docs/tools/dump_format_style.py", line 23, in <module>
    with open(PLURALS_FILE, "a+") as f:
PermissionError: [Errno 13] Permission denied: '/var/llvm-compile-time-tracker/llvm-project/clang/docs/tools/plurals.txt'

[nikic]

Reading back here, the general thing you are trying to do here is not allowed. At best you can add this as a manually invoked target that is not implicitly called by anything else. Though the actually correct way to handle this is what was proposed in #113739.

[owenca]

FAILED: /var/llvm-compile-time-tracker/llvm-project/clang/docs/ClangFormatStyleOptions.rst 
cd /var/llvm-compile-time-tracker/llvm-project/clang/docs/tools && /usr/bin/python3.10 dump_format_style.py
Traceback (most recent call last):
  File "/var/llvm-compile-time-tracker/llvm-project/clang/docs/tools/dump_format_style.py", line 23, in <module>
    with open(PLURALS_FILE, "a+") as f:
PermissionError: [Errno 13] Permission denied: '/var/llvm-compile-time-tracker/llvm-project/clang/docs/tools/plurals.txt'

Changed the mode to read-only in 74d29c6.

[owenca]

If CLANG_INCLUDE_DOCS=OFF then this should be skipped, otherwise the build will fail if llvm/clang/docs doesn't exist.

This was fixed in f0b09df, which also addressed an issue that ClangFormatStyleOptions.rst would be deleted by ninja clean.