Create better tooling for canonical tor header includes

When we fix includes, we could also:

• standardise the whitespace
• sort into a consistent order
• remove duplicates

Eventually, we should try to find unused headers, but that seems like a separate ticket. (But we need to do this task first.)

changed milestone to %Tor: unspecified

Trac:
Parent Ticket: #29226 (moved)
Child Ticket(s): #32609 (moved), #32610 (moved), #32613 (moved), #32655 (moved)

added 043-deferred 044-deferred actualpoints::5 component::core tor/tor milestone::Tor: unspecified parent::29226 points::1 priority::medium reviewer::nickm severity::normal status::assigned type::task labels

I have a draft script, it's harder than I thought. I need to move the comments along with the headers/private defines they belong to.

Trac:
Actualpoints: N/A to 0.8

There's still some work to do here, but I should have a PR ready for review soon. I found a lot of interesting bugs and inconsistencies along the way, see the child tickets.

Trac:
Actualpoints: 0.8 to 4

Here's my draft pull request:

• https://github.com/torproject/tor/pull/1578

And here's what the code actually ends up doing:

• better canonical header paths
• delete unused PRIVATE, INTERNAL, and EXPOSE defines
• delete duplicate includes
• standardise whitespace (conforming to make check-spaces, plus extra fixes)
• add command-line arguments to control which files get which fixes

Trac:
Status: assigned to needs_review
Actualpoints: 4 to 5
Reviewer: N/A to nickm

Hm. This looks like a complete rewrite to me. That's okay, but I'm going to have to make a few general comments before I move in to a line-by-line review. I hope that's okay.

1. The changes made by this file look good, and the improved documentation and code structure is nice.

2. I think this is the kind of rewrite where we want to have tests now. Do you think tests are in order here?

3. Do you think that we should standardize on one of PRIVATE, INTERNAL, or EXPOSE? Alternatively, do you think we should document the difference between them?

4. Can/should we reuse python's logging framework rather than rolling our own set of warning/error reporting functions?

5. When using "global", please make sure that you're actually modifying the variable. It isn't necessary to say "global" when you're only reading the variable.

6. This branch uses several different new maps, with new semantics. Would it make sense to turn them into one or more classes? If not, we should at least have an overview listing all of them and what they're for.

7. Does is really make sense to have "current_file" be a global? Is there some more OO approach that would make the code cleaner? (Obviously we shouldn't do that if it makes the code uglier.)

8. Consider running this script through a python style checker, if you haven't done so already; it usually catches a few things when I remember to do that.

9. I don't think that we should remove any normalizations that this script does, but before we add any more normalizations, we should be sure that we aren't replicating work that our chosen code styling tool can already do for us.

Once we've decided what to do with the above, I can start on a line-by-line review.

Trac:
Status: needs_review to needs_revision

Replying to nickm:

Hm. This looks like a complete rewrite to me. That's okay, but I'm going to have to make a few general comments before I move in to a line-by-line review. I hope that's okay.

1. The changes made by this file look good, and the improved documentation and code structure is nice.

2. I think this is the kind of rewrite where we want to have tests now. Do you think tests are in order here?

Yes, and the script can be re-targeted at a test directory using its command-line options.

3. Do you think that we should standardize on one of PRIVATE, INTERNAL, or EXPOSE? Alternatively, do you think we should document the difference between them?

Yes, probably PRIVATE. I can make this change, and then modify the script.

4. Can/should we reuse python's logging framework rather than rolling our own set of warning/error reporting functions?

Probably, if we can work out how to get the current file context into it.

5. When using "global", please make sure that you're actually modifying the variable. It isn't necessary to say "global" when you're only reading the variable.

I think this is tied up with 4) and 7).

6. This branch uses several different new maps, with new semantics. Would it make sense to turn them into one or more classes? If not, we should at least have an overview listing all of them and what they're for.

Probably one or two classes, implemented with an internal map.

7. Does is really make sense to have "current_file" be a global? Is there some more OO approach that would make the code cleaner? (Obviously we shouldn't do that if it makes the code uglier.)

I'll see if python's logging has some kind of context argument.

7. Consider running this script through a python style checker, if you haven't done so already; it usually catches a few things when I remember to do that.

Do we have a recommended python style checker? Should we standardise on one, and start moving our scripts to it?

8. I don't think that we should remove any normalizations that this script does, but before we add any more normalizations, we should be sure that we aren't replicating work that our chosen code styling tool can already do for us.

I don't expect to add any more normalizations, my next step is #32655 (moved), which a styling tool definitely can't do.

Once we've decided what to do with the above, I can start on a line-by-line review.

I think I'd rather revise first, and then have the review.

Let's work out what's remaining here, after our clang-format work has made more progress.

Trac:
Parent: #31851 (moved) to #29226 (moved)

Sponsor 31 is over, this isn't a roadmap task any more.

Trac:
Keywords: network-team-roadmap-november deleted, N/A added

I cherry-picked the obvious code cleanups from this PR to master, there may be some conflicts when we go to revise it.

I opened #32818 (moved) to standardise on PRIVATE.

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

No more sponsor 31. All this tickets remained open after sponsor 31 ended.

Trac:
Sponsor: Sponsor31-can to N/A

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 8h

added 40h of time spent

mentioned in issue #32610 (moved)

mentioned in issue #32613 (moved)

mentioned in issue #32655 (moved)

mentioned in issue #32818 (moved)

mentioned in issue #29226 (moved)

mentioned in issue #32609 (moved)

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

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

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