Opened 5 months ago

Closed 5 months ago

#25679 closed defect (implemented)

Default for TOR_RUST_DEPENDENCIES is wrong?

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: fast-fix, 033-included-20180403
Cc: Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description

Try building Tor using the tor-rust-dependencies submodule, without actually setting TOR_RUST_DEPENDENCIES. It's supposed to work, but it doesn't: it looks at the wrong directory.

Child Tickets

Change History (12)

comment:1 Changed 5 months ago by nickm

Status: assignedneeds_review

Tiny fix in bug25679_033

comment:2 Changed 5 months ago by asn

Reviewer: catalyst

comment:3 Changed 5 months ago by nickm

Keywords: 033-included-20180403 added

comment:4 Changed 5 months ago by catalyst

Status: needs_reviewneeds_revision

It looks like that doesn't currently work because of the following lines which override it:

    if test "x$NEED_MOD" = "x1"; then
      dnl When looking for dependencies from cargo, pick right directory
      TOR_RUST_DEPENDENCIES="../../src/ext/rust"
    fi

I'm not quite sure why it's trying to set it twice. It would be good to understand why, and what NEED_MOD is meant to indicate.

comment:5 Changed 5 months ago by nickm

Sebastian says that the NEED_MOD thing may have been a hack for out-of-tree builds, but if the build works without it, we should rip it out.

comment:6 Changed 5 months ago by nickm

I've added a fixup commit to bug25679_033, with better testing than before. It removes the NEED_MOD logic and makes sure that the path is absolute.

comment:7 Changed 5 months ago by nickm

Status: needs_revisionneeds_review

comment:8 Changed 5 months ago by Hello71

I still don't understand what ERRORED is for, but otherwise LGTM.

comment:9 Changed 5 months ago by catalyst

Status: needs_reviewneeds_revision

Thanks, mostly looks good!

A few nits:

  • ERRORED is probably not necessary because AC_MSG_ERROR already exits the configure script with an error
  • maybe pwd should be pwd -P || pwd, because bash will retain symbolic link path components by default unless set -o physical is in effect. (The || pwd is in case we're running on a shell that's too old to support pwd -P.)

Probably not for a bugfix (maybe a separate ticket):

  • using dnl for comments prevents them from appearing in the generated configure script; maybe these should be # comments instead. (I think it's OK to match surrounding style for now.)

comment:10 in reply to:  9 ; Changed 5 months ago by Hello71

Replying to catalyst:

  • maybe pwd should be pwd -P || pwd, because bash will retain symbolic link path components by default unless set -o physical is in effect. (The || pwd is in case we're running on a shell that's too old to support pwd -P.)

either pwd works, and just use it, or it doesn't work, and we need to pull in a real realpath. in this case, it should work fine, so just use pwd.

Probably not for a bugfix (maybe a separate ticket):

  • using dnl for comments prevents them from appearing in the generated configure script; maybe these should be # comments instead. (I think it's OK to match surrounding style for now.)

doesn't really matter, configure scripts are utterly unreadable anyways.

comment:11 in reply to:  10 Changed 5 months ago by catalyst

Status: needs_revisionmerge_ready

Replying to Hello71:

either pwd works, and just use it, or it doesn't work, and we need to pull in a real realpath. in this case, it should work fine, so just use pwd.

Good point. If cargo doesn't need parent directories to be "physical", then maybe it's not important and plain pwd will do.

We could also leave cleaning up of ERRORED to another ticket that isn't a bugfix; I don't feel too strongly either way. So merging this as-is for 0.3.3 is fine with me.

comment:12 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to maint-0.3.3 and forward; thanks!

Note: See TracTickets for help on using tickets.