Closed
Bug 1512271
Opened 6 years ago
Closed 6 years ago
Upgrade to rustc 1.31
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
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)
(deleted),
patch
|
SimonSapin
:
review+
chmanchester
:
feedback+
|
Details | Diff | Splinter Review |
The release is tomorrow, December 6th, however we're already in the soft freeze, so this will wait until after the merge.
Assignee | ||
Comment 1•6 years ago
|
||
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)
Comment 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
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
Comment 4•6 years ago
|
||
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)
Comment 5•6 years ago
|
||
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?
Updated•6 years ago
|
Attachment #9030444 -
Flags: feedback? → feedback?(cmanchester)
Comment 6•6 years ago
|
||
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+
Assignee | ||
Comment 7•6 years ago
|
||
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+
Assignee | ||
Comment 8•6 years ago
|
||
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
Comment 10•6 years ago
|
||
bugherder |
Updated•5 years ago
|
Attachment #9030547 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•