Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18240 closed defect (fixed)

'make test-stem' yields No rule to make target '"./src/or/tor"'

Reported by: arma Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: tor-tests-integration, tor-tests-stem, TorCoreTeam201605, TorCoreTeam-postponed-201604
Cc: Actual Points:
Parent ID: Points: 0.4
Reviewer: isis Sponsor: SponsorS-can

Description

Taking Nick's shiny new 0.2.8.1-alpha test tarball, and after building it, on Debian stable I get:

$/tmp/tor-0.2.8.1-alpha$ make test-stem
make: *** No rule to make target '"./src/or/tor"', needed by 'test-stem'.  Stop.

In the Makefile I see:

test-stem: need-stem-path $(TESTING_TOR_BINARY)

where $TESTING_TOR_BINARY is

TESTING_TOR_BINARY = "$(top_builddir)/src/or/tor"

I have a src/or/tor file just fine.

I don't know make language, so hopefully this is just an easy obvious fix.

Child Tickets

Change History (27)

comment:1 Changed 3 years ago by cypherpunks

Make is not shell script. Remove the quotes around the path.

comment:2 Changed 3 years ago by arma

yes indeed, that looks like it works!

sounds like somebody will want to go through and find the other instances like this.

comment:3 Changed 3 years ago by cypherpunks

I didn't look very hard but it seems like the only 2 instances come from the following commit: https://gitweb.torproject.org/tor.git/commit/?id=670affa792e7613f6e6b5d960805418359ac0ec7

where some fellow cpunk apparently got m4/make/shell messed up in his head, and then nickm not testing what he applies. But hey it's an alpha, right? ...

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

Replying to cypherpunks:

I didn't look very hard but it seems like the only 2 instances come from the following commit: https://gitweb.torproject.org/tor.git/commit/?id=670affa792e7613f6e6b5d960805418359ac0ec7

where some fellow cpunk apparently got m4/make/shell messed up in his head, and then nickm not testing what he applies. But hey it's an alpha, right? ...

This commit was added for #17818. The quotation of the make variables is indeed a mistake. Using git grep "=\"" "*.am" only turned up instances of shell snippets within make rules except for those 2 instances. I propose the following patch

diff --git a/Makefile.am b/Makefile.am
index fc9f7b2..62f7ef5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -23,9 +23,9 @@ AM_CFLAGS = @TOR_SYSTEMD_CFLAGS@
 SHELL = @SHELL@
 
 if COVERAGE_ENABLED
-TESTING_TOR_BINARY="$(top_builddir)/src/or/tor-cov"
+TESTING_TOR_BINARY=src/or/tor-cov$(EXEEXT)
 else
-TESTING_TOR_BINARY="$(top_builddir)/src/or/tor"
+TESTING_TOR_BINARY=src/or/tor$(EXEEXT)
 endif
 
 include src/include.am

The top_builddir variable is not necessary because Tor only has one Makefile and according to the Autoconf documentation this variable is equal to builddir (which is always ".") in the top-level directory of the build tree.
Furthermore, the Automake variable EXEEXT automatically contains the appropriate executable extension for the platform. It is used internally by Automake to create its rules so in order to call the correct rule we have to add it too.

comment:5 Changed 3 years ago by cypherpunks

The top_builddir variable is not necessary because Tor only has one Makefile and according to the Autoconf documentation this variable is equal to builddir (which is always ".") in the top-level directory of the build tree.

This happens to be the case now. The point is that the path is specified relative to the top level ("/src/or/tor"). So don't remove it, otherwise one would need to adjust it when/if the Makefile moves.

I can't comment about EXEEXT.

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

Replying to cypherpunks:

The top_builddir variable is not necessary because Tor only has one Makefile and according to the Autoconf documentation this variable is equal to builddir (which is always ".") in the top-level directory of the build tree.

This happens to be the case now. The point is that the path is specified relative to the top level ("/src/or/tor"). So don't remove it, otherwise one would need to adjust it when/if the Makefile moves.

The Makefile is not going to move unless Tor moves back to recursive make (which is unlikely to happen, for the arguments see commit 2a4a1496241d6c4183763f90600be4775ccb5470). Iff that change happens the entire build configuration has to be rewritten because everything is currently specified relative to the top level (which can be seen in the include.am files).
Additionally, the generated Makefile contains no rules that include the builddir for building the tor binary but it does contain the rule

src/or/tor$(EXEEXT): $(src_or_tor_OBJECTS) $(src_or_tor_DEPENDENCIES) $(EXTRA_src_or_tor_DEPENDENCIES) src/or/$(am__dirstamp)

