Opened 3 years ago

Closed 3 years ago

#19028 closed defect (fixed)

Merge the header checks in configure.ac

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: 029-nickm-says-yes, review-group-2
Cc: Actual Points:
Parent ID: Points: 0.2 remaining
Reviewer: nickm Sponsor:

Description

The build configuration contains two sets of header checks where one checks headers that are deemed essential and one checks headers that are not. The only difference being that missing essential headers display a non-fatal warning when they aren't found.

For the following reasons i think it is better to merge the sets and discard the warning.

  • Commit e8cc839e41adc4975a61fee62abe7f6664fd0c0e adds sys/capability.h to the essential header set but Tor runs fine without capabilities being available. IMO it is in the wrong set.
  • Having two sets requires developers to make a choice whether headers are essential or not. Instead developers should write code that handles cases where the headers are unavailable (like the capabilities code).
  • The current warning is truncated because its message is incorrectly quoted. This means the part about sending the orconfig.h file isn't shown and has been ineffective.

Child Tickets

Attachments (2)

0001-Do-not-warn-on-missing-headers.patch (838 bytes) - added by cypherpunks 3 years ago.
0002-Fix-indentation-and-quotation-of-the-headers.patch (3.3 KB) - added by cypherpunks 3 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 3 years ago by nickm

Keywords: 029-proposed added
Milestone: Tor: 0.2.???
Status: newneeds_review

Changed 3 years ago by cypherpunks

comment:2 Changed 3 years ago by cypherpunks

I'm unsure why this ticket was set to needs_review because there were no patches yet.

I've now added two patches which 1) merges the two sets together and removes the warning and 2) fixes the inconsistent indentation and quotation of the headers. The ticket is now really ready to be reviewed.

comment:3 Changed 3 years ago by nickm

Keywords: 029-nickm-says-yes added

comment:4 Changed 3 years ago by nickm

Points: 0.2 remaining

comment:5 Changed 3 years ago by nickm

Keywords: review-group-2 added

Create a review-group-2 from (most of the) tickets in 0.2.8 or 0.2.9 or 029-nickm-says-yes listed as needs_review,

comment:6 Changed 3 years ago by nickm

Keywords: 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

Calling thsese "yes" for 0.2.9 because they are externally submitted patches that won't be too hard to review.

comment:7 Changed 3 years ago by nickm

Resolution: fixed
Reviewer: nickm
Status: needs_reviewclosed

looks correct to me. (I confirmed that the 0002 patch was indentation-only with 'git show -b'.) Merged; thanks!

Note: See TracTickets for help on using tickets.