Check .may_include order and tor subsystem init order are compatible

In #31615 (moved), we discovered that our module init order doesn't match their dependency order.

Let's use practracker and tor to make sure that doesn't happen again. We'll probably need a new make check target, because we'll need a compiled tor and practracker output. See #31615 (moved) for details.

Gaba, this is Sponsor 31-can, because it helps us catch refactoring bugs when we create new modules.

changed milestone to %Tor: 0.4.4.x-final

Trac:
Child Ticket(s): #33315 (moved), #33316 (moved)

added actualpoints::1 component::core tor/tor diagnostics milestone::Tor: 0.4.4.x-final owner::nickm points::2 practracker priority::medium resolution::fixed reviewer::teor severity::normal status::closed type::enhancement labels

We might also want to check against the order in subsystem_list.c.

Replying to teor:

We might also want to check against the order in subsystem_list.c.

Yes, but have a look at check_and_setup() in subsysmgr.c -- that causes tor to exit with an assertion failure on startup if the subsystems are not sorted by priority.

Replying to nickm:

Replying to teor:

We might also want to check against the order in subsystem_list.c.

Yes, but have a look at check_and_setup() in subsysmgr.c -- that causes tor to exit with an assertion failure on startup if the subsystems are not sorted by priority.

So we don't need to check the order in subsystem_list.c, because tor does it for us. That's good.

Mark a number of current 0.4.2.x "defects" as "enhancements."

Trac:
Type: defect to enhancement

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

Distributing 0.4.2 tickets between network team members.

Trac:
Owner: N/A to nickm
Status: new to assigned

I have started this, but I believe it is bigger than we should do in 0.4.2, and we should defer part of it till 0.4.3. Here is why:

• Our subsystems do not currently to our directory structures. Notably, these subsystems are defined in with names that do not match their locations: btrack, network, ocirc_event, oconn_event, relay, threads, tortls, winprocess.

• Our subsystem order does not currently match the topology very well at all: there are four discontinuities on the ordering. I think they are caused by: btrack, compress, winprocess, threads. We should re-order these, but doing so will require us to re-organize some of our code. That seems like a stability risk.

I have checked in a work-in-progress version of the code to do these checks as ticket31634. Do you think it's reasonable to defer to 0.4.3 based on the above reasoning? If not, please re-assign to me for 0.4.2, but this just got bigger than we anticipated.

Trac:
Keywords: 042-should deleted, 042-should? added
Status: assigned to needs_review
Reviewer: N/A to teor

(Right now I am not asking for review on the code per se, since it is not ready. I'm asking for review on the decision to defer this to 0.4.3 when we willing to take more stability risks.)

Yes, I think we should make big changes like this in 0.4.3.

And in 0.4.2, we should avoid:

• changes to .may_include
• changes to the module init order

Trac:
Status: needs_review to needs_revision
Keywords: 042-should? deleted, 043-should added
Milestone: Tor: 0.4.2.x-final to Tor: 0.4.3.x-final

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

Trac:
Sponsor: Sponsor31-can to N/A

I think this belongs in 'unspecified' for now. The changes we need here are significant refactoring, and don't belong in a cleanup phase.

Trac:
Keywords: 043-should deleted, N/A added
Milestone: Tor: 0.4.3.x-final to Tor: unspecified

I'm making child tickets for 044 for the parts that we can try without too much effort, though.

I have a working script to do this that actually passes now, in ticket31634. There is a PR in https://github.com/torproject/tor/pull/1741, based the code in #33316 (moved) -- we should review and merge #33316 (moved) first.

Trac:
Actualpoints: N/A to 0.5
Milestone: Tor: unspecified to Tor: 0.4.4.x-final
Status: needs_revision to needs_review

Sorry it's taken me a while to review this, I've been busy with some urgent things the past few weeks.

The code looks good, I added comments about a few typos and tweaks.

There are a few conflicts between this PR and master.

I think I ran tor --dbg-dump-subsystem-list and practracker/includes.py --check-subsystem-order the way you expect, and got the expected results. But I'm not sure.

Can we add the relevant commands to make check? Then I can run it locally, and in CI, and confirm we got the expected results.

Trac:
Status: needs_review to needs_revision

Okay, I've made an updated branch incorporating your suggestions, completely removing a script that apparently isn't what I had working, and rebasing on master to fix merge issues.

It's ticket31634_v2 with PR at https://github.com/torproject/tor/pull/1838

Trac:
Status: needs_revision to needs_review

Travis distcheck CI failed with:

Traceback (most recent call last):
  File "/home/travis/build/torproject/tor/tor-0.4.4.0-alpha-dev/_build/sub/../../scripts/maint/practracker/includes.py", line 348, in <module>
    main(sys.argv)
  File "/home/travis/build/torproject/tor/tor-0.4.4.0-alpha-dev/_build/sub/../../scripts/maint/practracker/includes.py", line 345, in main
    check_subsystem_order=args.check_subsystem_order)
  File "/home/travis/build/torproject/tor/tor-0.4.4.0-alpha-dev/_build/sub/../../scripts/maint/practracker/includes.py", line 309, in run_check_includes
    if check_subsys_file(check_subsystem_order, uses_dirs):
  File "/home/travis/build/torproject/tor/tor-0.4.4.0-alpha-dev/_build/sub/../../scripts/maint/practracker/includes.py", line 275, in check_subsys_file
    if fname in uses_closure[prev]:
KeyError: u'lib/llharden'
FAIL scripts/maint/run_check_subsystem_order.sh (exit status: 1)

Appveyor CI failed with a similar error:

Traceback (most recent call last):
  File "C:/projects/tor/x86_64-w64-mingw32/../scripts/maint/practracker/includes.py", line 348, in <module>
    main(sys.argv)
  File "C:/projects/tor/x86_64-w64-mingw32/../scripts/maint/practracker/includes.py", line 341, in main
    run_check_includes(topdir=args.topdir,
  File "C:/projects/tor/x86_64-w64-mingw32/../scripts/maint/practracker/includes.py", line 309, in run_check_includes
    if check_subsys_file(check_subsystem_order, uses_dirs):
  File "C:/projects/tor/x86_64-w64-mingw32/../scripts/maint/practracker/includes.py", line 275, in check_subsys_file
    if fname in uses_closure[prev]:
KeyError: 'lib/llharden'
FAIL scripts/maint/run_check_subsystem_order.sh (exit status: 1)

Otherwise the changes look good.

Trac:
Status: needs_review to needs_revision

Wow, there were a few issues there. I've added on commit to fix out-of-tree builds from git, and another to fix distcheck builds.

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