Opened 3 months ago

Closed 3 weeks ago

#25895 closed defect (fixed)

Cross-compiling tor rust for Windows is broken

Reported by: gk Owned by: Hello71
Priority: High Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust, 034-proposed, tbb-wants, 033-backport, 034-roadmap-proposed, 034-must
Cc: boklm, chelseakomlo Actual Points:
Parent ID: #25849 Points:
Reviewer: catalyst Sponsor:

Description

I managed to cross-compile at least a rust compiler for 64bit Windows but tor is not prepared for that:

x86_64-w64-mingw32-gcc  -mwindows -fstack-protector-all -Wstack-protector --param ssp-buffer-size=4 -fno-strict-overflow -Wno-missing-field-initializers -Wformat -Wformat-security -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wstack-protector --param ssp-buffer-size=1 -fasynchronous-unwind-tables -Wall -fno-strict-aliasing -Waddress -Warray-bounds -Wdate-time -Wdouble-promotion -Wduplicated-cond -Wextra -Wfloat-conversion -Wignored-attributes -Winit-self -Wlogical-op -Wmissing-field-initializers -Wmissing-format-attribute -Wmissing-noreturn -Wnormalized=nfkc -Wnull-dereference -Woverlength-strings -Woverride-init -Wshadow -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value -Wshift-overflow=2 -Wsizeof-array-argument -Wstrict-overflow=1 -Wsuggest-attribute=format -Wsuggest-attribute=noreturn -Wswitch-bool -Wsync-nand -Wtrampolines -Wunused-but-set-parameter -Wunused-but-set-variable -Wunused-const-variable=2 -Wunused-local-typedefs -Wvariadic-macros -W -Wfloat-equal -Wundef -Wpointer-arith -Wstrict-prototypes -Wmissing-prototypes -Wwrite-strings -Wredundant-decls -Wchar-subscripts -Wcomment -Wformat=2 -Wwrite-strings -Wnested-externs -Wbad-function-cast -Wswitch-enum -Waggregate-return -Wpacked -Wunused -Wunused-parameter  -Wold-style-definition -Wmissing-declarations  -mwindows -Wl,--dynamicbase -Wl,--nxcompat -Wl,--enable-reloc-section -lssp -L/var/tmp/dist/mingw-w64/gcclibs -Wl,--nxcompat -Wl,--dynamicbase -o src/tools/tor-resolve.exe src/tools/tor-resolve.o src/common/libor.a src/common/libor-ctime.a ./src/rust/target/release/tor_rust.lib  -lws2_32 -luserenv  
x86_64-w64-mingw32-gcc: error: ./src/rust/target/release/tor_rust.lib: No such file or directory

We don't want to have a *lib file I think. What we get instead when cross-compiling (libtor_rust.a) looks actually promising.

Child Tickets

Change History (26)

comment:1 Changed 3 months ago by boklm

Cc: boklm added

comment:2 Changed 3 months ago by teor

Keywords: 034-proposed tbb-wants added

comment:3 Changed 3 months ago by nickm

Keywords: 033-backport added
Milestone: Tor: 0.3.4.x-final
Priority: MediumHigh

comment:4 Changed 3 months ago by nickm

Keywords: 034-roadmap-proposed added

comment:5 Changed 3 months ago by ahf

Owner: set to ahf
Status: newassigned

comment:6 Changed 3 months ago by Hello71

uh... are you sure you were actually cross compiling? right now cross-compiling tor rust for *anything* is probably broken, because it doesn't pass the CHOST in as the target.

the cleanest way to do this AFAICT is:

if CBUILD != CHOST, set build.target = @CHOST@ in src/rust/.cargo/config, and set tor rust path to src/rust/target/@CHOST@/release/@tor_rust_static_name@. as far as I can tell, cargo does produce a lib file on Windows, and this is the correct extension for us. edit: we probably also need to set target.@CHOST@.{linker,ar} to $LD and $AR. edit 2: apparently ar isn't used anymore, and I'm pretty sure we don't need the linker to build a static library.

else, do not set build.target (build.target = '' causes an error), set tor rust path to src/rust/target/release/@tor_rust_static_name@ (the same as it is now).

you can see this at https://cgit.alxu.ca/tor.git/tree/src/rust?h=meson in {,.cargo/,tor_rust/}meson.build, .cargo/config.in, and tor_rust/build.sh. meson doesn't provide a target triple, so I ask the user to provide it. for autoconf, we would just use @CHOST@.

https://github.com/japaric/rust-cross

Last edited 3 months ago by Hello71 (previous) (diff)

comment:7 Changed 2 months ago by Hello71

Owner: changed from ahf to Hello71

yoink

comment:8 Changed 2 months ago by Hello71

Status: assignedneeds_revision

so it turns out it wasn't actually as easy as I thought, but definitely not too bad.

https://cgit.alxu.ca/tor.git/log/?h=rust-cross

I based my branch off of my misc-rust (#26038), but it doesn't actually depend on that one.

for some reason automake doesn't feel like adding the dependency though...

comment:9 Changed 2 months ago by Hello71

Status: needs_revisionneeds_review

I !@#$ing hate %&*ing automake.

Last edited 2 months ago by Hello71 (previous) (diff)

comment:10 Changed 2 months ago by gk

What you actually want is not the LLVM target triple, but the Rust target triple. So, instead of "LLVM target triple (for Rust)" just use "Rust target triple".

comment:11 in reply to:  10 ; Changed 2 months ago by gk

Replying to gk:

What you actually want is not the LLVM target triple, but the Rust target triple. So, instead of "LLVM target triple (for Rust)" just use "Rust target triple".

Better even "Rust target" as the target is strictly speaking not a triple here.

comment:12 Changed 2 months ago by cypherpunks

comment:13 in reply to:  11 Changed 2 months ago by Hello71

Replying to gk:

Replying to gk:

What you actually want is not the LLVM target triple, but the Rust target triple. So, instead of "LLVM target triple (for Rust)" just use "Rust target triple".

Better even "Rust target" as the target is strictly speaking not a triple here.

strictly strictly speaking, Rust takes a target specifier in LLVM format (which is apparently very similar to, but of course incompatible with autotools format). I guess it's not wrong to say Rust target though.

comment:14 Changed 2 months ago by asn

Reviewer: catalyst

comment:15 Changed 2 months ago by chelseakomlo

Cc: chelseakomlo added

comment:16 Changed 7 weeks ago by teor

Keywords: 034-must added

We found out in #25977 that cargo isn't getting the target triple.
When we fix that, it should fix both bugs.

comment:17 Changed 7 weeks ago by gk

FWIW, I tested Hello71's fix a while back and it solves the cross-compiling issue in this bug.

comment:19 in reply to:  18 ; Changed 6 weeks ago by catalyst

Status: needs_reviewneeds_revision

Replying to Hello71:

summary (for reviewing): https://cgit.alxu.ca/tor.git/diff/?id=rust-cross&id2=fix-rust-tests

Thanks for the patches! Could you please rebase them on the latest master? (without the fix-rust-tests patches?) They don't seem to apply cleanly. Also a GitHub pull request would help us to review the changes more effectively.

Please use some other variable like RUST_TARGET instead of TARGET because the variable target is set by autoconf someone passes the --target= option on the ./configure command line. Or maybe an option like --with-rust-target= would be better? I'm not sure which would be more idiomatic.

I think CHOST and CBUILD aren't standard autoconf terminology; what system uses those terms? HOST and BUILD would be better. (These are the terms that the help text in ./configure uses to refer to the arguments passed to the --host= and --build= options.)

comment:20 in reply to:  19 ; Changed 6 weeks ago by Hello71

Replying to catalyst:

Replying to Hello71:

summary (for reviewing): https://cgit.alxu.ca/tor.git/diff/?id=rust-cross&id2=fix-rust-tests

Thanks for the patches! Could you please rebase them on the latest master? (without the fix-rust-tests patches?) They don't seem to apply cleanly. Also a GitHub pull request would help us to review the changes more effectively.

Rebased. https://cgit.alxu.ca/tor.git/diff/?id=rust-cross&id2=master. I don't like GitHub. I use Travis grudgingly. https://travis-ci.org/Hello71/tor/builds/392516161

Please use some other variable like RUST_TARGET instead of TARGET because the variable target is set by autoconf someone passes the --target= option on the ./configure command line. Or maybe an option like --with-rust-target= would be better? I'm not sure which would be more idiomatic.

I renamed it to RUST_TARGET.

I think CHOST and CBUILD aren't standard autoconf terminology; what system uses those terms? HOST and BUILD would be better. (These are the terms that the help text in ./configure uses to refer to the arguments passed to the --host= and --build= options.)

Gentoo calls it CHOST (presumably because of this namespacing issue), I thought autoconf did too.

As an aside, I researched how Firefox handles this, and it appears that they hardcode a list of targets: https://dxr.mozilla.org/mozilla-central/source/layout/style/ServoBindings.toml. I think this solution is ugly, especially since we aim for Tor to be as compatible as possible rather than limiting to Windows, Mac, Linux.

comment:21 Changed 5 weeks ago by catalyst

Status: needs_revisionneeds_review

comment:22 in reply to:  20 ; Changed 4 weeks ago by catalyst

Replying to Hello71:

Rebased. https://cgit.alxu.ca/tor.git/diff/?id=rust-cross&id2=master. I don't like GitHub. I use Travis grudgingly. https://travis-ci.org/Hello71/tor/builds/392516161

Thanks!

+# not a no-op, fixes out-of-source builds
+target-dir = './target'

Could you please explain the details of what this fixes for out-of-souce-dir builds? Is it cargo build or cargo test that fails? Online or offline Rust dependencies?

comment:23 in reply to:  22 Changed 4 weeks ago by Hello71

Replying to catalyst:

Replying to Hello71:

+# not a no-op, fixes out-of-source builds
+target-dir = './target'

Could you please explain the details of what this fixes for out-of-souce-dir builds? Is it cargo build or cargo test that fails? Online or offline Rust dependencies?

think I rebased wrong, sorry. rebased on master and force-pushed to the same branch.

Last edited 4 weeks ago by Hello71 (previous) (diff)

comment:24 Changed 4 weeks ago by catalyst

Status: needs_reviewmerge_ready

Thanks for the patches! I rebased to maint-0.3.4, adjusted some variable names and error messages in the configure script, and squashed the commits.

Pull request at:
https://github.com/torproject/tor/pull/189

comment:25 Changed 4 weeks ago by catalyst

I've checked that the compilation succeeds; I don't have a setup readily available to test the resulting binaries.

comment:26 Changed 3 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

seems reasonable to me. Merged to 0.3.4 and forward!

Note: See TracTickets for help on using tickets.