Closed Tachi107 closed 2 years ago
Hi and thanks for the PR!
There's a few problems that I see with letting CMake generate this:
make
, which I find annoying because Doxygen spams a ton of stuff into my terminal, making it hard to see the actual relevant warnings from the C compiler
OFF
ZYDIS_BUILD_DOC
option and/or cranking down Doxygen's verbosityON
and silencing Doxygen to only print errors. Documentation generation is sufficiently fast to not be annoying during development.zydoc
behavior: zydoc
re-builds the documentation for every Zydis version on each build. It takes the Doxyfile from master
and uses that to build documentation for all versions. The idea here was to have consistent documentation theming and configs across all versions. Admittedly, this approach has its drawbacks -- e.g. breaking Google-indexed links if Doxygen changes HTML generation -- but I'm also not looking forward to spending time on re-engineering this.
The documentation is now built every time I type
make
, which I find annoying because Doxygen spams a ton of stuff into my terminal, making it hard to see the actual relevant warnings from the C compiler
- This can probably easily be fixed by introducing a default-
OFF
ZYDIS_BUILD_DOC
option and/or cranking down Doxygen's verbosity
That's true, and I find it quite annoying too. I'd personally make Doxygen quieter instead of adding a new build option, as I don't see why you wouldn't want to generate documentation if Doxygen is found on the host, and also because if you really want to you can already tell CMake to treat Doxygen as not found with -DCMAKE_DISABLE_FIND_PACKAGE_Doxyen=true
. Verbosity can be reduced with Doxygen's QUIET
option.
- The configuration is now duplicated in two places, and without the duplication there is no obvious way forward to retain the current
zydoc
behavior:zydoc
re-builds the documentation for every Zydis version on each build. It takes the Doxyfile frommaster
and uses that to build documentation for all versions. The idea here was to have consistent documentation theming and configs across all versions. Admittedly, this approach has its drawbacks -- e.g. breaking Google-indexed links if Doxygen changes HTML generation -- but I'm also not looking forward to spending time on re-engineering this.
When looking at zydoc
I didn't realize that it builds documentation for all branches, so my suggestion to tell zydoc
to use already generated documentation is not actionable :/
I could write a little CMake script to tell CMake to import the options from the Doxyfile; does this sound reasonable to you?
Edit: done, the options are now read from the Doxyfile; here's the updated commit message: _Using doxygen_add_docs()
requires setting the various Doxygen options in CMakeLists.txt, so to avoid duplicating things in different places I wrote a bit of CMake code that parses the Doxyfile and sets the various options as CMake variables prefixed by DOXYGEN_. The "parser" is really basic, and does not handle values that span over multiple lines._
Il giorno dom 2 ott 2022 alle 11:21:38 -07:00:00, Joel Höner @.***> ha scritto:
There appears to be an issue with the README.md being picked up, though. The index.html page is blank for me when documentation is built on your branch.
Thanks for noticing this, I somehow forgot to check that the parser actually worked 😅️
The issue was caused by the fact that option_name contained trailing whitespace, and is now fixed. As for the reason why option, option_name and option_value are lowercase that's because they are only used as temporary variables inside the loop, and are not result variables (unlike DOC_PATHS, for examples). This seems to be a CMake convention I've seen a few times in Find Modules.
-- OpenPGP key: 66DE F152 8299 0C21 99EF A801 A8A1 28A8 AB1C EE49
Thanks to you for being so collaborative :D
Here are the two commit messages:
This patch adds a doc target generating Doxygen documentation similarly to https://github.com/zyantific/zycore-c/pull/51, but with some extra bits: if doxygen-awesome.css is found on the host system, CMake will automatically use it to generate themed documentation.
Using
doxygen_add_docs()
requires setting the various Doxygen options in CMakeLists.txt, so there's currently a bit of duplication as the same options are also contained in Doxyfile and Doxyfile-themed. I couldn't remove the two standalone files as they are still required by the doc.yml CI/CD pipeline, aszydoc
runs Doxygen itself; as a solution,zydoc
could add support for a--doxygen-generated <path>
flag that tells it to use already generated Doxygen documentation instead of generating its own.This patch tweaks the Makefile a bit to make it use the new doc target.
A new
configure
target was added, that simply runscmake -B build_dir
, so thatmake doc
doesn't require building the full project.I've also made a few minor changes like using
cmake --build
andcmake --install
instead of plainmake
, as they lead to more concise commands and also make switching build backend easier (e.g. you simply need to add-G Ninja
to theconfigure
target, without having to replace all themake
calls toninja
ones).Another fix was changing
&> /dev/null
to> /dev/null
, as they broke the if statement on my system, and also because POSIX mandates thatcommand -v
should print to stdout on success, and shouldn't print anything on error (i.e. no stderr is involved).