#25341 closed enhancement (implemented)

Remove now-unnecessary Rust linking workaround

Reported by: isis Owned by:
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version: Tor: 0.3.2.2-alpha
Severity: Normal Keywords: rust, tor-build, autoconf, review-group-34, 034-triage-20180328, fast-fix, merge-deferred, 035-triaged-in-20180711
Cc: chelseakomlo Actual Points:
Parent ID: Points: .1
Reviewer: catalyst Sponsor:

Description (last modified by isis)

We've got the following stanza in our configure.ac:

  dnl This is a workaround for #46797                                                                           
  dnl (a.k.a https://github.com/rust-lang/rust/issues/46797 ).  Once the                                        
  dnl upstream bug is fixed, we can remove this workaround.                                                     
  case "$host_os" in                                                                                            
      darwin*)                                                                                                  
        TOR_RUST_EXTRA_LIBS="-lresolv"                                                                          
    ;;                                                                                                          
  esac

It looks like https://github.com/rust-lang/rust/issues/46797 has been resolved as of 22 January 2018, so we can probably remove this workaround now! (Someone who is on MacOS should probably test this, I don't have access to any Macs right now.)

Child Tickets

Change History (29)

comment:1 Changed 22 months ago by isis

Keywords: rust tor-build autoconf added

comment:2 Changed 22 months ago by isis

Status: newneeds_review

Patch in my bug25341 branch. Someone with a Mac, please test! :)

comment:3 Changed 22 months ago by isis

Description: modified (diff)

comment:4 Changed 22 months ago by teor

We can't remove this workaround until we stop supporting old rustc versions without the fix.

comment:5 in reply to:  4 Changed 22 months ago by isis

Replying to teor:

We can't remove this workaround until we stop supporting old rustc versions without the fix.


Okay! I made #25379 for figuring out more clearly which rustc(s) we support.

comment:6 Changed 22 months ago by nickm

Keywords: review-group-34 added

comment:7 Changed 21 months ago by dgoulet

Reviewer: catalyst

Reviewer week 03/19th

comment:8 Changed 21 months ago by catalyst

MacOS 10.12.6, with rust from homebrew:

  CCLD     src/or/tor
Undefined symbols for architecture x86_64:
  "_res_9_init", referenced from:
      std::sys::unix::net::res_init_if_glibc_before_2_26::he8c674397a656892 in libtor_rust.a(std-b11ab8f1585f119d.std15-94ef799fc274932c8f7e09df8e855996.rs.rcgu.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [src/or/tor] Error 1
make: *** [all] Error 2
mirkwood:tor tlyu$ rustc --version
rustc 1.24.1

I haven't tracked down any further yet.

comment:9 Changed 21 months ago by catalyst

Maybe the fix didn't make it into rust 1.24? It seems to work with nightly or whatever travis uses:
https://travis-ci.org/tlyu/tor/jobs/356676338

comment:10 Changed 21 months ago by catalyst

Status: needs_reviewneeds_revision

If I'm reading this correctly, it's not in stable yet? I'm not familiar with the rust release engineering processes.

https://github.com/rust-lang/rust/blob/stable/src/libstd/sys/unix/net.rs
https://github.com/rust-lang/rust/pull/47334
https://github.com/rust-lang/rust/commit/090a968fe7680cce0d3aa8fde25a5dc48948e43e

I'm not sure whether we want to defer this until that fix hits rust stable, or do something else.

comment:11 Changed 21 months ago by isis

catalyst is totally right, I just checked in my copy of the rust repo and it's in the current beta, which I think means we have to defer this until 1.25 is stable (2018-03-29 according to https://github.com/rust-lang/rust/blob/master/RELEASES.md).

Last edited 21 months ago by isis (previous) (diff)

comment:12 Changed 21 months ago by nickm

Keywords: 034-triage-20180328 added

comment:13 Changed 21 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:14 Changed 21 months ago by nickm

Keywords: fast-fix added; 034-removed-20180328 removed

comment:15 Changed 21 months ago by catalyst

Status: needs_revisionneeds_review

Looks like 1.25 is out, at least according to my Homebrew install. I'll give this another try once I upgrade.

comment:16 Changed 21 months ago by catalyst

Status: needs_reviewmerge_ready

Just tried with Rust 1.25 on macOS. Works now.

comment:17 Changed 21 months ago by nickm

Thank you! Out of selfishness and for my personal convenience, I am going to defer this merge until Fedora updates to 1.25.

comment:18 Changed 20 months ago by nickm

Update: Fedora is on 1.25 now, but it seems that sid is still on 1.24.

comment:19 Changed 19 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

Pessimisticly assuming we don't get to merge these until 0.3.5, and hoping I'll be wrong.

comment:20 Changed 17 months ago by nickm

Keywords: merge-deferred added

comment:21 Changed 17 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:22 Changed 17 months ago by cypherpunks3

Debian sid has had rustc 1.25 since May 18th. https://tracker.debian.org/news/958316/accepted-rustc-1250dfsg1-1-source-into-unstable/ Was that the last thing blocking merge?

Last edited 17 months ago by cypherpunks3 (previous) (diff)

comment:23 Changed 17 months ago by chelseakomlo

Cc: chelseakomlo added

comment:24 Changed 16 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

comment:25 Changed 15 months ago by teor

Our CI has rustc 1.29, so it is safe to merge this branch in 0.3.6.

comment:26 Changed 13 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:27 Changed 13 months ago by nickm

okay, picking this back up. I've made a PR at https://github.com/torproject/tor/pull/524 to make sure I handled the merge conflict correctly.

comment:28 Changed 13 months ago by nickm

looks like I messed up that merge. Trying another.

https://github.com/torproject/tor/pull/526

comment:29 Changed 13 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Okay, that one succeeded. Merged it to master.

Note: See TracTickets for help on using tickets.