1412486 - stylo: Upgrade bindgen to 0.31

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

Per bug 1412077 comment 5, upgrade bindgen to the latest version should improve the build time of style crate by quite a bit.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Taking this since one contributor is trying to do the Servo change and I promised to help.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

But there is really nothing about Servo change... It has to be verified with a Gecko build.

Does the contributor has a working Stylo build?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

It does not. For now, blockers:

 - https://github.com/rust-lang-nursery/rust-bindgen/pull/1111 (actually not a blocker I guess, but nice)
 - https://github.com/rust-lang-nursery/rust-bindgen/pull/1112
 - https://github.com/rust-lang-nursery/rust-bindgen/pull/1113 (fixed by https://github.com/rust-lang-nursery/rust-bindgen/pull/1114)

After those merge I'll try again, I don't want to remember how to do all the [replace] setup.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Last remaining blocking issue is https://github.com/rust-lang-nursery/rust-bindgen/issues/1118

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I'm pushing a WIP now because I want to pull this from the office tomorrow.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1412486: Update bindgen.

Review commit: https://reviewboard.mozilla.org/r/194362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194362/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1412486: Don't force ComputedValues to derive Debug.

The code that uses it is of dubious utility, fwiw, but some of it is used on
Servo, so I'll kill that separately.

Review commit: https://reviewboard.mozilla.org/r/194538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194538/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1412486: Make the replacement for UniquePtr not have a deleter parameter.

Bindgen now assumes for replaced types that all the template parameters are
used, which makes sense.

We don't use the Deleter, so let's get rid of it.

Review commit: https://reviewboard.mozilla.org/r/194540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194540/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1412486: Unconditionally implement Debug for RefPtr.

Bindgen doesn't know how to derive debug for a Gecko font family list. Hopefully
it doesn't need to.

Review commit: https://reviewboard.mozilla.org/r/194542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194542/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1412486: Update bindgen to 0.31.2.

Review commit: https://reviewboard.mozilla.org/r/194544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194544/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Revendor.

Review commit: https://reviewboard.mozilla.org/r/194546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194546/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1412486: Avoid using deprecated bindgen methods.

This is an automated change.

Review commit: https://reviewboard.mozilla.org/r/194548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194548/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923220 [details]
Bug 1412486: Update bindgen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194362/diff/1-2/

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8923220 [details]
Bug 1412486: Update bindgen.

https://reviewboard.mozilla.org/r/194362/#review199768

Cancel review for wildcards in `rusty-enums`.

::: layout/style/ServoBindings.toml:134
(Diff revision 2)
> +    "nsIDocument_Type",
> +    "nsCSSUnit",
> +    "nsCSSPropertyID",
> +    "nsCSSCounterDesc",
> +    "nsMediaExpression_Range",
> +    "nsMediaFeature.*",

You mean `nsMediaFeature::RangeType` and `nsMediaFeature::ValueType`? I think we should list them out explicitly here. Also it doesn't seem to me `nsMediaFeature::RequirementFlags` should be a rust enum. It should probably be put in `bitfield-enums` section.

::: layout/style/ServoBindings.toml:142
(Diff revision 2)
> +    # FIXME: use proper namespaces and such when
> +    # https://github.com/rust-lang-nursery/rust-bindgen/issues/1125
> +    # is fixed.
> +    "nsStyleImageLayers_LayerType",
> +    "nsStyleImageOrientation_Angles",
> +    "nsTimingFunction.*",

Same here, I think we should have `nsTimingFunction::Type` and `nsTimingFunction::Keyword` listed explicitly.

::: layout/style/ServoBindings.toml:143
(Diff revision 2)
> +    # https://github.com/rust-lang-nursery/rust-bindgen/issues/1125
> +    # is fixed.
> +    "nsStyleImageLayers_LayerType",
> +    "nsStyleImageOrientation_Angles",
> +    "nsTimingFunction.*",
> +    ".*ServoElementSnapshot.*",

This is really unclear to me what is covered here... `ServoElementSnapshotFlags` should probably be in `bitfield-enums`, and I don't find anything else.

::: layout/style/ServoBindings.toml:144
(Diff revision 2)
> +    # is fixed.
> +    "nsStyleImageLayers_LayerType",
> +    "nsStyleImageOrientation_Angles",
> +    "nsTimingFunction.*",
> +    ".*ServoElementSnapshot.*",
> +    ".*Side",

It doesn't seem to me that we have that many `Side` enums... [1] Could you also just list what we need here?

[1] https://searchfox.org/mozilla-central/search?q=enum+.*Side%5Cb&case=true&regexp=true&path=.h%24

::: layout/style/ServoBindings.toml:145
(Diff revision 2)
> +    ".*PlaybackDirection",
> +    ".*FillMode",
> +    ".*HalfCorner",

Why arbitrary prefix? Because of namespace?

::: layout/style/ServoBindings.toml:148
(Diff revision 2)
> +    "Type",
> +    "Flags",

What `Type` and `Flags`?

::: layout/style/ServoBindings.toml:150
(Diff revision 2)
> +    ".*URLMatchingFunction",
> +    ".*Style.*",
> +    ".*ColorID",
> +    ".*BooleanFlag",
> +    ".*CSSPseudoElementType",
> +    ".*CSSPseudoClassType",
> +    ".*FontID",
> +    ".*MatrixTransformOperator",

