-
Notifications
You must be signed in to change notification settings - Fork 173
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
RFE: Add Doxygen Github Pages #366
base: main
Are you sure you want to change the base?
Conversation
This could close issue #195 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM(nb) except for a nit about .gitignore.
c84f301
to
ec5f257
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @drakenclimber, this is a really nice addition! I made a new suggestions, but nothing serious.
* @defgroup APIs | ||
* @{ | ||
*/ | ||
|
||
#define API __attribute__((visibility("default"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this explicitly pull in the API
macro? If so I think we can leave that out, the compiler/linker visibility attribute isn't something users/devs need to worry about, it's simply there to limit what symbols are visible in the shared library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope.
It seemed like a logical grouping to combine all of the API methods into a Doxygen group. Once I had them in a group, it provided an easy way to link to them in Doxygen. Definitely open to other suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, my mistake.
Somewhat related, is there a way to hit the API
macro from the Doxygen docs? It isn't really something a user/dev would need to worry about and I fear it could be misleading to some.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API
macro is definitely accessible from Doxygen. A user can view it either by going to Files -> Globals -> Macros or by searching for API
directly.
https://drakenclimber.github.io/libseccomp/doxygen/wip/doxygen/globals_defs_a.html#index_a
Thoughts?
Add a Doxygen Github Action and the requisite Doxygen config file to generate Doxygen documentation. Documentation is only generated for the "main" branch and is available here: https://seccomp.github.io/libseccomp/ Signed-off-by: Tom Hromatka <tom.hromatka@oracle.com>
ec5f257
to
b242d41
Compare
Signed-off-by: Tom Hromatka <tom.hromatka@oracle.com>
Add a Doxygen introduction to seccomp.h.in and add an API section to api.c. Signed-off-by: Tom Hromatka <tom.hromatka@oracle.com>
b242d41
to
f2548f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really just that one question regarding hiding the API
macro, but I'm good with this either way. I'll refrain from merging this due to that question, but if there is no good answer go ahead and merge it @drakenclimber.
Acked-by: Paul Moore <paul@paul-moore.com>
* @defgroup APIs | ||
* @{ | ||
*/ | ||
|
||
#define API __attribute__((visibility("default"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, my mistake.
Somewhat related, is there a way to hit the API
macro from the Doxygen docs? It isn't really something a user/dev would need to worry about and I fear it could be misleading to some.
I would really prefer to resolve the question over the |
Fair enough :) FWIW, I found these suggestions while quickly googling the issue: |
In an effort to get v2.6.0 out sooner than later, I'm going to suggest we push this out to v2.7.0; if you have any concerns or objections please drop a comment. |
This patchset automatically generates Doxygen documentation and writes it to a github branch. Github supports a feature called pages where a branch is displayed as HTML, and I'm using that to then render the Doxygen output.
Here's what it looks like on my personal libseccomp repo:
https://drakenclimber.github.io/libseccomp/index.html
A couple more comments:
main
branch. If we wanted to generate Doxygen for the release branches as well, we could publish each branch to a subfolder. Then we could make a simple index pageseccomp.h.in
so that the start page isn't empty. Definitely room for improvement here