Skip to content

[CI] Restore the per-function Doxygen check - #688

Open
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check
Open

[CI] Restore the per-function Doxygen check#688
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check

Conversation

@myungjoo

@myungjoo myungjoo commented Sep 3, 2026

Copy link
Copy Markdown
Member

Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).

What was wrong

.github/workflows/static.check.scripts/doxygen-tag.sh here is byte-identical to the nnstreamer copy: local function_check_flag="f+p" at script top level, which bash rejects, and no set -e, so ctags -x --c-kinds= ran with an empty kind list and the per-function @brief check has passed vacuously since the Actions import. $report_path was never set (an "ambiguous redirect" per file on stderr) and $brief was not reset between files.

What this PR does

Takes the corrected checker and its self-test from nnstreamer/nnstreamer#4914 verbatim, and adds the self-test step to static.check.yml with the same merge-ref guard:

  • definitions are checked everywhere, prototypes only in headers;
  • the trailing /**< ... */ form is recognised (declaration line, a line opening with /**< right after it, or after an inline body);
  • block comments are tracked so a * at the start of a wrapped parameter line is not taken for a comment continuation;
  • report_path defaults to /dev/null, brief resets per file;
  • test_doxygen_tag.sh generates 18 fixtures into a temp dir; all 18 fail against the checker on main and pass here.

Backlog

None. Under the corrected scope the tree is already clean (48 C/C++ files, zero findings with Exuberant Ctags 5.9, the CI version), so the check turns on without touching any source file.

🤖 Generated with Claude Code

@myungjoo

myungjoo commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

[Automated review by a separate Claude agent, relayed to this PR]

Scope: read nnstreamer/nnstreamer#4908 and the full review history of nnstreamer/nnstreamer#4914 (gh pr view 4914 --repo nnstreamer/nnstreamer --comments, 4 review rounds), diffed this PR (upstream/main...HEAD, 3 files, +325/-8) and byte-compared doxygen-tag.sh/test_doxygen_tag.sh against nnstreamer's final approved commit 4490a668 (identical modulo CRLF), read static.check.yml's new step and the rest of the job, and independently re-verified the PR's "zero backlog" claim against this repo's own 48 C/C++ files (ctags is not available in my sandbox, so I wrote and sanity-checked a Python heuristic against the checker's actual state-machine rules, cross-checked with gh pr checks / gh run view --log). Also checked nnstreamer/api's branch protection via the GitHub API.

Does it solve the stated problem

Yes. doxygen-tag.sh here was byte-identical to nnstreamer's pre-#4914 copy, carrying the same top-level local function_check_flag="f+p" (invalid outside a function, silently emptying ctags's kind list with no set -e to catch it), the same unset $report_path, and the same missing per-file reset of $brief. This PR takes nnstreamer's already-fixed, 4-round-reviewed version verbatim — I confirmed diff -u --strip-trailing-cr between this PR's two scripts and nnstreamer PR #4914's head (4490a668) is empty. That version's design (definitions checked everywhere, prototypes only in .h/.hh/.hpp; trailing /**< recognition on same line, next line, or after an inline body; real block-comment tracking via in_comment instead of a bare-* heuristic) was independently traced and stress-tested across 4 review rounds on the origin PR, including two real counterexamples found and fixed in-tree (gsttensor_converter.c's wrapped-pointer prototype, Looper::post getting credited for a sibling's trailing /**<). Nothing in this port reopens either of those.

Backlog = 0 claim — verified independently

I could not run ctags locally (not installed, no sudo in my sandbox), so instead of trusting the PR body I wrote a Python approximation of the checker's own rules (K&R return-type-on-own-line handling, @brief/@copydoc block detection, trailing /**< on the same/next line) and sanity-checked it against a synthetic documented/undocumented pair before trusting its output on the real tree.

  • Whole-tree scan (48 files): 1141 candidate function definition/declaration sites, 0 flagged as undocumented.
  • Header-prototype-only scan (f+p scope, 9 header files): 216 candidate prototypes, 0 flagged.
  • Every one of the 438 GTest TEST (suite, case) macros across tests/capi/*.cc (these get tagged as ordinary functions by ctags' generic C parser) is immediately preceded by a comment block containing @brief, verified with a separate direct script, not just the heuristic above.
  • Every named-tag struct/class in the tree (_ml_pipeline, _ml_pipeline_element, _ml_pipeline_common_elem, _type_int, _type_string, _type_element, _conf_detail, _camera_conf) has its own leading @brief.
  • This matches the PR's own CI: the self-test step ran with the real exuberant-ctags the "Preparing step 2" installs, and all 18 PASS: lines plus test_doxygen_tag.sh: all checks passed. appear in the live log of run 33715005844.

I did not find a counterexample. The claim holds as far as I can verify it.

Self-test / CI wiring

test_doxygen_tag.sh (18 fixtures) is a solid regression suite: it pins the negative control (undocumented definition must fail — the exact property that was silently broken for years), both scope halves (f vs f+p), all three /**< placements plus the two adjacency traps found in the origin review (trailing_belongs_to_next.hh, pointer_line.h/wrapped_pointer.h), the per-file state reset in both directions, and asserts zero stderr output (which is how the original local bug and the unset report_path actually manifested). It generates fixtures into mktemp -d rather than committing them, so an intentionally-undocumented fixture never lands in a future PR's $changed_file_list.

Positive note on the port, not an issue: in nnstreamer's static.check.yml this same self-test step's comment says "needs the same merge-ref guard as the self-test steps below" — there it's one of five sibling self-test steps sharing the pattern. nnstreamer/api's static.check.yml has no other self-test step, and the author rewrote the comment here to be self-contained (explaining why the merge-ref-vs-checkout-ref mismatch can happen, rather than pointing at nonexistent siblings). I checked — there is genuinely no other diff-filter=D/self-test step anywhere in this repo's .github/workflows/, so the rewritten comment is the correct call, not an oversight.

should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)

nnstreamer/api's branch protection on main does not include "Static checks" as a required status check (gh api repos/nnstreamer/api/branches/main/protectionrequired_status_checks.contexts: ["DCO"] only). This predates this PR — the "Static checks" job already existed with a dozen other checkers before this change — and it isn't something this PR's diff can fix (the repo has no .github/settings.yml or other repo-as-code branch-protection file; it's a GitHub UI/API-only setting). But it directly bears on the review question "can CI failing actually block a future regression from merging": today, a red Static checks job (including a future regression this exact self-test would catch) does not by itself block the merge button, only a red/missing DCO does. Recommend the maintainer add Static checks to the required status checks for main in a follow-up (not part of this PR).

Informational / no PR action needed

  • Spell Check with Typos is currently failing on this PR's CI, but on a pre-existing typo ("gievn" → "given") in c/src/ml-api-service-offloading.c:306, introduced in commit 751f51b6 (2023-06-30), a file this PR does not touch. Confirmed via git blame against upstream/main. Unrelated to this change.
  • The known, already-accepted-upstream heuristic limits of has_trailing_doc/in_comment (can't distinguish a real /* from one inside a string literal or a x / *p expression without a space) carry over unchanged from the already-reviewed nnstreamer version; no live instance of either pattern exists in this tree today (checked by grep), same conclusion the origin reviews reached for nnstreamer's tree.
  • has_trailing_doc accepts any /**< without requiring @brief/@copydoc inside it, which is a slightly lower bar than the leading-comment form — again unchanged from the reviewed-and-approved upstream version, and consistent with how this repo's own headers already use /**< on enum/struct members.

Size / scope

Proportionate and minimal: 3 files, all under .github/workflows/, no source file touched (the tree already being clean under the corrected scope means there's no comment-only backlog-clearing commit needed here, unlike the origin PR's 34 files). No unrelated module is touched. Single commit, DCO-signed, subject line follows [Component] Summary convention, body explains what/why and credits the upstream fix.

Architecture / API docs

None needed. CI-script-only change; this repo has no CLAUDE.md/AGENTS.md/Documentation/ referencing the doxygen-check policy that this PR's scope choices would need to update.

CI status at review time

Static checks: pass (run 33715005844), self-test 18/18 PASS, doxygen-tag check itself ran clean over the 2 changed files. DCO: pass. Spell Check with Typos: fail, pre-existing/unrelated (see above). build (armeabi-v7a): pass. Remaining build (*)/Tizen GBS jobs were still pending/queued at review time; this PR touches no source or build file, so I have no reason to expect them to be affected, but I did not wait for them to finish.

Verdict

Mergeable on content. The restored checker and its self-test are a faithful, verified port of the version that survived 4 rounds of review upstream, the "zero backlog" claim checks out under independent verification (including the highest-volume risk category, the 438 GTest macros), and the PR is minimal and correctly scoped. The one should-fix (branch protection not actually requiring "Static checks") is real and worth a maintainer follow-up, but it's a repository setting outside this PR's diff, not a reason to hold this PR.

@myungjoo
myungjoo marked this pull request as ready for review September 3, 2026 05:06
jaeyun-jung pushed a commit that referenced this pull request Sep 3, 2026
"gievn" for "given" in the brief of the offloading URI helper. The
typos job flags it on every PR that runs the spell check, including
ones that do not touch this file (#688).

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@myungjoo-bot myungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR fixes the top-level local that emptied the ctags kind list (making the per-function Doxygen check pass vacuously), defaults report_path, resets brief per file, adds /**< trailing-comment and block-comment tracking, and adds an 18-fixture self-test wired into static.check.yml (trigger pull_request, no secrets). Verified locally under Exuberant Ctags 5.9 (the flavor CI installs): test_doxygen_tag.sh 18/18 PASS, exit 0 (matching the CI run). doxygen-tag.sh was also run on 27 additional probes (undocumented definition, gtest TEST(), #if 0, static inline, multi-line, K&R, C++ out-of-line methods with a lambda, templates, macro-declared functions, prototypes in .h vs .c, extern "C", brace-in-string) with the expected results, and on all 48 C/C++ files at the PR head: exit 0, 0 errors, 1166 function/prototype tags inspected. git merge-tree against current main is clean. The failing "Spell Check with Typos" job is unrelated (no source files touched). Approving; the items below are non-blocking and mostly pre-existing.

  1. [Low] /**< inside a function body counts as documentation.github/workflows/static.check.scripts/doxygen-tag.sh:54 (has_trailing_doc): the /**< test runs before the brace-depth update. Reproduced: int foo (int x) { int y; /**< local */ return x + y; } with no leading comment passes. Fix: update depth first and accept only when depth <= 0.
  2. [Low] One-line /** @brief foo */ int foo (int x); is flagged:136 vs :150: the function check runs before the current line's @brief is registered (pre-existing ordering; no instance in the tree). Also at :137 the unquoted echo $line | cut ... glob-expands /**, so the message printed /bin function needs @brief tag. Fix: test @brief|@copydoc before line 136, and use "${line%% *}" in the message.
  3. [Low] Preprocessor conditional between doc comment and definition resets the pending brief:155 (pre-existing; no instance in the tree). Exclude #if/#ifdef/#ifndef/#else/#elif/#endif from the reset.
  4. [Low] Non-ASCII files are skipped entirely:70 file $file | grep "ASCII text": c/include/ml-api-common.h (contains ®) is skipped by every rule. It passes with 0 errors once ASCII-ified, so widening the gate has no backlog. Fix: grep -E "ASCII text|UTF-8 (Unicode )?text".
  5. [Low] Self-test gapstest_doxygen_tag.sh:83-249: no .cc fixture (gtest TEST() / out-of-line C::m) and no #if 0 fixture, although both behave correctly under Exuberant Ctags; two fixtures would pin them. :33 checks command -v ctags but not the flavor; a ctags --version | grep -q Exuberant guard with a clear message would help local runs.
  6. [Info] Workflow wiring (static.check.yml:86-102) is correct: runs on pull_request, handles the merge-ref/old-base mismatch, catches deletions via --diff-filter=D --no-renames, and fails the step on non-zero exit.

