Closed
Bug 1356150
Opened 8 years ago
Closed 8 years ago
stylo: can't compile due to bad bindgen?
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
WORKSFORME
Tracking | Status | |
---|---|---|
firefox55 | --- | affected |
People
(Reporter: n.nethercote, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
On my Ubuntu Linux64 box I get lots of compile errors from generated code. See the attached file.
I'm using clang 3.9. Here's my mozconfig:
> ac_add_options --with-ccache
> ac_add_options --enable-stylo
> ac_add_options --enable-dmd
> ac_add_options --enable-optimize='-O1'
>
> mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/o64sty
> mk_add_options MOZ_MAKE_FLAGS="-j8"
> mk_add_options AUTOCLOBBER=1
>
> export CC=clang-3.9
> export CXX=clang++-3.9
> export LLVM_CONFIG=llvm-config-3.9
I will attach the generated file in a moment.
![]() |
Reporter | |
Comment 1•8 years ago
|
||
fitzgen, any ideas what's going wrong?
Flags: needinfo?(nfitzgerald)
Comment 3•8 years ago
|
||
I don't know anything concrete from looking at those logs, but I do know that Stylo is using an old bindgen version. At a glance, those errors all look vaguely template-y related, and template stuff received an overhaul in newer bindgen. In fact, we're now building bindings for stylo as a sanity/smoke test in bindgen's CI, so I suspect a bindgen upgrade will fix the issues you are seeing. If you want, you can try with a local bindgen checkout. If you choose to go down this route, can you verify that stylo binding generation works with bindgen master? To use a local bindgen, add this to the bottom of Servo's top level Cargo.toml: [replace] "bindgen:0.22.0": { path = "/path/to/bindgen/checkout" } Follow https://github.com/servo/servo/pull/16392 for Stylo's bindgen version bump.
Flags: needinfo?(nfitzgerald)
Updated•8 years ago
|
Blocks: stylo-tooling
Priority: -- → P2
![]() |
Reporter | |
Comment 4•8 years ago
|
||
> Could you delete your obj folder and rebuild again? No change. Same errors. > If you want, you can try with a local bindgen checkout. If you choose to go > down this route, can you verify that stylo binding generation works with > bindgen master? To use a local bindgen, add this to the bottom of Servo's > top level Cargo.toml: > > [replace] > "bindgen:0.22.0": { path = "/path/to/bindgen/checkout" } I tried that -- modifying servo/Cargo.toml within my mozilla-inbound clone -- and got the same error. I also tried "bindgen:0.23.0" and got the same error. In both cases I got this line just before the errors: > 4:13.67 Compiling bindgen v0.22.0 Is that expected? I'm not convinced my local copy of rust-bindgen is being used. How can I tell?
Flags: needinfo?(nfitzgerald)
Comment 5•8 years ago
|
||
I don't think your local copy is being used. Could you attach also the structs files?
Flags: needinfo?(n.nethercote)
![]() |
Reporter | |
Comment 6•8 years ago
|
||
I tried adding a deliberate syntax error to src/main.rs in rust-bindgen, and it had no effect. I then did likewise with servo/Cargo.toml, again to no effect. So I have no idea what's going on.
> Could you attach also the structs files?
Which files? I've already attached structs_release.rs.
Flags: needinfo?(n.nethercote)
Comment 7•8 years ago
|
||
Whoops, sorry, missed the second attachment. Seems we're generating basic_string without using it anywhere, which seems dumb. First of all, does this patch fix your build? diff --git a/servo/components/style/build_gecko.rs b/servo/components/style/build_gecko.rs index 07ed63298084..4c10d7bd92bd 100644 --- a/servo/components/style/build_gecko.rs +++ b/servo/components/style/build_gecko.rs @@ -454,6 +454,8 @@ mod bindings { "std::namespace::atomic___base", "std::atomic__My_base", "std::atomic", "std::atomic___base", + "std::string", + "std::basic_string", "mozilla::gfx::.*", "FallibleTArray", "mozilla::dom::Sequence", Second, the "replaces" thing will only work to debug bindgen if you are doing: ./mach build-geckolib --with-gecko /path/to/gecko/checkout/objdir/dist From the servo dir. I'm not sure what the right path is for [replace] in a Gecko build.
![]() |
Reporter | |
Comment 8•8 years ago
|
||
> Seems we're generating basic_string without using it anywhere, which seems > dumb. First of all, does this patch fix your build? It's a lot closer to working. I'm down to two errors now: > error[E0091]: type parameter `T` is unused > --> /home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-3490280569f9f1de/out/gecko/structs_release.rs:5887:36 > | > 5887 | pub type conditional_t<T, F> = (); > | ^ unused type parameter > > error[E0091]: type parameter `F` is unused > --> /home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-3490280569f9f1de/out/gecko/structs_release.rs:5887:39 > | > 5887 | pub type conditional_t<T, F> = (); > | ^ unused type parameter Seems like the kind of thing that could be a warning rather than an error?
Comment 9•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #8) > > Seems we're generating basic_string without using it anywhere, which seems > > dumb. First of all, does this patch fix your build? > > It's a lot closer to working. I'm down to two errors now: Nice! Adding mozilla::span_details::conditional_t to the same list should make it work. This should be fixed by the bindgen update, but that also fails for me and :heycam, so we need to work a bit more on that. > Seems like the kind of thing that could be a warning rather than an error? Ask that to the rustc devs, that's been a bindgen pain point for ages :)
Comment 10•8 years ago
|
||
I think Emilio responded to everything I was ni'd for. Will dig into the bindgen update issues from the servo PR.
Flags: needinfo?(nfitzgerald)
Updated•8 years ago
|
Blocks: stylo-nightly-build
![]() |
Reporter | |
Comment 11•8 years ago
|
||
I now get *tons* of warnings like this:
> error: use of extern static requires unsafe function or block (error E0133)
> --> /home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-3490280569f9f1de/out/properties.rs:115488:12
> |
> 115488 | if structs::root::mozilla::SERVO_PREF_ENABLED_background_color {
> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> |
> = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
> = note: for more information, see issue #36247 <https://github.com/rust-lang/rust/issues/35112>
> note: lint level defined here
> --> /home/njn/moz/mi2/servo/components/style/lib.rs:26:9
> |
> 26 | #![deny(warnings)]
> | ^^^^^^^^
>
> error: use of extern static requires unsafe function or block (error E0133)
> --> /home/njn/moz/mi2/o64sty/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-3490280569f9f1de/out/properties.rs:115512:12
> |
> 115512 | if structs::root::mozilla::SERVO_PREF_ENABLED_background_image {
> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> |
> = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
> = note: for more information, see issue #36247 <https://github.com/rust-lang/rust/issues/35112>
fitgzen, any ideas?
Flags: needinfo?(nfitzgerald)
Comment 12•8 years ago
|
||
Are you using clang 3.8? We used to support it, but people have started using the constant evaluation features of clang 3.9+. If so, you need to update your libclang version to at least 3.9
Flags: needinfo?(nfitzgerald)
![]() |
Reporter | |
Comment 13•8 years ago
|
||
As per comment 0, I'm using clang 3.9. Not clang 4.0, not clang 3.8. clang 3.9.
Comment 14•8 years ago
|
||
Can you clone bindgen, run the following, and paste the result here? (Or, in particular, whether the output is different from https://github.com/servo/rust-bindgen/blob/master/tests/expectations/tests/constant-evaluate.rs). $ cargo build $ ./target/debug/bindgen tests/headers/constant-evaluate.h If so, can you post here the output of: $ RUST_LOG=bindgen ./target/debug/bindgen tests/headers/constant-evaluate.h
![]() |
Reporter | |
Comment 15•8 years ago
|
||
(In reply to Emilio Cobos Γlvarez [:emilio] from comment #14) > Can you clone bindgen, run the following, and paste the result here? (Or, in > particular, whether the output is different from > https://github.com/servo/rust-bindgen/blob/master/tests/expectations/tests/ > constant-evaluate.rs). It is different. > $ cargo build > $ ./target/debug/bindgen tests/headers/constant-evaluate.h > > If so, can you post here the output of: > > $ RUST_LOG=bindgen ./target/debug/bindgen tests/headers/constant-evaluate.h Output (both stdout and stderr) is attached.
![]() |
Reporter | |
Comment 16•8 years ago
|
||
> Output (both stdout and stderr) is attached. Wait, that's using clang 3.8, which is my default |clang|. But when I build stylo I'm specifying |clang-3.9| via the mozconfig, as per comment 0.
Comment 17•8 years ago
|
||
Hmm, so it seems to be that bindgen is still using libclang 3.8 for the stylo build... So, we call into clang_sys::load(), which ends up calling this function: https://github.com/KyleMayes/clang-sys/blob/master/build.rs#L126 That looks first into LIBCLANG_PATH, then runs llvm-config. I assume this isn't bug 1355464 because it's Linux, so the LIBCLANG_PATH environment variable should still be there. Does it happen to be the case that LIBCLANG_PATH is the same for libclang 3.8 and 3.9? That could explain it I guess.
Comment 18•8 years ago
|
||
(In reply to Emilio Cobos Γlvarez [:emilio] from comment #17) > That looks first into LIBCLANG_PATH, then runs llvm-config. I assume this > isn't bug 1355464 because it's Linux, so the LIBCLANG_PATH environment > variable should still be there. Does it happen to be the case that > LIBCLANG_PATH is the same for libclang 3.8 and 3.9? Nick, is your LIBCLANG_PATH the same for libclang 3.8 and 3.9? configure should now require exactly 3.9 (bug 1357889) or 5.0+ (bug 1359508).
Flags: needinfo?(n.nethercote)
Comment 19•8 years ago
|
||
Nick says he is going to try making clang 3.9 the system default on his Linux machine. If bindgen works, then it seems like configure or bindgen is somehow not honoring his mozconfig settings to use clang 3.9 when 3.8 is his system default. https://askubuntu.com/questions/907873/how-to-set-clang-3-9-as-the-default-in-zesty/907886#907886
![]() |
Reporter | |
Comment 20•8 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #19) > Nick says he is going to try making clang 3.9 the system default on his > Linux machine. If bindgen works, then it seems like configure or bindgen is > somehow not honoring his mozconfig settings to use clang 3.9 when 3.8 is his > system default. It didn't work. I'm getting the same old errors.
Flags: needinfo?(n.nethercote)
![]() |
Reporter | |
Comment 22•8 years ago
|
||
Finally got it working with help from Emilio. The fix was to add a symlink from libclang.so to libclang.so.1 in /usr/lib/llvm-3.9/lib/. Emilio said he will land a fix to clang-sys to address this issue.
Comment 23•8 years ago
|
||
Emilio's fix has been merged upstream and released in clang-sys versions v0.14.1 and v0.15.2. I filed servo and rust-bindgen bugs about upgrading to clang-sys v0.14.1 and v0.15.2, respectively: https://github.com/servo/servo/issues/16653 https://github.com/servo/rust-bindgen/issues/671
![]() |
Reporter | |
Comment 24•8 years ago
|
||
Thanks for the update, cpeterson. I think we can declare victory now.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•