[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
boomanaiden154 wrote: Closing in favor of #111513 / #118154. https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
https://github.com/boomanaiden154 closed https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
owenca wrote: > Can you explicate on what your requirements are here? I don't see why just > wrapping the python script in CMake does anything tangible here. Having a > build target that modifies the source directory is also weird and something > that we should avoid if possible, in my opinion. >From https://github.com/llvm/llvm-project/pull/111513#issuecomment-2408412143: > 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. With #111513, I no longer need to remind myself to run the python script when testing a patch. Please note that we rarely need to do `ninja check-clang-format` to run the lit tests, which takes an extra hour or two. https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
boomanaiden154 wrote: > That and to also ensure that the edited Format.h doesn't break the python > script. Makes sense. #118154 would cover that too. > If we are to leave the generated part of the rst file in the repo, > https://github.com/llvm/llvm-project/pull/111513 would satisfy my > requirements nicely. Something like > https://github.com/llvm/llvm-project/pull/118154 may be a useful addition. Can you explicate on what your requirements are here? I don't see why just wrapping the python script in CMake does anything tangible here. Having a build target that modifies the source directory is also weird and something that we should avoid if possible, in my opinion. > That would be nice. https://github.com/llvm/llvm-project/pull/118159 https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
owenca wrote: > What's the point of adding it to the `FormatTests` target though? Just to > ensure proper test coverage of the `dump_format_style.py` file? That and to also ensure that the edited Format.h doesn't break the python script. > Thinking about this a bit more, I think we can probably satisfy all the > constraints if we write a lit test that ensures the in-tree > ClangFormatStyleOptions.rst matches what the script will produce > (https://github.com/llvm/llvm-project/pull/118154)? That solves the original > problem that https://github.com/llvm/llvm-project/pull/111513 set out to > solve (if I remember correctly) and will show up as a failure in CI if not > done. If we are to leave the generated part of the rst file in the repo, #111513 would satisfy my requirements nicely. Something like #118154 may be a useful addition. > I'm also planning on making the documentation build action upload the built > docs as artifacts, which would also let reviewers just download the built > HTML files and inspect those if that's easier/useful. That would be nice. https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
boomanaiden154 wrote: > Whenever I review a pull request that has edited Format.h, I always fetch the > PR and run the python script to ensure that the generated rst file matches > the one in git. I don't think there's a way around that. Thinking about this a bit more, I think we can probably satisfy all the constraints if we write a lit test that ensures the in-tree `ClangFormatStyleOptions.rst` matches what the script will produce (https://github.com/llvm/llvm-project/pull/118154)? That solves the original problem that #111513 set out to solve (if I remember correctly) and will show up as a failure in CI if not done. That doesn't get rid of the generated code being in the repo, but it seems like that is an essential part of some review workflows currently. I'm also planning on making the documentation build action upload the built docs as artifacts, which would also let reviewers just download the built HTML files and inspect those if that's easier/useful. https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
owenca wrote: > As a reviewer that means we have to download the review, rebuild the rst then > check it. I don't quite understand the problem we are trying to solve here? > Given that the python script unpacks the Format.h and makes the rst I would > prefer to see the finished rst as part of the review. Whenever I review a pull request that has edited Format.h, I always fetch the PR and run the python script to ensure that the generated rst file matches the one in git. I don't think there's a way around that. So I'm ok with reducing the current rst file to a template and removing thousands of lines from the repo. The alternative would be #111513, which leaves the rst file in tact. https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
mydeveloperday wrote: > > I'd personally like a solution that doesn't remove the > > ClangFormatStyleOptions.rst from the review > > Could we just test that the output looks as expected? I think it would be > pretty easy to write a lit test that asserts all of the output is as > expected. That would then show up in review. > > It would be a bit of a hack, but it would solve the original problem, making > sure that the docs actually get updated with the script automatically, and > that things show up in review. As a reviewer that means we have to download the review, rebuild the rst then check it. I don't quite understand the problem we are trying to solve here? Given that the python script unpacks the Format.h and makes the rst I would prefer to see the finished rst as part of the review. Sorry this doesn't fit your agenda but your not the one doing the reviews! https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
boomanaiden154 wrote: > I'd personally like a solution that doesn't remove the > ClangFormatStyleOptions.rst from the review Could we just test that the output looks as expected? I think it would be pretty easy to write a lit test that asserts all of the output is as expected. That would then show up in review. It would be a bit of a hack, but it would solve the original problem, making sure that the docs actually get updated with the script automatically, and that things show up in review. https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
https://github.com/mydeveloperday requested changes to this pull request. Ok so here now lie a problem, we'll not "see the ClangFormatStyleOptions.rst" during the review, so we'll not be able to sanity check the code in Format.h has been correctly generated. I feel this is a backward step and will likely lead to invalid documentation. I'd personally like a solution that doesn't remove the ClangFormatStyleOptions.rst from the review https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
boomanaiden154 wrote: > I'm not concerned about how much time it takes to run. I want to add it to > clangFormat's dependencies if it doesn't run every time I do ninja > FormatTests. I've updated the patch so that we only rerun the python script if the dependencies change. I've added a `.template` extension to the docs file so that it doesn't interfere with the copy in the build directory. It does not seem like there's an easy way to get CMake to copy the docs directory to the build directory while excluding that individual file. So renaming it prevents Sphinx from picking up the templatized version. What's the point of adding it to the `FormatTests` target though? Just to ensure proper test coverage of the `dump_format_style.py` file? This seems like something that more should be covered by the documentation builds, which occur both precommit and postcommit with the precommit builds having reasonably nice UI. https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
owenca wrote: > > can we have clang-format-style-options rebuilt only when it's outdated, > > i.e. only if at least one of the following has been updated? > > We can, but it requires more changes that I'm not sure are worth doing given > that this step takes essentially zero time compared to regenerating the docs > with Sphinx, which already does run every time. I'm not concerned about how much time it takes to run. I want to add it to `clangFormat`'s dependencies if it doesn't run every time I do `ninja FormatTests`. Can something similar to https://github.com/llvm/llvm-project/pull/111513/files#r1832152957 be done here? https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
owenca wrote: @boomanaiden154 can we have `clang-format-style-options` rebuilt only when it's outdated, i.e. only if at least one of the following has been updated? ``` clang/include/clang/Format/Format.h clang/include/clang/Tooling/Inclusions/IncludeStyle.h clang/docs/ClangFormatStyleOptions.rst clang/docs/tools/dump_format_style.py ``` https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
boomanaiden154 wrote: > can we have clang-format-style-options rebuilt only when it's outdated, i.e. > only if at least one of the following has been updated? We can, but it requires more changes that I'm not sure are worth doing given that this step takes essentially zero time compared to regenerating the docs with Sphinx, which already does run every time. https://github.com/llvm/llvm-project/pull/113739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
@@ -143,8 +143,17 @@ if (LLVM_ENABLE_SPHINX)
gen_rst_file_from_td(DiagnosticsReference.rst -gen-diag-docs
../include/clang/Basic/Diagnostic.td "${docs_targets}")
gen_rst_file_from_td(ClangCommandLineReference.rst -gen-opt-docs
../include/clang/Driver/ClangOptionDocs.td "${docs_targets}")
+add_custom_target(clang-format-style-options
+ COMMAND "${Python3_EXECUTABLE}"
"${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+ DEPENDS "${CLANG_SOURCE_DIR}/include/clang/Format/Format.h"
+
"${CLANG_SOURCE_DIR}/include/clang/Tooling/Inclusions/IncludeStyle.h"
+ "${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ copy-clang-rst-docs
+)
+
foreach(target ${docs_targets})
- add_dependencies(${target} copy-clang-rst-docs)
+ add_dependencies(${target} copy-clang-rst-docs
clang-format-style-options)
boomanaiden154 wrote:
We add the dependency in the `add_custom_target` call for `copy-clang-rst-docs`.
https://github.com/llvm/llvm-project/pull/113739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
@@ -143,8 +143,17 @@ if (LLVM_ENABLE_SPHINX)
gen_rst_file_from_td(DiagnosticsReference.rst -gen-diag-docs
../include/clang/Basic/Diagnostic.td "${docs_targets}")
gen_rst_file_from_td(ClangCommandLineReference.rst -gen-opt-docs
../include/clang/Driver/ClangOptionDocs.td "${docs_targets}")
+add_custom_target(clang-format-style-options
+ COMMAND "${Python3_EXECUTABLE}"
"${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+ DEPENDS "${CLANG_SOURCE_DIR}/include/clang/Format/Format.h"
+
"${CLANG_SOURCE_DIR}/include/clang/Tooling/Inclusions/IncludeStyle.h"
+ "${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ copy-clang-rst-docs
boomanaiden154 wrote:
The `clang-format-style-options` custom target does. It's the last line in the
dependencies listed.
https://github.com/llvm/llvm-project/pull/113739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
@@ -143,8 +143,17 @@ if (LLVM_ENABLE_SPHINX)
gen_rst_file_from_td(DiagnosticsReference.rst -gen-diag-docs
../include/clang/Basic/Diagnostic.td "${docs_targets}")
gen_rst_file_from_td(ClangCommandLineReference.rst -gen-opt-docs
../include/clang/Driver/ClangOptionDocs.td "${docs_targets}")
+add_custom_target(clang-format-style-options
+ COMMAND "${Python3_EXECUTABLE}"
"${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
owenca wrote:
```suggestion
set (dump_format_style ${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py)
add_custom_target(clang-format-style-options
COMMAND ${Python3_EXECUTABLE} ${dump_format_style}
```
See below.
https://github.com/llvm/llvm-project/pull/113739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
@@ -143,8 +143,17 @@ if (LLVM_ENABLE_SPHINX)
gen_rst_file_from_td(DiagnosticsReference.rst -gen-diag-docs
../include/clang/Basic/Diagnostic.td "${docs_targets}")
gen_rst_file_from_td(ClangCommandLineReference.rst -gen-opt-docs
../include/clang/Driver/ClangOptionDocs.td "${docs_targets}")
+add_custom_target(clang-format-style-options
+ COMMAND "${Python3_EXECUTABLE}"
"${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+ DEPENDS "${CLANG_SOURCE_DIR}/include/clang/Format/Format.h"
+
"${CLANG_SOURCE_DIR}/include/clang/Tooling/Inclusions/IncludeStyle.h"
+ "${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ copy-clang-rst-docs
owenca wrote:
```suggestion
${CLANG_SOURCE_DIR}/docs/ClangFormatStyleOptions.rst
${dump_format_style}
```
It doesn't depend on `copy-clang-rst-docs`?
https://github.com/llvm/llvm-project/pull/113739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
@@ -185,6651 +179,10 @@ the configuration (without a prefix: ``Auto``).
subdirectories. This is also possible through the command line, e.g.:
``--style={BasedOnStyle: InheritParentConfig, ColumnLimit: 20}``
+.. This section of the file is automatically generated by the
+ dump_format_style.py tool and should be updated manually.
.. START_FORMAT_STYLE_OPTIONS
owenca wrote:
```suggestion
.. START_FORMAT_STYLE_OPTIONS
..
This section is automatically generated by the dump_format_style.py script and
should not be updated manually.
```
https://github.com/llvm/llvm-project/pull/113739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
@@ -143,8 +143,17 @@ if (LLVM_ENABLE_SPHINX)
gen_rst_file_from_td(DiagnosticsReference.rst -gen-diag-docs
../include/clang/Basic/Diagnostic.td "${docs_targets}")
gen_rst_file_from_td(ClangCommandLineReference.rst -gen-opt-docs
../include/clang/Driver/ClangOptionDocs.td "${docs_targets}")
+add_custom_target(clang-format-style-options
+ COMMAND "${Python3_EXECUTABLE}"
"${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+ DEPENDS "${CLANG_SOURCE_DIR}/include/clang/Format/Format.h"
+
"${CLANG_SOURCE_DIR}/include/clang/Tooling/Inclusions/IncludeStyle.h"
+ "${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ copy-clang-rst-docs
+)
+
foreach(target ${docs_targets})
- add_dependencies(${target} copy-clang-rst-docs)
+ add_dependencies(${target} copy-clang-rst-docs
clang-format-style-options)
owenca wrote:
Shouldn't it be something like the following?
```
add_dependencies(clang-format-style-options copy-clang-rst-docs)
foreach(target ${docs_targets})
add_dependencies(${target} copy-clang-rst-docs)
```
https://github.com/llvm/llvm-project/pull/113739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format][CMake] Generate formatting options docs during build (PR #113739)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Aiden Grossman (boomanaiden154)
Changes
This patch changes up the CMake configuration so that
ClangFormatStyleOptions.rst has the format style options section generated by
the dump_format_style.py python script during the build rather than manually.
This allows us to remove ~6000 lines of automatically generated documentation
from the repository.
If the context of the options is needed while generating the docs, it is easy
enough to open up the generated file with the options from within the build
directory after the clang-format-style-options target has been built.
---
Patch is 181.72 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/113739.diff
3 Files Affected:
- (modified) clang/docs/CMakeLists.txt (+10-1)
- (modified) clang/docs/ClangFormatStyleOptions.rst (+2-6649)
- (modified) clang/docs/tools/dump_format_style.py (+1-1)
``diff
diff --git a/clang/docs/CMakeLists.txt b/clang/docs/CMakeLists.txt
index 4fecc007f59954..313b0efd1dbd64 100644
--- a/clang/docs/CMakeLists.txt
+++ b/clang/docs/CMakeLists.txt
@@ -143,8 +143,17 @@ if (LLVM_ENABLE_SPHINX)
gen_rst_file_from_td(DiagnosticsReference.rst -gen-diag-docs
../include/clang/Basic/Diagnostic.td "${docs_targets}")
gen_rst_file_from_td(ClangCommandLineReference.rst -gen-opt-docs
../include/clang/Driver/ClangOptionDocs.td "${docs_targets}")
+add_custom_target(clang-format-style-options
+ COMMAND "${Python3_EXECUTABLE}"
"${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+ DEPENDS "${CLANG_SOURCE_DIR}/include/clang/Format/Format.h"
+
"${CLANG_SOURCE_DIR}/include/clang/Tooling/Inclusions/IncludeStyle.h"
+ "${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+ copy-clang-rst-docs
+)
+
foreach(target ${docs_targets})
- add_dependencies(${target} copy-clang-rst-docs)
+ add_dependencies(${target} copy-clang-rst-docs
clang-format-style-options)
endforeach()
endif()
endif()
diff --git a/clang/docs/ClangFormatStyleOptions.rst
b/clang/docs/ClangFormatStyleOptions.rst
index 9ef1fd5f36d1d3..508a058de6f768 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -1,9 +1,3 @@
-..
- NOTE
- This file is automatically generated, in part. Do not edit the style options
- in this file directly. Instead, modify them in include/clang/Format/Format.h
- and run the docs/tools/dump_format_style.py script to update this file.
-
.. raw:: html