Well... I'm really not a big fan of using wildcard like this... Unless there are really a large number of things with restrictive common pattern, please just list them out.

::: servo/components/style/build_gecko.rs:390
(Diff revision 2)
>              }
>          }
>  
>          let builder = Builder::get_initial_builder()
>              .enable_cxx_namespaces()
> +            .rustfmt_bindings(false)

If bindgen is messing code further... we probably want to integrate rustfmt at some point... That would require some bootstrap and tooltool change I guess. Probably worth filing a bug for.

::: servo/components/style/build_gecko.rs:426
(Diff revision 2)
> +                                .map(|s| format!("\\s*{}\\s*", s))
> +                                .collect::<Vec<_>>()
> +                                .join("::");
> +
>                  fixups.push(Fixup {
> -                    pat: format!("\\broot::{}\\b", gecko),
> +                    pat: format!("\\broot\\s*::\\s*{}\\b", gecko),

With these many backslashes, it would probably be better to just use raw string literal `r"\broot\s*::\s*::{}\b"?

::: servo/components/style/build_gecko.rs:514
(Diff revision 2)
>              // that has been fixed in clang 4.0+. When we switch people
>              // to libclang 4.0, we can remove this.
>              .handle_table_items("array-types", |builder, item| {
>                  let cpp_type = item["cpp-type"].as_str().unwrap();
>                  let rust_type = item["rust-type"].as_str().unwrap();
> -                builder.hide_type(format!("nsTArrayBorrowed_{}", cpp_type))
> +                builder

Is it because those types are not in whitelist so we don't need to hide them at all?

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8923338 [details]
Bug 1412486: Don't force ComputedValues to derive Debug.

https://reviewboard.mozilla.org/r/194538/#review199774

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8923339 [details]
Bug 1412486: Make the replacement for UniquePtr not have a deleter parameter.

https://reviewboard.mozilla.org/r/194540/#review199776

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8923340 [details]
Bug 1412486: Unconditionally implement Debug for RefPtr.

https://reviewboard.mozilla.org/r/194542/#review199780

::: commit-message-d26df:3
(Diff revision 1)
> +Bindgen doesn't know how to derive debug for a Gecko font family list. Hopefully
> +it doesn't need to.

It doesn't seem to me there is anything particularly hard to derive `Debug` for font family list... Is this a bindgen bug?

::: servo/components/style/gecko_bindings/sugar/refptr.rs
(Diff revision 1)
>  /// Trait for types which can be shared across threads in RefPtr.
>  pub unsafe trait ThreadSafeRefCounted: RefCounted {}
>  
>  /// A custom RefPtr implementation to take into account Drop semantics and
>  /// a bit less-painful memory management.
> -#[derive(Debug)]

Also I think this is really a Rust bug to require `T: Debug` here... Pointers are unconditionally `Debug`. Maybe Rust always requires all type params to be `Debug` to derive `Debug` for a generic type? That restriction doesn't really make sense to me...

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8923341 [details]
Bug 1412486: Update bindgen to 0.31.2.

https://reviewboard.mozilla.org/r/194544/#review199782

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8923341 [details]
Bug 1412486: Update bindgen to 0.31.2.

https://reviewboard.mozilla.org/r/194544/#review199784

::: servo/components/style/Cargo.toml:82
(Diff revision 1)
>  kernel32-sys = "0.2"
>  
>  [build-dependencies]
>  lazy_static = "0.2"
>  log = "0.3"
> -bindgen = { version = "0.29.1", optional = true }
> +bindgen = { version = "0.31.2", optional = true }

Is it possible for us to update bindgen in `js/rust/Cargo.toml` at the same time so that we stop having duplicate bindgens in tree?

Maybe this can be in a separate bug if that would also be some non-trivial work...

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8923343 [details]
Bug 1412486: Avoid using deprecated bindgen methods.

https://reviewboard.mozilla.org/r/194548/#review199788

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923220 [details]
Bug 1412486: Update bindgen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194362/diff/2-3/

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923338 [details]
Bug 1412486: Don't force ComputedValues to derive Debug.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194538/diff/1-2/

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923339 [details]
Bug 1412486: Make the replacement for UniquePtr not have a deleter parameter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194540/diff/1-2/

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923340 [details]
Bug 1412486: Unconditionally implement Debug for RefPtr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194542/diff/1-2/

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923341 [details]
Bug 1412486: Update bindgen to 0.31.2.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194544/diff/1-2/

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923343 [details]
Bug 1412486: Avoid using deprecated bindgen methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194548/diff/1-2/

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923342 [details]
Revendor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194546/diff/1-2/

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8923220 [details]
Bug 1412486: Update bindgen.

https://reviewboard.mozilla.org/r/194362/#review199976

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923339 [details]
Bug 1412486: Make the replacement for UniquePtr not have a deleter parameter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194540/diff/2-3/

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8923220 [details]
Bug 1412486: Update bindgen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194362/diff/3-4/

Comment 31

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46a4ddbc0541
Make the replacement for UniquePtr not have a deleter parameter. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/b21ef5bb00d3
Update bindgen. r=xidorn

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d480e8818314
Revendor rust dependencies. r=me

Comment 34

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/46a4ddbc0541
https://hg.mozilla.org/mozilla-central/rev/b21ef5bb00d3
https://hg.mozilla.org/mozilla-central/rev/d480e8818314