Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PoC] Documentation generation #275

Merged
merged 14 commits into from
Jun 10, 2024

Conversation

eliemichel
Copy link
Collaborator

@eliemichel eliemichel commented Mar 3, 2024

Starting from @rajveermalviya gen scripts, I did a draft of a documentation generation (using MkDocs for the final HTML generation).

The generated doc can be seen here: https://eliemichel.github.io/webgpu-headers

This is a Proof of Concept rather than something to merge as is. Some notable points:

  • What do we expect the URLs to be? I was thinking maybe having a api/Foo to work with any function or object type Foo so that it is easy to forge urls, but at the same time the current state where functions are grouped by object make sense as well. Maybe there could be a global redirection mechanism on top of the more hierarchical approach
  • I used the default MkDocs template, of course we can use a different one.

Edit. Maybe this could be an opportunity to write up a wish list about the future documentation. The documentation of WebGPU Native should be:

  • easy to access It should be on a correctly referenced website, be clear about versionning, provide permalinks, and ideally enable one to forge (guess) the permalinks from the name of the symbol they want to know about. There should be a way to download it for offline consultation.
  • easy to read Each documentation page should not be cumbersome and go straight to the point. All pages must be consistent in their formatting. It is an API reference, not a tutorial, nevertheless there could be an introduction of the key terms/concepts needed to understand the API.
  • easy to edit Writing documentation requires many pairs of eyes to proof read. It is common that one who consults the documentation spots a typo: since they were likely consulting the documentation on their journey to another unrelated goal, the detour to fix documentation should be as fast as possible.

@rajveermalviya rajveermalviya added the !discuss Needs discussion (at meeting or online) label Mar 3, 2024
@kainino0x
Copy link
Collaborator

I think I would like to generate webgpu.h with Doxygen-style headers, as this would be the most useful in IDEs. Not to discount your work, but if we do that, then it might make sense to generate documentation using Doxygen (or some other doc generator that can ingest Doxygen-style headers, maybe Mkdocs can do it). What do you think?

@kainino0x
Copy link
Collaborator

Oh, I see from the failing check that this is also generating the doxygen-style comments.
Another thought: I think we would still want to run Doxygen on the header even if we don't use it for doc generation, to validate syntax.

@eliemichel
Copy link
Collaborator Author

eliemichel commented Mar 10, 2024

Oh, I see from the failing check that this is also generating the doxygen-style comments.

Yes this is actually already the case in @rajveermalviya's code in main, just ignoring it in case the doc equals "TODO" ;)

It might make sense to generate documentation using Doxygen

That's indeed part of the open questions arising from this first PoC indeed. I considered the option of going through Doxygen indeed, I'd say that there are pros and cons to using Doxygen:

  • Pro We want to generate the docstrings anyway because they are important for IDEs
  • Pro Doxygen is a pretty standard tool, that integrates well with documentation generators such as mkdocs (MkDoxy) or sphinx (breathe).
  • Cons Our webgpu.yml already splits documentation into a different field for brief, arguments, erturn values, etc. so it's somehow useless to synthesize docstrings only to parse them back (besides verification).
  • Cons By directly generating the doc from webgpu.yml we could benefit from having a little more flexibility for automatically adding annotations specific to some WebGPU idioms (e.g., nullable). This is the main point that motivated me to go without Doxygen for this first test.
  • Cons It should be as easy as possible for a contributor to fix typos and imprecision to the documentation as they are consulting it. Requiring to add yet another tool (doxygen) could add more friction and reduce contribution. But this is a more general concern actually since there is already Go and Make to install.

I admit that I may also be biased by the way the raw HTML output of doxygen looks, but there may be ways to get it right.

Another thought: I think we would still want to run Doxygen on the header even if we don't use it for doc generation, to validate syntax.

Agreed for sure!

Not to discount your work

No worry, I'm aware that this is an early test that may end up entierly dropped! As long as it helps asking the right questions it's fine with me.

@eliemichel
Copy link
Collaborator Author

