Closed Bug 1512271 Opened 2 years ago Closed 2 years ago

Upgrade to rustc 1.31

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox66 fixed)

RESOLVED FIXED
Tracking Status
firefox66 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(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 https://github.com/rust-lang/rust/pull/44603#issuecomment-338807312 Does this workaround need an update for rustc 1.31? Is this something you could take a look at? Here's the try push: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=216291408&revision=9f4b8f6207de6a10c83a136d60caf687959de146
Flags: needinfo?(simon.sapin)
I really don’t know what’s going on here. I think that the work-around at https://github.com/servo/servo/pull/19003 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\servo_function_signatures.rs:19:40
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 https://github.com/rust-lang/rust/pull/54451, 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 https://github.com/rust-lang/rust/pull/54451 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/glue.rs, 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 glue.rs 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 cmanchester@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f24bab080b
Allow link.exe to keep linking the stylo tests after rust-lang/rust#54451. r=simonsapin
https://hg.mozilla.org/integration/mozilla-inbound/rev/677bcec279cd
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.