No back-door or suspicious behavior found: trigger is pull_request (not pull_request_target), no secrets or write token, no eval, and the self-test writes only under mktemp -d.

doxygen-tag.sh declared `local function_check_flag="f+p"` at script
top level. Bash rejects `local` outside a function, the script has no
`set -e`, so ctags ran with an empty kind list and the per-function
@brief check never fired. The script is byte-identical to the copy in
nnstreamer/nnstreamer, where the same defect was found and fixed in
nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908); this
takes that checker and its self-test verbatim.

- Drop the `local`, default report_path to /dev/null (it was unset
  and produced an "ambiguous redirect" per file), and reset the brief
  state per file.
- Check function definitions everywhere but prototypes only in
  headers; a static forward declaration in a .c is documented at its
  definition.
- Recognise the trailing `/**< ... */` form, and track block comments
  so a '*' at the start of a wrapped parameter line is not taken for
  a comment continuation.
- Add test_doxygen_tag.sh (18 generated fixtures) and a workflow step
  for it with the same merge-ref guard the nnstreamer copy uses.

The tree is already clean under the corrected scope: 48 C files,
zero findings, so the check turns on with nothing to clear.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@myungjoo
myungjoo force-pushed the ci/doxygen-function-check branch from 0800caa to 1222a6b Compare September 4, 2026 06:24