Added a note in the first comment about the intended goals of the documentation. The documentation of WebGPU Native should be:

  • easy to access It should be on a correctly referenced website, be clear about versionning, provide permalinks, and ideally enable one to forge (guess) the permalinks from the name of the symbol they want to know about. There should be a way to download it for offline consultation.
  • easy to read Each documentation page should not be cumbersome and go straight to the point. All pages must be consistent in their formatting. It is an API reference, not a tutorial, nevertheless there could be an introduction of the key terms/concepts needed to understand the API.
  • easy to edit Writing documentation requires many pairs of eyes to proof read. It is common that one who consults the documentation spots a typo: since they were likely consulting the documentation on their journey to another unrelated goal, the detour to fix documentation should be as fast as possible.

This should drive the choice of technical tool, rather than the other way around. :)

@kainino0x
Copy link
Collaborator

We (@rajveermalviya and Dawn people) discussed this in a meeting and here's our suggestion:

Mar 14 meeting:

  • RM: Should we generate doc comments or just have separate docs? Vulkan only has separate docs for example.
  • KN/AE/LK: Prefer generating doc comments for IDEs.
  • KN: Nice that if people generate doxygen docs for their project that includes webgpu.h, they’ll get the same docs that we publish online.
  • KN: Also don’t have to maintain a separate documentation generator. Doxygen support has to be maintained either way.
  • RM: Doxygen supports extra markdown files?
    • KN: I think so, may be able to put all documentation just in the header though. I think this would be easier to manage if it works.
  • RM: Also simplifies generating documentation for extension headers, they don’t need to set up documentation generator for them too, can just point Doxygen at the additional header files.
  • Use Doxygen to generate documentation, try to put all documentation in header, but add extra .md files if necessary.

@eliemichel
Copy link
Collaborator Author

Okey let's give a shot to Doxygen then!

I carried this in a separate repo so we can easily compare:

