Closed Bug 1512271 Opened 2 years ago Closed 2 years ago

Upgrade to rustc 1.31


(Firefox Build System :: General, enhancement)

Not set


(firefox66 fixed)

Tracking Status
firefox66 --- fixed


(Reporter: chmanchester, Assigned: chmanchester)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

The release is tomorrow, December 6th, however we're already in the soft freeze, so this will wait until after the merge.
Hi Simon, the rust tests builds start failing with this version with linker errors like Does this workaround need an update for rustc 1.31? Is this something you could take a look at? Here's the try push:
Flags: needinfo?(simon.sapin)
I really don’t know what’s going on here. I think that the work-around at is not relevant because Gecko (libxul) should be available, unlike when running Rust’s `src/tools/cargotest` test suite.

Emilio, any idea where to look?

I see from the log that `mach build -v pre-export export recurse_rusttests` is the command failing, but I don’t find the exact Cargo command that that runs.
Flags: needinfo?(simon.sapin) → needinfo?(emilio)
It's in the log it can just be tricky to spot:

01:15:48     INFO -  env  RUSTC_WRAPPER='z:/build/build/src/sccache2/sccache.exe' CARGO_TARGET_DIR=z:/build/build/src/obj-firefox RUSTFLAGS='-C opt-level=2 -C debuginfo=2  ' RUSTC=z:/build/build/src/rustc/bin/rustc.exe RUSTDOC=z:/build/build/src/rustc/bin/rustdoc.exe RUSTFMT=z:/build/build/src/rustc/bin/rustfmt.exe CFLAGS_i686_pc_windows_msvc="-w"  MOZ_SRC=z:/build/build/src MOZ_DIST=z:/build/build/src/obj-firefox/dist LIBCLANG_PATH="z:/build/build/src/clang/bin" CLANG_PATH="z:/build/build/src/clang/bin/clang.exe" PKG_CONFIG_ALLOW_CROSS=1 RUST_BACKTRACE=full MOZ_TOPOBJDIR=z:/build/build/src/obj-firefox CARGO_INCREMENTAL=0  z:/build/build/src/rustc/bin/cargo.exe test --target=i686-pc-windows-msvc --no-fail-fast -p selectors -p servo_arc -p stylo_tests --features "servo bindgen gecko_debug quantum_render simd-accel moz_memory spidermonkey_rust gecko_profiler"  --frozen --manifest-path z:/build/build/src/toolkit/library/rust/Cargo.toml -vv
Not off-hand, but taking a closer look:

01:26:42     INFO -  warning: lint `private_no_mangle_fns` has been removed: `no longer an warning, #[no_mangle] functions always exported`
01:26:42     INFO -    --> servo\ports\geckolib\tests\
01:26:42     INFO -     |
01:26:42     INFO -  19 | #[allow(non_snake_case, unused_unsafe, private_no_mangle_fns)]
01:26:42     INFO -     |                                        ^^^^^^^^^^^^^^^^^^^^^
01:26:42     INFO -     |
01:26:42     INFO -     = note: #[warn(renamed_and_removed_lints)] on by default

Which comes from, which is in the range.

So it seems we are relying on Rust not exporting private #[no_mangle] functions so that link.exe could strip them out (since they're not called).
Flags: needinfo?(emilio)
So I suspect something like this should do the trick if I'm right, though I have no windows machine to test.

Chris, does that fix the issue?

Longer term we should just avoid relying on this test that uses the bindgen-generated signatures and switch to cbindgen for this job, presumably.
Attachment #9030444 - Flags: review?(simon.sapin)
Attachment #9030444 - Flags: feedback?
Attachment #9030444 - Flags: feedback? → feedback?(cmanchester)
Comment on attachment 9030444 [details] [diff] [review]
Allow link.exe to keep linking the stylo tests after rust-lang/rust#54451.

Review of attachment 9030444 [details] [diff] [review]:

Ok, the theory that this is due to sounds very plausible. This looks like an appropriate fix. r+ assuming a successful try run.

I chatted with Emilio on IRC to understand how this all works: we have `extern fn` Rust functions in servo/ports/geckolib/, and hand-written C++ declarations in layout/style/ServoBindings.h so that they can be called form C++. There’s a test that uses Bindgen to convert those C++ headers back to Rust declarations (in this generated file), and compares function pointer types to make sure the signatures are identical.
Attachment #9030444 - Flags: review?(simon.sapin) → review+
Comment on attachment 9030444 [details] [diff] [review]
Allow link.exe to keep linking the stylo tests after rust-lang/rust#54451.

Review of attachment 9030444 [details] [diff] [review]:

Try looks good, thank you very much for the patch!
Attachment #9030444 - Flags: feedback?(cmanchester) → feedback+
Pushed by
Allow link.exe to keep linking the stylo tests after rust-lang/rust#54451. r=simonsapin
Upgrade builders to rustc 1.31 r=ted
Attachment #9030547 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.