Closed Bug 1432153 Opened 2 years ago Closed 2 years ago

Make the generated stylo bindings be formatted.

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

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 :)
Summary: Make the generated bindings be formatted. → Make the generated stylo bindings be formatted.
Thanks Emilio!
I think I got something to work, waiting on a build on all platforms just to double-check.

It was surprisingly painless!
Assignee: nobody → emilio
Depends on: 1432563
Please make this optional.
(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).
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 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?
Attachment #8945983 - Flags: review?(xidorn+moz) → review+
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.
Attachment #8945984 - Flags: review?(xidorn+moz) → review+
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 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?
(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 :)
Depends on: 1430927
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?
(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.
(And the bindings will be formatted regardless)
Product: Core → Firefox Build System
https://hg.mozilla.org/mozilla-central/rev/0e06932d93c2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.