So far the outcome of my tests:

  • The Doxygen Awesome theme definitly improves over the default theme.
  • Hard to get nice URLs (i.e., that don't look like struct_w_g_p_u_bind_group_layout_descriptor.html for WGPUBindGroupLayoutDescriptor.
  • By default Doxygen's parser doesn't expand macros so the WGPU_ENUM_ATTRIBUTE and other similar defines cause issues, e.g., it leads to all typedefs being called "WGPU_ENUM_ATTRIBUTE". It can be tuned with specific options, but when webgpu.h gets embedded into third party projects this would easily lead to broken doc.
  • A lot of pages still look messy/cluttered (e.g., enum pages) after some time spend on configuration.

@kainino0x
Copy link
Collaborator

Mar 28 meeting:

  • Biggest problem is Doxygen can’t parse our header (because of the ATTRIBUTE macros) without a special option enabled
  • Probably fine if people have to enable that option (if they want to generate documentation for our header - many projects may not be generating documentation for third party stuff)
    • RM found it’s very common on GitHub
  • Still good reasons to use Doxygen, other problems don’t seem bad enough to avoid it

I'll add, the most important thing was that the documentation is passing through Doxygen-style doc comments (to make sure IDE support works well and avoid extra maintenance work). So, if you think we should use a different tool like MkDoxy/Breathe to generate the actual documentation we publish, that would most likely be fine.

Aside, I tried looking up if it was possible to use doc comments to direct Doxygen to expand specific macros. Unfortunately it seems not.

@kainino0x
Copy link
Collaborator

My comments on the generated documentation itself:

  • It looks nice!
    • It's confusing that the little triangle dropdowns are invisible by default
  • Briefly confused why WGPUAdapter was the only thing listed in the dropdown but that seems to just be because it has placeholder documentation and the others don't.
  • The defgroups are good
    • Though I wonder if there's any way we could get the Methods listed with their associated Object (if not, just defgrouping them would be OK). Unfortunately we can't order the file in that order because the objects need to be defined first.
  • Any way to get the enums to keep the line breaks instead of wrapping?
  • "Structures" and "Data structures" seem redundant

@kainino0x kainino0x removed the !discuss Needs discussion (at meeting or online) label Mar 28, 2024
@eliemichel
Copy link
Collaborator Author

eliemichel commented Mar 31, 2024

Thx for the feedback! I'll do the suggested updates.

The main thing that still bother me are URLs. I really believe that it is important to have clear forgeable URLs in an API spec, I'll see what we can do.

Regarding the content of the doc, I was thiking it could be interesting to organize a little "doc jam"/"docathon"/whatever to have people help copy relevant parts of the JS doc in webgpu.yml, what do you think about it? I experimented with a dedicated repo that would automatically accept PRs that only affect webgpu.yml and update the web doc, and shared the idea on the Discord community of my LearnWebGPU guide.

@kainino0x
Copy link
Collaborator

Regarding the content of the doc, I was thiking it could be interesting to organize a little "doc jam"/"docathon"/whatever to have people help copy relevant parts of the JS doc in webgpu.yml, what do you think about it? I experimented with a dedicated repo that would automatically accept PRs that only affect webgpu.yml and update the web doc, and shared the idea on the Discord community of my LearnWebGPU guide.

I have some ideas about how we should do this, in particular if possible we should reuse / pull in / reference what we already have in the JS spec.

Let's discuss that on #25

@kainino0x
Copy link
Collaborator

kainino0x commented Apr 11, 2024

The main thing that still bother me are URLs. I really believe that it is important to have clear forgeable URLs in an API spec, I'll see what we can do.

Any progress on this? My guess is the only way would be to generate docs using something other than Doxygen (but still using the doc comments). Things I found online seemed to say Doxygen couldn't do this.

That said, I would like to start getting changes merged, even if we generate using Doxygen for now. We can just hold off on actually publishing docs (on github pages) until we have the permalinks we want.

@eliemichel
Copy link
Collaborator Author

Since we're sure we'll go for Doxygen at least internally to make sure docstrings are correctly generated, I switched this PR to the doxygen version and cleaned it up a little bit to be able to merge things!

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great, just a few small things before I think we should land this initial work!

.github/workflows/gen-docs.yml Outdated Show resolved Hide resolved
.github/workflows/gen-docs.yml Show resolved Hide resolved
.github/workflows/gen-docs.yml Outdated Show resolved Hide resolved
.github/workflows/gen-docs.yml Show resolved Hide resolved
Doxyfile Outdated Show resolved Hide resolved
Doxyfile Outdated Show resolved Hide resolved
Doxyfile Outdated Show resolved Hide resolved
gen/cheader.tmpl Outdated Show resolved Hide resolved
gen/cheader.tmpl Outdated Show resolved Hide resolved
gen/cheader.tmpl Outdated Show resolved Hide resolved
@eliemichel
Copy link
Collaborator Author

Thanks for the feedback @kainino0x! I addressed all of your points. I had left the Doxyfile to what doxygen's initialization generates but I agree it is better to only leave what we set to non default values. I tried to document each choice in the comments

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few failing checks:

  • webgpu.h needs to be regenerated
  • doc/generated/html: Cannot open: No such file or directory

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me think: Would it be possible to include this theme as a submodule instead of copying the files in?

Copy link
Collaborator Author

@eliemichel eliemichel May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personnally find it easier to maintain copy pasted code with only what we need from a repo rather than to use submodules (no need to warn users about the presence of submodules, no need to download the whole git history of the other repo, no need for half the repo -- though in this case its ok, no need to maintain a mirror in case the third party force pushes, etc.) but we can easily switch to a submodule pointing to https://github.com/jothepro/doxygen-awesome-css indeed!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A submodule could make sense here though, because since the raw doxygen output is not satisfying due to dirty URLs, we may want to avoid poluting the git history with theme files that we may eventually get rid of in favor of a different solution (I've started looking at sphinx-breathe, not ideal neither, the more I work on this the more I lean towards heading back to a Doxygen-less generation)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too worried about a submodule being annoying, fortunately not very many people will need to check out and build the docs for this repo. (And you don't even need to subrepo if you're not building docs)

That said, if you think the theme is likely to be short-lived since we'll replace Doxygen or replace the theme, how about we just start without a theme? The output may initially be ugly, but we can improve it later.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @eliemichel I'd love to get this landed, what do you think of the proposal to remove the theme?

I'd be happy to implement it and land the PR if you don't have a chance to do it.

We'll potentially look into theming (or using another doxygen-compatible
documentation generator) later on.

Also moves the gitignore line to doc/ so that Doxygen has a directory to
generate into.
@kainino0x
Copy link
Collaborator

Landing without theme so we can get some generated documentation up and set up GitHub Pages!

@kainino0x kainino0x merged commit 0dc92bd into webgpu-native:main Jun 10, 2024
4 checks passed
@kainino0x
Copy link
Collaborator

... and it just worked! https://webgpu-native.github.io/webgpu-headers/

@beaufortfrancois
Copy link
Contributor

That's pretty nice! Thank you @eliemichel!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants