Default for TOR_RUST_DEPENDENCIES is wrong?

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.

changed milestone to %Tor: 0.3.3.x-final in legacy/trac

added 033-included-20180403 in Legacy / Trac component::core tor/tor in Legacy / Trac fast-fix in Legacy / Trac milestone::Tor: 0.3.3.x-final in Legacy / Trac owner::nickm in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac reviewer::catalyst in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac labels

Tiny fix in bug25679_033

Trac:
Status: assigned to needs_review

Trac:
Reviewer: N/A to catalyst

Trac:
Keywords: N/A deleted, 033-included-20180403 added

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.

Trac:
Status: needs_review to needs_revision

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.

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.

Trac:
Status: needs_revision to needs_review

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

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.)

Trac:
Status: needs_review to needs_revision

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.

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.

Trac:
Status: needs_revision to merge_ready

Merged to maint-0.3.3 and forward; thanks!

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

closed

moved from legacy/trac#25679 (moved)

added Bug label and removed 1 deleted label

removed 1 deleted label