Opened 3 years ago

Closed 3 years ago

#18933 closed defect (fixed)

Several problems with test-network Makefile target

Reported by: isis Owned by: cypherpunks
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.8.2-alpha
Severity: Normal Keywords: 029-nickm-says-yes, review-group-3, nickm-deferred-20161005, triage-out-030-201612
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor: SponsorS

Description

In #18240, 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.

Child Tickets

Attachments (5)

Change History (36)

comment:1 in reply to:  description ; Changed 3 years ago by teor

Replying to isis:

In #18240, 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

comment:2 in reply to:  1 Changed 3 years ago by isis

Replying to teor:

Replying to isis:

In #18240, 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.

comment:3 Changed 3 years ago by cypherpunks

The second problem is addressed in #18737 but is waiting for #18240 to be merged. Depending on whether coverage is enabled, the *-cov binaries may or may not be available. After the merge of #18240 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.

comment:4 in reply to:  3 ; Changed 3 years ago by isis

Status: newneeds_review

Replying to cypherpunks:

The second problem is addressed in #18737 but is waiting for #18240 to be merged. Depending on whether coverage is enabled, the *-cov binaries may or may not be available. After the merge of #18240 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).

comment:5 in reply to:  4 Changed 3 years ago by cypherpunks

Replying to isis:

Replying to cypherpunks:

The second problem is addressed in #18737 but is waiting for #18240 to be merged. Depending on whether coverage is enabled, the *-cov binaries may or may not be available. After the merge of #18240 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.

comment:6 Changed 3 years ago by nickm

Keywords: 029-proposed added

Guess I should consider this as 029-proposed?

comment:7 Changed 3 years ago by nickm

Reviewer: nickm

comment:8 Changed 3 years ago by cypherpunks

Status: needs_reviewneeds_revision

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

comment:9 Changed 3 years ago by cypherpunks

Owner: changed from isis to cypherpunks
Status: needs_revisionassigned

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

comment:10 Changed 3 years ago by 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.

Changed 3 years ago by cypherpunks

Changed 3 years ago by cypherpunks

Changed 3 years ago by cypherpunks

comment:11 Changed 3 years ago by cypherpunks

Status: assignedneeds_review

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

comment:12 Changed 3 years ago by nickm

Reviewer: nickmteor

comment:13 Changed 3 years ago by nickm

Keywords: 029-nickm-says-yes added

comment:14 Changed 3 years ago by isabela

Points: small1

comment:15 Changed 3 years ago by nickm

Keywords: review-group-2 added; 029-proposed removed
Milestone: Tor: 0.2.9.x-final

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

comment:16 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 removed

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

comment:17 Changed 3 years ago by nickm

Reviewer: teornickm
Status: needs_reviewmerge_ready

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.

comment:18 Changed 3 years ago by nickm

Keywords: review-group-3 added; review-group-2 removed

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

comment:19 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

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

comment:20 Changed 3 years ago by nickm

Resolution: implemented
Status: closedreopened

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.

comment:21 Changed 3 years ago by nickm

Status: reopenedneeds_revision

comment:22 Changed 3 years ago by nickm

Reviewer: nickm

comment:23 Changed 3 years ago by nickm

Reviewer: nickm

comment:24 in reply to:  20 Changed 3 years ago by cypherpunks

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.

comment:25 Changed 3 years ago by cypherpunks

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

comment:26 in reply to:  25 Changed 3 years ago by cypherpunks

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 for this preexisting problem.

comment:27 Changed 3 years ago by nickm

Keywords: nickm-deferred-20161005 added
Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final

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

comment:28 Changed 3 years ago by teor

Status: needs_revisionneeds_information

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.

comment:29 Changed 3 years ago by cypherpunks

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

comment:30 Changed 3 years ago by dgoulet

Keywords: triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-finalTor: unspecified

Triaged out on December 2016 from 030 to Unspecified.

comment:31 Changed 3 years ago by teor

Resolution: fixed
Status: needs_informationclosed

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.

Note: See TracTickets for help on using tickets.