Opened 3 months ago

Closed 2 months ago

Last modified 2 months ago

#32385 closed defect (implemented)

doxygen: respect --enable-fatal-warnings

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: doxygen
Cc: Actual Points:
Parent ID: #29214 Points: 0
Reviewer: catalyst Sponsor: Sponsor31-can

Description

Doxygen has an option to fail with an error if any warning message occurs. We can enable it, if we disable missing documentation warnings. We should have another option to turn missing documentation warnings back on.

Child Tickets

Change History (10)

comment:1 Changed 3 months ago by nickm

Status: assignedneeds_review

See branch ticket32385 with PR at https://github.com/torproject/tor/pull/1502

comment:2 Changed 2 months ago by dgoulet

Reviewer: catalyst

comment:3 Changed 2 months ago by catalyst

Status: needs_reviewneeds_revision

Thanks! This mostly looks good. I left a minor suggestion on the pull request.

Also, I noticed the following Doxygen error when manually running make doxygen:

/Users/tlyu/src/tor/src/core/or/core_or.dox:63: error: Reached end of file while still inside a (nested) comment. Nesting level 1 (probable line reference: 1) (warning treated as error, aborting now)

which looks like it was introduced by e1cdca2e4f58c108539fe4c36205b16caca8d44f. I'm not sure whether we should fix it as part of this ticket or open a new one. I think it's not the only warning, though, because at least one more pops up if I fix core_or.dox.

comment:4 Changed 2 months ago by nickm

Also, I noticed the following Doxygen error when manually running make doxygen:

This is one I fixed separately in 2ab5b7520ec6d3a45923c9e5a61c3c6ddfb22c43

comment:5 Changed 2 months ago by nickm

(I'll make the other change now.)

comment:6 Changed 2 months ago by nickm

Status: needs_revisionneeds_review

I've made and pushed the other change. Thanks for the review!

comment:7 in reply to:  6 ; Changed 2 months ago by nickm

Replying to nickm:

I've made and pushed the other change. Thanks for the review!

(which is to say: I've done a fix for the documentation issue and pushed it to the branch here pending your approval.)

comment:8 in reply to:  7 Changed 2 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to nickm:

Replying to nickm:

I've made and pushed the other change. Thanks for the review!

(which is to say: I've done a fix for the documentation issue and pushed it to the branch here pending your approval.)

Thanks! I commented on the pull request about a minor wording issue/typo. Feel free to merge after fixing.

Note: also, as far as I can tell, none of our CI runs make doxygen (yet?). So we might want to start doing that in CI when we have everything fully documented from Doxygen's perspective.

comment:9 Changed 2 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Thank you for the reviews! I've fixed the typo and merged, after fixing a couple of doxygen errors that had slipped in since the last time I checked.

I've opened #32455 to add a doxygen CI item. I think we can do that even before we have full documentation: with this branch merged, --enable-fatal-warnings will turn off missing documentation warnings, since there are thousands of them right now.

comment:10 Changed 2 months ago by nickm

Parent ID: #29214
Note: See TracTickets for help on using tickets.