Opened 3 months ago

Closed 8 weeks ago

#31176 closed enhancement (fixed)

Teach practracker about .may_include files

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: practracker, tech-debt, refactoring, easy, 041-deferred-20190530, network-team-roadmap-august
Cc: nickm Actual Points: 1
Parent ID: #29746 Points: 1.5
Reviewer: teor, asn Sponsor: Sponsor31-can

Description

We would like to introduce a second category of .may_include rules: those that should only apply on an advisory basis. We would treat violations of these rules as a best practices violation rather than an error. It would allow us to start ratcheting down the number of layering violations.

Child Tickets

Change History (11)

comment:1 Changed 3 months ago by gaba

Cc: nickm added
Keywords: network-team-roadmap-august added

comment:2 Changed 2 months ago by nickm

Actual Points: 1

See branch ticket31176 with PR in https://github.com/torproject/tor/pull/1206 .

This branch comes in two main parts. In the first part, I refactor checkIncludes.py until it can be integrated into practracker. In the second, I move checkIncludes.py to practracker/includes.py, and teach it that some violations are "advisory only" -- to be counted as best-practices violations, but not to cause build failures.

I've added .may_include files to src/core/* for this, since that is where we currently most want to push the modularity boundary.

comment:3 Changed 2 months ago by nickm

Status: assignedneeds_review

comment:4 Changed 2 months ago by asn

Reviewer: teor

comment:5 Changed 2 months ago by teor

Reviewer: teorteor, asn

Looks fine to me, but I think asn knows more about practracker, and can give it a better review.

comment:6 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

Design looks reasonable.

We should update the file-level comment of practracker.py to mention that we are now checking for include dependecies too. Same goes for the header of exceptions.txt .

And maybe it's time to make a small README file to specify all the metrics that practracker is currently looking for, before they become too many?

Also, I wonder if we should be requiring unittests for practracker features.

comment:7 Changed 8 weeks ago by nickm

I agree with all of your suggestions here, but I am wondering if I can do them in a new branch after this is merged.

The issue is that the current PR (for #31176) is based on an older version of practracker that did not support header files (#31175) and which did not have integration test support (#31263).

We should document header files as well as includes, and we should use the test support to test all of this.

I've made a new ticket (#31476) for the documentation, and another for the new tests (#31477). Okay to merge this?

comment:8 Changed 8 weeks ago by nickm

Status: needs_revisionneeds_review

comment:9 Changed 8 weeks ago by dgoulet

Status: needs_reviewmerge_ready

Since asn is away for couple days, Nick asked me to step in here. I think nickm's request is reasonable to I'll merge the PR in a jiffy so he can move on.

Last edited 8 weeks ago by dgoulet (previous) (diff)

comment:10 Changed 8 weeks ago by nickm

There are merge conflicts so I made a branch called ticket31176_merged with PR at https://github.com/torproject/tor/pull/1245

comment:11 Changed 8 weeks ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged! Thanks!

Note: See TracTickets for help on using tickets.