For these reasons i believe the builddir can safely be removed from the TESTING_TOR_BINARY variable. If there are instances where this would fail, i'm interested in hearing about them.

I can't comment about EXEEXT.

If you're interested you can read more about it in the Automake documentation.

comment:7 Changed 3 years ago by cypherpunks

Status: newneeds_review
Version: Tor: 0.2.8.1-alpha

The issue has been fixed in 040ec4d0347235da4fd5696b3eb063e609047024 outside of this ticket. However, i believe the patch in comment:4 is still worth applying.

comment:8 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

comment:9 Changed 3 years ago by isabela

Sponsor: SponsorS-can

comment:10 Changed 3 years ago by nickm

Points: small-remaining

comment:11 Changed 3 years ago by nickm

Keywords: TorCoreTeam201604 added

Every postponed needs_review ticket should get a review in April

comment:12 Changed 3 years ago by nickm

Owner: set to nickm
Status: needs_reviewassigned

I think I'll retain the top_builddir, since it isn't actually hurting anything.

I needed to add some quotes and change some other stuff to make the dependencies make sense again.

Please have a look at branch bug18240 in my public repository?

comment:13 Changed 3 years ago by nickm

Status: assignedneeds_review

comment:14 in reply to:  12 Changed 3 years ago by cypherpunks

Replying to nickm:

Please have a look at branch bug18240 in my public repository?

Making the test-network and test-network-all rules depend on TESTING_TOR_BINARY is redundant because they already depend on the all rule. Also these two rules actually need to depend on the all rule because they not only need the tor binary but also the tor-gencert binary.

Be sure to update the changes file if you revert the changes on these two rules.

comment:15 Changed 3 years ago by nickm

Hm. Actually no, they should only depend on what they use.

Added a fixup commit.

comment:16 Changed 3 years ago by cypherpunks

Looking at this again it's pretty clear that $(top_builddir) and $(top_srcdir) are not being used consistently. Perhaps some day they'll be cleaned up.

comment:17 in reply to:  15 ; Changed 3 years ago by cypherpunks

Replying to nickm:

Hm. Actually no, they should only depend on what they use.

Added a fixup commit.

As with the TESTING_TOR_BINARY change, the tor-gencert dependency should include top_builddir and EXEEXT.

Note that this dependency currently only works because the test-network script does not use tor-cov-gencert binary when in coverage mode (see #18737).

comment:18 Changed 3 years ago by nickm

Keywords: tor-tests-integration added

comment:19 Changed 3 years ago by nickm

Keywords: tor-tests-stem added

comment:20 Changed 3 years ago by isis

Reviewer: isis

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

Replying to cypherpunks:

Replying to nickm:

Hm. Actually no, they should only depend on what they use.

Added a fixup commit.

As with the TESTING_TOR_BINARY change, the tor-gencert dependency should include top_builddir and EXEEXT.


It should, for the sake of writing a nice Makefile, although it's not strictly necessary for fixing this bug.

Note that this dependency currently only works because the test-network script does not use tor-cov-gencert binary when in coverage mode (see #18737).


AFAICT, the stem integration tests don't use the tor-gencert or tor-cov-gencert binaries.

comment:22 in reply to:  12 Changed 3 years ago by isis

Status: needs_reviewmerge_ready

Replying to nickm:

Please have a look at branch bug18240 in my public repository?


LGTM. Maybe mirror the changes for gencert (see comment #21), if you feel like doing so makes the Makefile cleaner.

comment:23 in reply to:  21 ; Changed 3 years ago by cypherpunks

Replying to isis:

Replying to cypherpunks:

Replying to nickm:
Note that this dependency currently only works because the test-network script does not use tor-cov-gencert binary when in coverage mode (see #18737).


AFAICT, the stem integration tests don't use the tor-gencert or tor-cov-gencert binaries.

The test-network script uses Chutney which needs tor(-cov)-gencert for configuring its relays.

comment:24 in reply to:  23 Changed 3 years ago by isis

Replying to cypherpunks:

Replying to isis:

Replying to cypherpunks:

Replying to nickm:
Note that this dependency currently only works because the test-network script does not use tor-cov-gencert binary when in coverage mode (see #18737).

AFAICT, the stem integration tests don't use the tor-gencert or tor-cov-gencert binaries.

The test-network script uses Chutney which needs tor(-cov)-gencert for configuring its relays.

Okay, good point. That (and some other problems preventing running that target) are now #18933.

comment:25 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 TorCoreTeam-postponed-201604 added; TorCoreTeam201604 removed

April is over; calling these april tickets postponed into may.

comment:26 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Squashed and merged!

comment:27 Changed 3 years ago by isabela

Points: small-remaining0.4
Note: See TracTickets for help on using tickets.