@myungjoo-bot myungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated re-review of head 1222a6ba79 after the rebase onto main (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR content is unchanged from the previously approved head; only the base moved to main 8952123. git merge-tree is clean, GitHub reports MERGEABLE (blocked only on required review), and all 10 checks pass on the current head, including the previously red "Spell Check with Typos" (fixed on main by #689) and Static checks whose log shows the new self-test 18/18 PASS under CI's exuberant-ctags. Real ctags was not available locally this time, so the scripts were exercised against a ctags -x emulator: self-test 18/18 in both Git Bash and WSL, a 21-fixture probe suite behaves as designed, and a whole-tree run over all 48 C/C++ files (1110 function/prototype tags) exits 0 with zero errors. The five Low findings from the previous review were each re-verified and still hold; a 4-line patch addressing (1)-(4) below was also verified to keep the self-test, probes, and whole tree green (including c/include/ml-api-common.h, which the widened text gate would newly check). Approval stands; the items below remain non-blocking follow-ups.

  1. [Low] /**< inside a function body is accepted as documentationdoxygen-tag.sh:54 (has_trailing_doc): the test runs before the brace-depth update at :55-57. Fix: [[ $depth -le 0 && $l == *"/**<"* ]] && return 0 after the depth update.
  2. [Low] One-line /** @brief foo */ int foo (int x); is flagged; error message glob-expands:136-137 vs :150. Fix: add && ! $line =~ @(brief|copydoc) to the condition at :136 (do not move the @brief block above the check, or the ; reset would flag every prototype), and print the name via parameter expansion instead of the unquoted echo $line | cut.
  3. [Low] Preprocessor conditional between doc comment and definition resets the pending brief:155. Fix: exclude ^[[:space:]]*#[[:space:]]*(if|ifdef|ifndef|elif|else|endif) from the reset (#define should still reset).
  4. [Low] Non-ASCII sources are skipped entirely:70 grep "ASCII text"; c/include/ml-api-common.h is UTF-8 text and is never checked. Fix: grep -E "ASCII text|UTF-8"; the tree still passes with that gate.
  5. [Low] Self-test gapstest_doxygen_tag.sh:83-249: no .cc / gtest TEST() / out-of-line C::m fixture and no #if 0 fixture; :33 checks only command -v ctags without a flavor guard. Suggest adding fixtures for items 1-4 when they are fixed.
  6. [Low] Block-comment tracker can be stuck open by /* inside a // comment or string literal:163. Only a false negative is possible and no instance exists in the tree; optional: skip lines starting with // before the /* test.

No back-door or suspicious behavior found: trigger is pull_request (not pull_request_target), no secrets or write token, no eval; the only ${{ }} interpolations are head.sha and commits, matching the pre-existing pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants