Several problems with test-network Makefile target

In #18240 (moved), cypherpunks pointed out that we should use the same $(EXEEXT) syntax for finding the appropriate tor-gencert and tor-cov-gencert in the test-network target.

There's a couple other problems with that target:

• NETWORK_FLAVOUR=${NETWORK_FLAVOUR:-"bridges+hs"}: the "bridges+hs" network template doesn't exist anymore.
• If $use_coverage_binary is set when src/test/test-network.sh is called, then chutney will still try to use tor-gencert instead of tor-cov-gencert. (Also the filename extension problem mentioned above applies here also.

changed milestone to %Tor: unspecified

added 029-nickm-says-yes component::core tor/tor milestone::Tor: unspecified nickm-deferred-20161005 owner::cypherpunks points::1 priority::medium resolution::fixed review-group-3 severity::normal sponsor::S status::closed triage-out-030-201612 type::defect version::tor 0.2.8.2-alpha labels

Replying to isis:

In #18240 (moved), cypherpunks pointed out that we should use the same $(EXEEXT) syntax for finding the appropriate tor-gencert and tor-cov-gencert in the test-network target.

There's a couple other problems with that target:

• NETWORK_FLAVOUR=${NETWORK_FLAVOUR:-"bridges+hs"}: the "bridges+hs" network template doesn't exist anymore.

It does in the latest version of chutney - do you have an older version? https://gitweb.torproject.org/chutney.git/tree/networks

Replying to teor:

Replying to isis:

In #18240 (moved), cypherpunks pointed out that we should use the same $(EXEEXT) syntax for finding the appropriate tor-gencert and tor-cov-gencert in the test-network target.

There's a couple other problems with that target:

• NETWORK_FLAVOUR=${NETWORK_FLAVOUR:-"bridges+hs"}: the "bridges+hs" network template doesn't exist anymore.

It does in the latest version of chutney - do you have an older version? https://gitweb.torproject.org/chutney.git/tree/networks

Oops, heh. There it is now. One problem down.

The second problem is addressed in #18737 (moved) but is waiting for #18240 (moved) to be merged. Depending on whether coverage is enabled, the *-cov binaries may or may not be available. After the merge of #18240 (moved) i plan on adding a fixup patch that adds a variable for tor-gencert (like TESTING_TOR_BINARY) so the dependencies of the test-network target are dynamic. I'm waiting for the merge because the changes are overlapping and would lead to merge conflicts.

Replying to cypherpunks:

The second problem is addressed in #18737 (moved) but is waiting for #18240 (moved) to be merged. Depending on whether coverage is enabled, the *-cov binaries may or may not be available. After the merge of #18240 (moved) i plan on adding a fixup patch that adds a variable for tor-gencert (like TESTING_TOR_BINARY) so the dependencies of the test-network target are dynamic. I'm waiting for the merge because the changes are overlapping and would lead to merge conflicts.

Great! That fixes all of it except for the $(EXEEXT) problem for the tor-[cov-]gencert binary.

Please see my bug18933 branch (which is based on nickm's bug18240 branch).

Trac:
Status: new to needs_review

Replying to isis:

Replying to cypherpunks:

The second problem is addressed in #18737 (moved) but is waiting for #18240 (moved) to be merged. Depending on whether coverage is enabled, the *-cov binaries may or may not be available. After the merge of #18240 (moved) i plan on adding a fixup patch that adds a variable for tor-gencert (like TESTING_TOR_BINARY) so the dependencies of the test-network target are dynamic. I'm waiting for the merge because the changes are overlapping and would lead to merge conflicts.

Great! That fixes all of it except for the $(EXEEXT) problem for the tor-[cov-]gencert binary. I can think of only one clean way to communicate the $(EXEEXT) to the script and that is with parameters (similar to Stem's run-tests.py (see test-stem target)). The added benefit would be that it simplifies test-network.sh because it would just use the binaries as is which obsoletes the tor-path and coverage options.

Guess I should consider this as 029-proposed?

Trac:
Keywords: TorCoreTeam201605 deleted, TorCoreTeam201605 029-proposed added

Trac:
Reviewer: N/A to nickm

I think this ticket should be needs_revision as it does not fix the $(EXEEXT) problem yet.

Trac:
Status: needs_review to needs_revision

Changing owner because I'm working on a patch according to comment:5.

Trac:
Status: needs_revision to assigned
Owner: isis to cypherpunks

The test-network target also has the problem of not finding the path to Chutney when Tor is built out-of-tree. The need-chutney-path target can find it but that's only because it knows the source directory. Using the --tor-path option for specifying the source directory should make test-network.sh able to correctly detect Chutney. I'll make a patch for this.

Comparing the checks in test-network.sh and need-chutney-path shows lots of duplication so I'm thinking about removing the need-chutney-path target altogether. Unless there is a specific use-case for it that i am unaware of.

Trac:
0001-Add-support-for-specifying-Tor-binary-paths.patch

Trac:
0002-Use-the-new-test-network-script-options.patch

Trac:
0003-Remove-the-coverage-option.patch

Trac:
0004-Fix-Chutney-detection-when-building-out-of-tree.patch

Trac:
0005-Remove-the-need-chutney-path-target.patch

Created multiple patches to make the changes easier to review and they're based on the branch in comment:4.

Trac:
Status: assigned to needs_review

Trac:
Reviewer: nickm to teor

Trac:
Keywords: TorCoreTeam201605 029-proposed deleted, TorCoreTeam201605, 029-proposed, 029-nickm-says-yes added

Trac:
Points: small to 1

Calling thsese "yes" for 0.2.9 because they are externally submitted patches that won't be too hard to review.

Trac:
Keywords: 029-proposed deleted, review-group-2 added
Milestone: N/A to Tor: 0.2.9.x-final

Remove "TorCoreTeam201605" keyword. The time machine is broken.

Trac:
Keywords: TorCoreTeam201605 deleted, N/A added

This looks okay to me. I'd like the absolute-path stuff to become a function someday, but that doesn't seem too urgent.

(Hoping you don't mind me stealing the review from you, teor.)

Calling this merge-ready, but not merging right now, since the patches don't seem to apply cleanly to master; we'll have to clean it up a little as we go.

Trac:
Reviewer: teor to nickm
Status: needs_review to merge_ready

These have all had at least one review during review-group-2.

Trac:
Keywords: review-group-2 deleted, review-group-3 added

Okay; hand-applied and merged to master. Please reopen if I messed up doing the hand-apply.

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

Actually, it seems I probably messed something up. With these patches, 'make test-network-all' no longer passes for me. So, not merging.

I've stuck them in a branch ticket18933 instead in case anybody wants to have a look to see what's up.

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

Trac:
Status: reopened to needs_revision

Trac:
Reviewer: nickm to N/A

Replying to nickm:

Actually, it seems I probably messed something up. With these patches, 'make test-network-all' no longer passes for me. So, not merging.

I've stuck them in a branch ticket18933 instead in case anybody wants to have a look to see what's up. It seems you forgot to apply the patches on top of isis's branch in comment:4 as was stated in comment:11. Their branch defines the TESTING_TOR_GENCERT_BINARY variable which is used in the submitted patches.

make test-network-all does not work on master either.

Replying to cypherpunks:

make test-network-all does not work on master either. This was caused by building out-of-tree which the target does not handle properly. I've opened #19421 (moved) for this preexisting problem.

Deferring big/risky-feature things (even the ones I really love!) to 0.3.0. Please argue if I'm wrong.

Trac:
Keywords: N/A deleted, nickm-deferred-20161005 added
Milestone: Tor: 0.2.9.x-final to Tor: 0.3.0.x-final

I think we have fixed many of these issues already.

The latest tor version of this script calls the chutney version if it exists.

So we need to re-test for these issues, and open separate tickets for each of the remaining problems.

Then we should fix the script in chutney (as it is the script that's actually executed most of the time), and fix the script in tor if needed.

Trac:
Status: needs_revision to needs_information

I'll dust of the branch by rebasing it and taking a look at this again tomorrow.

Triaged out on December 2016 from 030 to Unspecified.

Trac:
Keywords: N/A deleted, triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-final to Tor: unspecified

I'd like to merge some of these fixes, but there have been significant changes to the test-network.sh script in both tor and chutney since they were written.

It's unclear how many of these issues still exist in the latest tor and chutney master branches.

I am also concerned that some of these patches will break make test-network-all for users already using environmental variables to set their tor and chutney paths.

Please open other tickets in either the tor or chutney components for the bugs that still exist.

Trac:
Resolution: N/A to fixed
Status: needs_information to closed

closed

changed time estimate to 8h

mentioned in issue #19421 (moved)

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