-
Notifications
You must be signed in to change notification settings - Fork 34
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
ci: Build and store doxygen documentation #692
Conversation
Codecov Report
@@ Coverage Diff @@
## master #692 +/- ##
=======================================
Coverage 99.30% 99.30%
=======================================
Files 72 72
Lines 10300 10300
=======================================
Hits 10228 10228
Misses 72 72
Flags with carried forward coverage won't be shown. Click here to find out more. |
// Fizzy: A fast WebAssembly interpreter | ||
// Copyright 2020 The Fizzy Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
/// @file |
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.
Without this file is considered private by default and documentation for free functions is not generated.
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.
Are you sure it can't just be added in the doxygen config?
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.
Well adding the file explicitly in INPUT
doesn't help (it's included anyway)
The only other way to fix it that I found is to set EXTRACT_ALL = YES
, but it might include more stuff than needed, EVMC has it at NO
.
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.
https://www.doxygen.nl/manual/commands.html#cmdfile
The documentation of global functions, variables, typedefs, and enums will only be included in the output if the file they are in is documented as well or if EXTRACT_ALL is set to YES.
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.
This is done wrongly. We don't want to the "copyright header" to be included in the documentation text.
It rather should be
// Copyright...
/// Fizzy public C API.
/// @file
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.
Changed. I just hate doxygen 😠
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.
clang-doc
maybe if you want to contribute to LLVM.
# Note: If this tag is empty the current directory is searched. | ||
|
||
INPUT = include/fizzy/ \ | ||
lib/fizzy/ \ |
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.
Do we want to include these internal files?
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.
Do we want to use this doxygen generated file for the external documentation only or as a project documentation?
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.
Perhaps we can decide later, and for now merge it like this, to have something working.
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.
I'd go with generating docs for everything as a start. There may be better way of separating public and internal API.
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.
E.g. there is @internal
tag. https://stackoverflow.com/questions/11025971/separate-internal-from-external-documentation-in-doxygen
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.
Good for me.
// Fizzy: A fast WebAssembly interpreter | ||
// Copyright 2020 The Fizzy Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
/// @file |
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.
clang-doc
maybe if you want to contribute to LLVM.
Doxyfile
Outdated
# pixels and the maximum width should not exceed 200 pixels. Doxygen will copy | ||
# the logo to the output directory. | ||
|
||
PROJECT_LOGO = |
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.
Hmm, can this accept svg?
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.
I tried locally, and it didn't work for me.
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.
Actually, it does work, when I resize it to 200x200.
Doxyfile
Outdated
# types are typedef'ed and only the typedef is referenced, never the tag name. | ||
# The default value is: NO. | ||
|
||
TYPEDEF_HIDES_STRUCT = NO |
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.
Can y'all check the description above if it applies to us? I don't think we do too much typedefing.
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.
This should be YES
.
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.
Ok, but would maybe we could do a pass on the doxygen config. I only did a brief scroll. We can do it after this is merged too.
I split Doxyfile into two commits:
|
cc283c9
to
eae8072
Compare
Part of #531.