Opened 4 months ago

Closed 4 weeks ago

#29792 closed defect (fixed)

practracker problems and CI broken on master

Reported by: asn Owned by: teor
Priority: High Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: practracker, tech-debt, tor-ci, 041-deferred-20190530
Cc: Actual Points: 0.3
Parent ID: Points: 0.3
Reviewer: Sponsor: Sponsor31-can

Description

CI is broken on master because of the following practracker issues:

python3 ./scripts/maint/practracker/practracker.py .
problem function-size /src/feature/nodelist/nodelist.c:compute_frac_paths_available() 193
problem file-size /src/core/or/circuituse.c 3150

These were caused by #28656 and #29665.
They were not detected during the PR-check phase, because the PR was for 035 and 040 which dont include the practracker check. How should we avoid this issue in the future?

Child Tickets

TicketStatusOwnerSummaryComponent
#29880closedWrite a script to update practracker on multiple branchesCore Tor/Tor
#30033closedThe pre-push hook should call the pre-commit hook on the final pushed commitCore Tor/Tor
#30051closedrl1987Add practracker as a pre-commit and pre-push git hook for frequent codersCore Tor/Tor

Change History (12)

comment:1 Changed 4 months ago by asn

Keywords: tor-ci added

comment:2 Changed 4 months ago by asn

Status: newneeds_review

Fix for the issues can be found here: https://github.com/torproject/tor/pull/799

We should also figure out what to do about the greater problem mentioned in the top post, otherwise this is gonna come up again and again.

comment:3 Changed 4 months ago by nickm

I've merged the PR, but we should still think about good solutions here.

comment:4 Changed 4 months ago by asn

Status: needs_reviewnew

Potential solutions to the issue where practracker can break master, when we merge-forward bad commits from earlier releases (and the PR was on those earlier releases which don't contain practracker):

a) Add inline comments to denote practracker exceptions. Negative: Ugly.
b) Add practracker to all the active releases. Negative: This will still not solve the problem, since we would need to open PRs for all releases everytime we merge-forward to somewhere.
c) Add practracker as a post-commit git hook. Negative: Might be annoying. Not all volunteers will know about that thing.
d) Disable practracker in CI. Negative: We lose all the positive aspects of practracker that will probably be ignored.

comment:5 Changed 4 months ago by catalyst

Maybe if we can get metric diffs from practracker, we can see if an increase in badness exceeds some threshold before failing the build? and use the GitHub APIs to post a comment otherwise? (like Coveralls does)

comment:6 in reply to:  4 Changed 4 months ago by teor

Here's my suggested combination of fixes:

e) Disable practracker in CI, when a release becomes a release candidate:

  • we get practracker on master, where it is most useful
  • we keep practracker in alphas, where we still get some large code changes
  • we don't have to worry about practracker for backports:
    • we rely on the existing backport triage process to maintain code quality in backport branches
    • we don't need practracker for backport branches, because we (usually) don't make large changes to backport branches

f) Add practracker as a post-commit git hook for frequent coders

  • it might still be annoying, but it's less annoying than having CI fail
  • we can fix issues for infrequent coders at the CI or merge stage
  • reviewers can see the practracker diff and decide if it's reasonable

g) Add practracker as a pre-push git hook for merges

  • all mergers know about practracker anyway
  • it might still be annoying, but it's less annoying than having CI fail
  • we can fix practracker issues at merge time, in a separate commit, without another review

h) Add a "tolerance" argument to practracker, which makes it create an exceptions file with N% higher allowances

  • at the start of every new release, regenerate the file with a 10% tolerance
  • it would be nicer if we could review practice violations on a commit-by-commit basis

comment:7 Changed 3 months ago by teor

I opened child tickets for e) and f).
f) and #30033 will give us g).
I don't like h) any more.

comment:8 Changed 8 weeks ago by teor

Sponsor: Sponsor31-can

comment:9 Changed 7 weeks ago by nickm

Keywords: 041-deferred-20190530 added

Marking these tickets as deferred from 041.

comment:10 Changed 7 weeks ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final

comment:11 Changed 4 weeks ago by teor

Actual Points: 0.3
Owner: set to teor
Points: 0.3
Status: newassigned

#29981 is the only remaining open child ticket, so I un-parented it, and I'm closing this ticket as done.

A whole bunch of people worked on this ticket, so I'm not sure who to assign it to.

comment:12 Changed 4 weeks ago by teor

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.