Opened 2 months ago

Closed 7 weeks ago

#31919 closed enhancement (fixed)

Add a check-local target that runs our coccinelle parsing problems script

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 042-can, extra-review
Cc: Actual Points: 0.9
Parent ID: Points: 0.5
Reviewer: catalyst, nickm Sponsor:

Description

In #31705, we add a script that detects coccinelle parsing problems. We should run it when we run "make check", if coccinelle is installed.

Child Tickets

Change History (15)

comment:1 Changed 7 weeks ago by teor

Actual Points: 0.3
Milestone: Tor: 0.4.2.x-finalTor: 0.4.3.x-final
Reviewer: nickm
Status: newneeds_review

See my draft pull request:

TODO:

  • merge #31705 and rebase
  • squash the two Travis commits
  • add a changes file
  • run 'make autostyle-operators' or 'make autostyle', and see if the exceptions file gets smaller

comment:2 Changed 7 weeks ago by teor

Oh, it's based on #31705, you only want the last 6 commits.

Edit: counting is hard

Last edited 7 weeks ago by teor (previous) (diff)

comment:3 Changed 7 weeks ago by teor

Status: needs_reviewneeds_revision

It looks like we might need to upgrade to the xenial or bionic images to get a recent enough version of spatch. Testing now.

comment:4 Changed 7 weeks ago by teor

The minimum required version seems to be ~1.0.4:

  • 1.0.0-rc19 does not work
  • 1.0.4 works on Debian
  • 1.0.6 works on macOS

comment:5 Changed 7 weeks ago by nickm

Quick comment on patch:

I'm not 100% sure about requiring everybody to install coccinelle for them to be able to run the pre-commit hook.

comment:6 in reply to:  5 Changed 7 weeks ago by teor

Replying to nickm:

Quick comment on patch:

I'm not 100% sure about requiring everybody to install coccinelle for them to be able to run the pre-commit hook.

If spatch is not installed, the script logs a message and exits without error:
https://github.com/torproject/tor/pull/1454/commits/616bc21859c50f75e0f1092cab0dca0de62f47b9#diff-e4d63afb05764cd8a5149d2f09edd87dR21

I'm not entirely happy with the speed of spatch, but let's see how it goes. Maybe there's some nice way to do it in parallel.

I'm making progress on this, just doing performance checks now.

comment:7 Changed 7 weeks ago by teor

Parent ID: #31705

comment:8 Changed 7 weeks ago by teor

Actual Points: 0.30.8
Status: needs_revisionneeds_review

This was slightly more complicated than I expected, we needed Ubuntu Bionic for a recent spatch, and spatch is so slow that we can only check modified files.

See my PR:

comment:9 Changed 7 weeks ago by teor

We should consider backporting "Travis: Use Ubuntu Bionic, but keep Trusty for chutney" before we merge this ticket.

I'll open a child ticket.

comment:10 Changed 7 weeks ago by nickm

Status: needs_reviewmerge_ready

This looks pretty good to me. I'm only so-so on git hooks, though, so I'd like somebody else to take a second look at those changes before we merge.

comment:11 Changed 7 weeks ago by teor

I force-pushed the branch to remove the commit and changes entry for "Travis: Use Ubuntu Bionic, but keep Trusty for chutney". I'm backporting that change in #32260, all the other changes go in master.

comment:12 Changed 7 weeks ago by teor

Keywords: extra-review added
Reviewer: nickm

Marking as extra-review for the git hooks commits:

  • scripts/git: Add check_cocci_parse.sh to the pre-commit hook
  • scripts/git: Only check modified files in existing git hook checks
  • scripts/git: Make spaces consistent in pre-push.git-hook

Here's how you can check them:

Coccinelle checks for correct C syntax. Here's how you can test it:

  • Modify a file to have really bad C syntax
  • Run a hook or make check-cocci
  • Files in scripts/coccinelle/exceptions.txt are ignored, they have known bad syntax
  • Files in src/ext are also ignored

pre-commit hook:

  • At pre-push time, the pre-commit hook takes a list of changed files from the pre-push hook. So you can call the pre-commit hook manually with a list of files
  • At pre-commit time, the pre-commit hook creates the changed file list itself. So you can call it with no arguments, and a set of staged (git add) files
  • Install the hooks, and coccinelle, and try a commit

pre-push hook:

  • Install the hooks, and coccinelle, and try a push

comment:13 Changed 7 weeks ago by teor

Status: merge_readyneeds_review

comment:14 Changed 7 weeks ago by catalyst

Reviewer: catalyst
Status: needs_reviewneeds_revision

I manually tested the pre-commit and pre-push hooks. They seem to work as advertised.

Changes seem reasonable to me by visual inspection.

There are some whitespace nits in Makefile.am that I commented on in the pull request.

comment:15 Changed 7 weeks ago by teor

Actual Points: 0.80.9
Resolution: fixed
Reviewer: catalystcatalyst, nickm
Status: needs_revisionclosed

Fixed the spacing in the new code in the existing commit, and added another commit that removes spaces from all the other file lines.

#30991 will automatically detect or fix issues like this in future.

Merged to master.

Note: See TracTickets for help on using tickets.