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

In #32764 (moved), 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)

changed milestone to %Tor: unspecified

Trac:
Child Ticket(s): #32818 (moved), #32907 (moved)

added 043-deferred 044-deferred actualpoints::1.3 component::core tor/tor milestone::Tor: unspecified points::2 priority::medium reviewer::nickm reviewer::teor severity::normal status::assigned type::task labels

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 (moved). 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.

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.

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:

• master: https://github.com/torproject/tor/pull/1618

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 (moved)
  • 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?

Trac:
Actualpoints: N/A to 1
Reviewer: N/A to nickm, teor
Status: new to needs_review

Trac:
Status: needs_review to assigned
Owner: N/A to teor

Trac:
Status: assigned to needs_review

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.

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.

Trac:
Actualpoints: 1 to 1.3
Status: needs_review to needs_revision

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.

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.

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

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.

Unparenting to close parent.

Trac:
Parent: #32764 (moved) to N/A

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

Trac:
Keywords: N/A deleted, 043-deferred added

Trac:
Milestone: Tor: 0.4.3.x-final to Tor: 0.4.4.x-final

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

Trac:
Status: needs_revision to assigned
Owner: teor to N/A

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

Trac:
Keywords: N/A deleted, 044-deferred added
Milestone: Tor: 0.4.4.x-final to Tor: unspecified

changed time estimate to 16h

added 10h 24m of time spent

mentioned in issue #32818 (moved)

mentioned in issue #32907 (moved)

moved to tpo/core/tor#32798 (closed)

mentioned in issue tpo/core/tor#32818 (closed)