Opened 10 months ago

Last modified 4 months ago

#32798 assigned task

Compile every header by itself as part of "make check"

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 043-deferred, 044-deferred
Cc: Actual Points: 1.3
Parent ID: Points: 2
Reviewer: nickm, teor Sponsor:

Description

In #32764, we need to make sure that some headers include their dependencies (or remove those dependencies), so that clang-format can re-order headers.

We should compile every header by itself, to check that it lists all its dependencies. Some headers contain conditional code, so we also need to compile with and without:

  • HAVE_MODULE_* (already in CI, as long as we use the defined from configure)
  • *{INTERNAL,PRIVATE,EXPOSE}* (not sure how to do this in CI, I guess we could script it using grep)

Child Tickets

TicketStatusOwnerSummaryComponent
#32818closedteorStandardise EXPOSE and INTERNAL macros to PRIVATECore Tor/Tor
#32907acceptednickmRemove or_options_t dependencies from module config headersCore Tor/Tor

Change History (16)

comment:1 Changed 10 months ago by nickm

This is a really good idea! It might be biting off more than I can chew this week, however: when I did a quick-and-dirty test here, I found that only 57% of our headers currently build on their own, even after the fixes in #32764. That leaves 177 headers to fix, which is a lot to tackle.

I think that maybe we could do this without special handling for HAVE_MODULE_*; since our CI runs with the full matrix of disable-module-foo, then we should learn about any headers where we've broken with a module turned off.

I'm going to hack on my quick-and-dirty test script a bit more, then upload it so we can iterate and improve it.

comment:2 Changed 10 months ago by nickm

You can have a look at my script in test_headers, with PR at https://github.com/torproject/tor/pull/1616 . It probably needs significant work before it could be used in production, and (as noted) we'd need to clean up a lot of headers before we could make it a part of make check.

comment:3 Changed 10 months ago by teor

Actual Points: 1
Reviewer: nickm, teor
Status: newneeds_review

I think I have this script in a reviewable, production-ready state. It checks the header failures against an exceptions file, and only fails when a header unexpectedly fails.

See my PR, based on yours:

Here are the key changes:

  • Add an exceptions file, so we can make changes incrementally
  • Support out-of-tree builds
  • Look for more defines, more reliably
  • Check more headers
  • Other minor fixes
  • Create prefix.h for allowed prefix includes

Maybe you can help with this change:

  • use CFLAGS etc. from the Makefile

After we've finished reviews and revisions, we should:

  • rebase on #32764
    • and run "make update-build-headers-exceptions"

There's still plenty we can do to improve it:

  • prefix.h:
    • stop including torint.h and testsupport.h
  • integrate using an include.am file, rather than generating a Makefile?
    • make Makefile dependencies actually work, rather than re-compiling every time

I used 1 point on this task today, can you add what you used yesterday?

comment:4 Changed 10 months ago by teor

Owner: set to teor
Status: needs_reviewassigned

comment:5 Changed 10 months ago by teor

Status: assignedneeds_review

comment:6 Changed 10 months ago by teor

Appveyor CI is failing, so I added the following files to the exceptions file:

+address_h.c
+ntmain_h.c
+process_win32_h.c
+timers_h.c
+tor_api_h.c

https://ci.appveyor.com/project/torproject/tor/builds/29647378/job/6ub2w7jvn8xr4u1l#L15589

I also made some mistakes when fixing backtrace.h, so I force-pushed an update.

comment:7 Changed 10 months ago by nickm

Actual Points: 11.3
Status: needs_reviewneeds_revision

Looks good!

I've left a couple of short questions on the PR. Also, CI is now failing because we don't have a declaration for ucontext_t in backtrace.c.

I'll try to think about the CFLAGS issue; at first glance I think it needs to be an environment variable. Same with CC, etc. I don't know if I'll get to actual implementation on those before I go on break, though.

Also we should stick our license statement at the head of the file.

comment:8 in reply to:  7 Changed 10 months ago by teor

Replying to nickm:

Looks good!

I've left a couple of short questions on the PR. Also, CI is now failing because we don't have a declaration for ucontext_t in backtrace.c.

Hmm, so the ucontext header doesn't define struct ucontext_t? I don't know how to work around that?
Should we be doing a forward type declaration instead?

I'll try to think about the CFLAGS issue; at first glance I think it needs to be an environment variable. Same with CC, etc. I don't know if I'll get to actual implementation on those before I go on break, though.

I wonder if we should actually be running this test via automake? Then we'd get a lot of these things for free.

Also we should stick our license statement at the head of the file.

Ok.

comment:9 Changed 10 months ago by teor

Some headers fail on Travis, but not locally for me:

11 unexpected header compilation failures:
+desc_store_st_h.c
+dir_connection_st_h.c
+dns_structs_h.c
+extend_info_st_h.c
+hs_intropoint_h.c
+node_select_h.c
+node_st_h.c
+rendcommon_h.c
+rend_service_descriptor_st_h.c
+tor_version_st_h.c
+vote_routerstatus_st_h.c

https://travis-ci.org/torproject/tor/jobs/627486698#L14985

Also, macOS doesn't like the way I defined ucontext_t.

comment:10 Changed 10 months ago by teor

Oh. Sort works differently on my machine and on Travis.

comment:11 Changed 10 months ago by teor

Ok, I pushed some fixes dealing with most of these issues. Still need to check CI across all platforms :-)

Here's the remaining work:

I'll try to think about the CFLAGS issue; at first glance I think it needs to be an environment variable. Same with CC, etc. I don't know if I'll get to actual implementation on those before I go on break, though.

I wonder if we should actually be running this test via automake? Then we'd get a lot of these things for free.

comment:12 Changed 10 months ago by nickm

Parent ID: #32764

Unparenting to close parent.

comment:13 Changed 9 months ago by nickm

Keywords: 043-deferred added

All 0.4.3.x tickets without 043-must, 043-should, or 043-can are about to be deferred.

comment:14 Changed 9 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

comment:15 Changed 5 months ago by teor

Owner: teor deleted
Status: needs_revisionassigned

It's unlikely that I'll ever finish this tooling.

comment:16 Changed 4 months ago by nickm

Keywords: 044-deferred added
Milestone: Tor: 0.4.4.x-finalTor: unspecified

Bulk-remove tickets from 0.4.4. Add the 044-deferred label to them.

Note: See TracTickets for help on using tickets.