Improve practracker unit tests, and run them in "make check" and pre-commit

changed milestone to %Tor: 0.4.3.x-final

Trac:
Parent Ticket: #32522 (moved)

added actualpoints::0.4 component::core tor/tor milestone::Tor: 0.4.3.x-final network-team-roadmap-november owner::teor parent::32522 points::0.4 priority::medium resolution::implemented reviewer::nickm severity::normal sponsor::31-can status::closed test type::enhancement labels

See my PR:

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

Trac:
Status: assigned to needs_review
Summary: Improve practracker unit tests, and run them in "make check" to Improve practracker unit tests, and run them in "make check" and pre-commit
Reviewer: N/A to nickm

This isn't quite the fix I wanted, so I'd like your advice.

I noticed that we merged a commit that included an entire ".c" file in a unit test, and I'd like "make check" to fail when that happens.

Is there a way we can restrict includes to ".h" and ".inc"? Perhaps with some custom code in practracker includes.py?

(I fixed the issue in d7fdab49f79c6c1ba3e0c8147ad9242f09d29f2c, but I'm concerned it got past review.)

This isn't quite the fix I wanted, so I'd like your advice.

The commits themselves look okay to me. Is the issue the one that you mention here?

Is there a way we can restrict includes to ".h" and ".inc"? Perhaps with some custom code in practracker includes.py?

We could add a .may_include to all directories, starting with ".h" and ".inc". Then practracker would start looking at them. Does that do what you want?

Right now it looks like there are some places where we include .c files intentionally. And arguably we should rename ".i" to ".inc"

[1424]$ git grep '#include' src/ |grep -v '.\(h\|inc\)[>"]'
src/ext/ed25519/donna/ed25519_tor.c:#include "test-internals.c"
src/ext/ed25519/donna/fuzz/ed25519-donna-sse2.c:#include "../ed25519.c"
src/ext/ed25519/donna/fuzz/ed25519-donna.c:#include "../ed25519.c"
src/ext/timeouts/bench/bench-wheel.c:#include "timeout.c"
src/ext/timeouts/lua/timeout-lua.c:#include "timeout.c"
src/ext/timeouts/timeout.c:#include "ext/timeouts/timeout-bitops.c"
src/lib/evloop/timers.c:#include "ext/timeouts/timeout.c"
src/lib/fs/files.c:#include "ext/getdelim.c"
src/lib/string/compat_string.c:#include "ext/strlcpy.c"
src/lib/string/compat_string.c:#include "ext/strlcat.c"
src/lib/version/git_revision.c:#include "micro-revision.i"
src/lib/version/git_revision.c:#include "micro-revision.i"

Replying to nickm:

This isn't quite the fix I wanted, so I'd like your advice.

The commits themselves look okay to me. Is the issue the one that you mention here?

Yes, the #include "feature/dircache/dirserv.c" in test_controller.c.

Is there a way we can restrict includes to ".h" and ".inc"? Perhaps with some custom code in practracker includes.py?

We could add a .may_include to all directories, starting with ".h" and ".inc". Then practracker would start looking at them. Does that do what you want?

No, because practracker just logs a warning, which most people ignore. (And CI doesn't fail.) We really do need to fail on weird include patterns like this.

Right now it looks like there are some places where we include .c files intentionally. And arguably we should rename ".i" to ".inc"

{{{ [1424]$ git grep '#include' src/ |grep -v '.(h|inc)[>"]' src/ext/ed25519/donna/ed25519_tor.c:#include "test-internals.c" src/ext/ed25519/donna/fuzz/ed25519-donna-sse2.c:#include "../ed25519.c" src/ext/ed25519/donna/fuzz/ed25519-donna.c:#include "../ed25519.c" src/ext/timeouts/bench/bench-wheel.c:#include "timeout.c" src/ext/timeouts/lua/timeout-lua.c:#include "timeout.c" src/ext/timeouts/timeout.c:#include "ext/timeouts/timeout-bitops.c" src/lib/evloop/timers.c:#include "ext/timeouts/timeout.c" src/lib/fs/files.c:#include "ext/getdelim.c" src/lib/string/compat_string.c:#include "ext/strlcpy.c" src/lib/string/compat_string.c:#include "ext/strlcat.c" src/lib/version/git_revision.c:#include "micro-revision.i" src/lib/version/git_revision.c:#include "micro-revision.i" }}}

If we:

• ignore "ext" (including and being included), and
• rename micro-revision.i to micro-revision.inc, then we can restrict includes to ".h" and ".inc".

Replying to teor:

Replying to nickm:

This isn't quite the fix I wanted, so I'd like your advice.

The commits themselves look okay to me. Is the issue the one that you mention here?

Yes, the #include "feature/dircache/dirserv.c" in test_controller.c.

Is there a way we can restrict includes to ".h" and ".inc"? Perhaps with some custom code in practracker includes.py?

We could add a .may_include to all directories, starting with ".h" and ".inc". Then practracker would start looking at them. Does that do what you want?

No, because practracker just logs a warning, which most people ignore. (And CI doesn't fail.) We really do need to fail on weird include patterns like this.

The .may_includes checker in practracker has two entry points: the one in practracker.py, and the one in includes.py directly that replaces our old checkIncludes.py. The latter does fail on violations, if the .may_includes file is not marked as advisory.

For example, add a .may_include file to src/test containing only '*.h', and run "make check". When I try this, "make check" fails.

Right now it looks like there are some places where we include .c files intentionally. And arguably we should rename ".i" to ".inc"

{{{ [1424]$ git grep '#include' src/ |grep -v '.(h|inc)[>"]' src/ext/ed25519/donna/ed25519_tor.c:#include "test-internals.c" src/ext/ed25519/donna/fuzz/ed25519-donna-sse2.c:#include "../ed25519.c" src/ext/ed25519/donna/fuzz/ed25519-donna.c:#include "../ed25519.c" src/ext/timeouts/bench/bench-wheel.c:#include "timeout.c" src/ext/timeouts/lua/timeout-lua.c:#include "timeout.c" src/ext/timeouts/timeout.c:#include "ext/timeouts/timeout-bitops.c" src/lib/evloop/timers.c:#include "ext/timeouts/timeout.c" src/lib/fs/files.c:#include "ext/getdelim.c" src/lib/string/compat_string.c:#include "ext/strlcpy.c" src/lib/string/compat_string.c:#include "ext/strlcat.c" src/lib/version/git_revision.c:#include "micro-revision.i" src/lib/version/git_revision.c:#include "micro-revision.i" }}}

If we:

• ignore "ext" (including and being included), and
• rename micro-revision.i to micro-revision.inc, then we can restrict includes to ".h" and ".inc".

I think that this general approach (ignoring ext and preferring .inc) is okay; so is listing files explicitly in .may_includes.

See my PR:

• master: https://github.com/torproject/tor/pull/1565

It will need to be squashed before merging.

I went with the simple change, and added the missing .may_include files. There are 4 !advisory files right now, in core/{crypto,mainloop,or,proto}. They won't catch "*.c" includes, but the others will. That's probably ok for now.

We can get more specific about the dependencies in future work.

I documented !advisory in practracker/includes.py, because it wasn't documented.

I also fixed an issue in the pre-commit hook, and added checkSpaceTest.sh from #32613 (moved) to that hook. (Otherwise, there would have been a merge conflict.)

Here's the script I used to create the new .may_include files:

$ cd tor
$ echo '*.h' > .may_include
$ for cf in `find src -name *.c`; do
    d=`dirname $cf`
    if [ ! -e $d/.may_include ]; then
        echo $d/.may_include
    fi
  done | sort -u | grep -v ext | xargs -n1 cp .may_include
$ echo '*.inc' >> src/app/config/.may_include
$ echo '*.inc' >> src/test/.may_include
$ rm .may_include

LGTM; squashed and merged.

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

closed

changed time estimate to 3h 12m

added 3h 12m of time spent

mentioned in issue #32705 (moved)

mentioned in issue #32522 (moved)

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

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