1432153 - Make the generated stylo bindings be formatted.

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

As discussed on IRC last week, it'd be nicer for them to be formatted using rustfmt, since that way we can run 3-way merges on them when landing to servo, which prevents most of the pain of having updated bindings.

I asked Ted about this today, and it doesn't look very terrible:

13:28 <emilio> ted: do you know if our CI machines install rust using rustup? Or do they use something completely different?
14:47 <@ted> emilio: we have toolchain jobs that just take the rust binary release packages and repack them, mostly to add other targets
14:48 <@ted> https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/toolchain/linux.yml#337
14:48 <@ted> https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/toolchain/windows.yml#100
14:49 <@ted> https://dxr.mozilla.org/mozilla-central/source/taskcluster/scripts/misc/repack_rust.py is the script those run
14:49 <emilio> ted: I see... we were thinking of adding rustfmt support in the generated bindings on Linux64-opt, so we could have consistent bindings... Bindgen would do that automatically, but the rustfmt binary should be there of course
14:49 <@ted> emilio: so you'd like to have the rustfmt binary available for the build?
14:49 <emilio> ted: yes
14:50 <@ted> you could do that one of two ways then: either build it during the rust toolchain job and stick it in the rust package, or write another toolchain job that builds it and use that
14:50 <@ted> if you look in those toolchain/*.yml files you can see we build all kinds of things for builds to use
14:51 <@ted> https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/toolchain/linux.yml#434
14:51 <@ted> is where we build sccache
14:52 — emilio looks at build-sccache.sh
14:52 <emilio> ted: alright, that sounds doable, thank you very much!
14:54 <@ted> yw!
14:54 <@ted> then you need to make the build jobs depend on it, which is easy
14:54 <@ted> https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/build/linux.yml#23
14:55 <@ted> you just stick it in the toolchains: bit in the build task definitions
14:55 <@ted> and the build will download it before building
14:55 <@ted> it'll get unpacked directly in $topsrcdir, so most of our toolchain packages build tarballs with all the files in a subdirectory, so we can use paths like $topsrcdir/clang/bin/clang

This is blocked on the next bindgen update, which will fix the rustfmt invocation. I already have a patch for that though :)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Permalinks for the links above:

 * https://dxr.mozilla.org/mozilla-central/rev/9fe69ff0762da191fbd2b9fc0718e96f1ab4137e/taskcluster/ci/toolchain/linux.yml#337
 * https://dxr.mozilla.org/mozilla-central/rev/9fe69ff0762da191fbd2b9fc0718e96f1ab4137e/taskcluster/ci/toolchain/windows.yml#100
 * https://dxr.mozilla.org/mozilla-central/rev/9fe69ff0762da191fbd2b9fc0718e96f1ab4137e/taskcluster/ci/toolchain/linux.yml#434
 * https://dxr.mozilla.org/mozilla-central/rev/9fe69ff0762da191fbd2b9fc0718e96f1ab4137e/taskcluster/ci/build/linux.yml#23

I think I'll only need to touch the Linux bits. And we may want to have a configuration option to make bindgen look at something that isn't $PATH, I guess, per the last few comments.

Comment 2

[Bobby Holley (:bholley)]

Thanks Emilio!

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I think I got something to work, waiting on a build on all platforms just to double-check.

It was surprisingly painless!

Comment 4

[Mike Hommey [:glandium]]

Please make this optional.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mike Hommey [:glandium] from comment #4)
> Please make this optional.

Yeah, my plan is to make it run on automation only, and since it broke the build for a couple people with old rustfmt I had already disabled it for now (https://github.com/servo/servo/pull/19845).

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Ok, so what I got works, though it hits an old, known rustfmt bug:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e542aec8fb8e3492657939bb7489966c50882a8a

I'll post the patches for reference and to get them reviewed, though we can't land them probably until the next rust compiler update.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1432153: Update bindgen (again).

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1432153: Rustfmt bindings on automation, and locally under an env variable (off by default).

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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8945983 [details]
Bug 1432153: Update bindgen (again).

https://reviewboard.mozilla.org/r/216040/#review222230

::: toolkit/library/rust/Cargo.lock:1776
(Diff revision 1)
>  "checksum quote 0.3.15 (registry+https://github.com/rust-lang/crates.io-index)" = "7a6e920b65c65f10b2ae65c831a81a073a89edd28c7cce89475bff467ab4167a"
> +"checksum quote 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)" = "1eca14c727ad12702eb4b6bfb5a232287dcf8385cb8ca83a3eeaf6519c44c408"

It's unfortunate to see duplicate quote and unicode-xid crate. Could you file a bug or Servo issue for updating them to the new ones?

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8945984 [details]
Bug 1432153: Rustfmt bindings on automation, and locally under an env variable (off by default).

https://reviewboard.mozilla.org/r/216042/#review222236

r=me with the nit addressed.

::: servo/components/style/build_gecko.rs:210
(Diff revision 1)
> +                env::var_os("TOOLTOOL_DIR").or_else(|| {
> +                    env::var_os("MOZ_SRC")
> +                })

It doesn't seem to me that it is necessary to check both `TOOLTOOL_DIR` and `MOZ_SRC`. Although ted mentioned that the files would get extracted into the top src directory, it seems that `TOOLTOOL_DIR` is also configured to be the same directory.

I think it makes more sense to keep consistent with other stuff that solely uses `TOOLTOOL_DIR`.

If the `MOZ_SRC` bit is necessary, please add some comment on why is that.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945984 [details]
Bug 1432153: Rustfmt bindings on automation, and locally under an env variable (off by default).

https://reviewboard.mozilla.org/r/216042/#review222236

> It doesn't seem to me that it is necessary to check both `TOOLTOOL_DIR` and `MOZ_SRC`. Although ted mentioned that the files would get extracted into the top src directory, it seems that `TOOLTOOL_DIR` is also configured to be the same directory.
> 
> I think it makes more sense to keep consistent with other stuff that solely uses `TOOLTOOL_DIR`.
> 
> If the `MOZ_SRC` bit is necessary, please add some comment on why is that.

That is what our mozconfigs do:

   TOOLTOOL_DIR=${TOOLTOOL_DIR:-$topsrcdir}

So I'll keep it for now with a comment.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8945984 [details]
Bug 1432153: Rustfmt bindings on automation, and locally under an env variable (off by default).

https://reviewboard.mozilla.org/r/216042/#review222236

> That is what our mozconfigs do:
> 
>    TOOLTOOL_DIR=${TOOLTOOL_DIR:-$topsrcdir}
> 
> So I'll keep it for now with a comment.

Doesn't that mean `TOOLTOOL_DIR` would always be set on automation?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #12)
> Comment on attachment 8945984 [details]
> Bug 1432153: Rustfmt bindings on automation, and locally under an env
> variable (off by default).
> 
> https://reviewboard.mozilla.org/r/216042/#review222236
> 
> > That is what our mozconfigs do:
> > 
> >    TOOLTOOL_DIR=${TOOLTOOL_DIR:-$topsrcdir}
> > 
> > So I'll keep it for now with a comment.
> 
> Doesn't that mean `TOOLTOOL_DIR` would always be set on automation?

Hmm, maybe, yeah... Good point. Then I'll _remove_ it with a comment :)

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Does this actually work? When I tried to rustfmt the generated structs.rs directly, I got lots of errors like:
> error: line exceeded maximum width (maximum: 100, found: 141)

We may need some configuration file to have it work?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> Does this actually work? When I tried to rustfmt the generated structs.rs
> directly, I got lots of errors like:
> > error: line exceeded maximum width (maximum: 100, found: 141)
> 
> We may need some configuration file to have it work?

Bindgen eats those errors, so the build doesn't fail.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(And the bindings will be formatted regardless)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e06932d93c2
Update bindgen. r=xidorn

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0e06932d93c2