1336540 - the configuration information build_gecko.rs should not be in servo/servo

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P3)

Description

[Bobby Holley (:bholley)]

If somebody shuffles/renames common declarations or headers on m-c, they may need to alter our bindgen config to avoid bustage. If the bindgen config lives in servo/servo, then the developer is suddenly making a change to the shared servo code, which means that (in the long term) their change will need to land on servo-integration instead of mozilla-inbound.

This is suboptimal. We don't get any value from having the script in servo/servo, since it requires a gecko objdir to do anything meaningful. So we should find some way to hoist it (or at least the parts that people will need to twiddle) into toolkit/library/rust or similar.

Comment 1

[Bobby Holley (:bholley)]

Nathan, what do you think is the best way to structure this?

Comment 2

[Bobby Holley (:bholley)]

(It would also make sense to be able to bump our bindgen version without needing to round-trip through s/s. Not sure if that's the case already)

Comment 3

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> (It would also make sense to be able to bump our bindgen version without
> needing to round-trip through s/s. Not sure if that's the case already)

Given that https://github.com/servo/servo/pull/15372 is in servo/servo, it appears that this is not already the case.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Upgrading non-breaking bindgen version doesn't need to roundtrip through s/s, we can always do so via cargo update in gecko directly. Breaking version change is trickier, unless s/s gives us more leeway to specify a wider range of bindgen version, which is probably also fine.

For moving build_gecko.rs, we can probably use imclude! to add that, as MOZ_DIST is always provided when build-time bindgen is enabled.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> > (It would also make sense to be able to bump our bindgen version without
> > needing to round-trip through s/s. Not sure if that's the case already)
> 
> Given that https://github.com/servo/servo/pull/15372 is in servo/servo, it
> appears that this is not already the case.

That's mainly for updating the generated binding files, which we don't need with build-time bindgen... It depends on whether we want to keep those generated files in-tree.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> Upgrading non-breaking bindgen version doesn't need to roundtrip through
> s/s, we can always do so via cargo update in gecko directly. Breaking
> version change is trickier, unless s/s gives us more leeway to specify a
> wider range of bindgen version, which is probably also fine.

Yeah, it seems like s/s itself shouldn't really care about the version of bindgen.
 
> For moving build_gecko.rs, we can probably use imclude! to add that, as
> MOZ_DIST is always provided when build-time bindgen is enabled.

That's a nice idea!

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> For moving build_gecko.rs, we can probably use imclude! to add that, as
> MOZ_DIST is always provided when build-time bindgen is enabled.

So we'd:

1. Move build_gecko.rs to m-c;
2. Write:

include!(env!("MOZ_DIST") + "/build_gecko.rs"); // or whatever

in stylo's bindgen?  Can we even do that kind of computation prior to macroexpansion time?  Or would we have to wrap it up in a second macro?

Alternatively, we may want to implement some sort of build system facility like:

CARGO_ENVIRONMENT_VARS["..."] = "value"

because I'm sure we'll want to pass more things to Cargo in the future and because there's no reason we should have to copy files like a config script to MOZ_DIST just so Cargo can get at it conveniently.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> > For moving build_gecko.rs, we can probably use imclude! to add that, as
> > MOZ_DIST is always provided when build-time bindgen is enabled.
> 
> So we'd:
> 
> 1. Move build_gecko.rs to m-c;
> 2. Write:
> 
> include!(env!("MOZ_DIST") + "/build_gecko.rs"); // or whatever
> 
> in stylo's bindgen?  Can we even do that kind of computation prior to
> macroexpansion time?  Or would we have to wrap it up in a second macro?

Yes. I don't quite understand your concern here. Why do we need to do this prior to macro expansion? I think this would work, just like C++'s #include. Is there any reason we need to wrap it up in a second macro?

build_gecko.rs currently includes both bindgen path and non-bindgen path. We may need to separate them, and move only the bindgen part into gecko. But I don't see there is anything else we need to do in source code level.

> Alternatively, we may want to implement some sort of build system facility
> like:
> 
> CARGO_ENVIRONMENT_VARS["..."] = "value"
> 
> because I'm sure we'll want to pass more things to Cargo in the future and
> because there's no reason we should have to copy files like a config script
> to MOZ_DIST just so Cargo can get at it conveniently.

I guess passing MOZ_TOPSRCDIR or something similar to Cargo in addition to MOZ_DIST would be enough for now. With that, build scripts would be able to access the source code directly, and then we don't need to copy files from source to dist just for them.

Comment 9

[Bobby Holley (:bholley)]

Nathan, do you have cycles for this anytime in the coming weeks?

Comment 10

[Nathan Froyd [:froydnj]]

Yes, I can take care of this...maybe later this week, more probably early next.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 1 - export MOZ_TOPSRCDIR to cargo

We'll need this so we can use it to include! files on the Servo side.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 2 - port over build_gecko.rs from servo/servo

When the Servo-side changes for this land, this will magically start
being used.

I will take care of landing the appropriate Servo-side change once
these patches have been approved and merged to central.

Comment 13

[Bobby Holley (:bholley)]

Awesome, thanks Nathan!

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Probably better put the file in layout/style? It seems to me toolkit/library/rust is share by various rust-based components, and build_gecko is a rather unclear name there. Alternatively, we can probably rename it to build_stylo rather than move it. What do you think?

Comment 15

[Bobby Holley (:bholley)]

If the file is moving to m-c I think it should be called build_servo_bindings.rs. Eventually we may want to do more than stylo with these bindings (and I expect the 'stylo' name to die after the project is done).

I think toolkit/library/rust is probably the right place for it.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

That is simply not true as far as we include it inside servo/component/style/build.rs. How can we make it cover more than stylo things if we have to have individual build scripts of different components invoke it?

If we can split the generated bindings into a separate crate, that would probably be a different story, though.

Comment 17

[Bobby Holley (:bholley)]

That's fair - I guess the bindings will probably need to be per-crate for the foreseeable future, given the coherency issues we ran into trying to keep them as a separate crate.

I guess build_servo_bindings.rs in layout/style is reasonable, or build_servo_style_bindings.rs in toolkit/library/rust.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

And I wonder what would happen to the Servo side, since I believe we still don't want the CI to depend on Gecko build?

Probably we should only move the part with bindgen enabled, and leave the file-copy part in Servo.

Comment 19

[Bobby Holley (:bholley)]

I think the Servo side is still going to need a copy of the checked-in bindings for a while.

IMO, the functionality that does that in build_gecko.rs should be replaced with a gecko mach command to copy the files.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Also I have a feeling that, rather than moving the whole build_gecko.rs into Gecko, we could probably instead split that file into code and config, and then move the config file into Gecko, with the code left in Servo.

There is an advantage that, we wouldn't need to rebuild the build script every time when we just want to add a new item somewhere. (Building the build script could be expensive... especially on Windows, because of MSVC's link-time optimization.)

We can use toml for the config file, I guess... Do you think it makes sense?

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851059 [details] [diff] [review]
part 2 - port over build_gecko.rs from servo/servo

Review of attachment 8851059 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine if we take this approach. FWIW I'd prefer the split into config and build system approach.

I have a question though (apart from the rest of the outstanding questions in the bug). With this, is there an easy way to re-check the bindings inside servo/servo? Right now the way to do it is (from the servo checkout) running ./mach build-geckolib --with-gecko path/to/stylo/objdir/dist. This will convert that into being a manual process (having to find the latest generated file by hand), is that right?

If so, that may be acceptable for now, but we still should have a bug on file to make that a bit more pleasurable while we still need the generated bindings checked in at least.

::: toolkit/library/rust/build_gecko.rs
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Generating bindings for Stylo.  This file is intended to be include!'d

nit: Make this a doc comment (using |//!| instead of just |//|).

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Updating generated binding files is... especially hard for Windows, as it generally generates very different binding files than Mac and Linux.

I hope we can have try server upload the generated files as artifacts, so that we use the files from a consistent source rather than machine of random people. Our current approach could make binding files change significantly from revision to revision, which makes it hard to review / audit.

Comment 23

[Bobby Holley (:bholley)]

FWIW, I don't think we need to review/audit the checked-in bindings. They're never used in actual stylo builds, so the only real requirement we have for them is that they compile.

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

It's true that we don't need to review/audit in general, but it would be helpful when you have merge conflict. The current workflow changes the whole binding files significantly among commits, and when you see a merge conflict, it is completely unclear what should be done.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1336540 part 1 - Move config info from build_gecko.rs to a toml file in gecko.

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

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1336540 part 2 - Vendor toml.

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

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1336540 part 3 - Remove --with-gecko from build-geckolib because it is not usable.

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

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8868937 [details]
Bug 1336540 part 1 - Move config info from build_gecko.rs to a toml file in gecko.

https://reviewboard.mozilla.org/r/140544/#review144014

::: layout/style/ServoBindings.toml:45
(Diff revision 1)
> +"arch=x86" = ["--target=i686-pc-win32"]
> +"arch=x86_64" = ["--target=x86_64-pc-win32"]
> +
> +[structs]
> +headers = [
> +    "nsCSSPseudoClasses.h", # servo/rust-bindgen#599

This shouldn't be needed now, right?

::: servo/components/style/build_gecko.rs:84
(Diff revision 1)
> +                let mut reason = String::from("Failed to parse config file:");
> +                for err in parser.errors.iter() {
> +                    let parsed = &contents[..err.lo];
> +                    write!(&mut reason, "\n* line {} column {}: {}",
> +                           parsed.lines().count(),
> +                           parsed.lines().last().map(|l| l.len()).unwrap_or(0),

nit: map_or

::: servo/components/style/build_gecko.rs:304
(Diff revision 1)
>                   .expect(&format!("Unrecognized line in ServoArcTypeList.h: '{}'", line))
>                   .get(1).unwrap().as_str().to_string())
>              .collect()
>      }
>  
> +    struct BuilderWithConfig<'a> {

I wonder if this would be a worthwile addition to bindgen itself, wdyt?

Presumably not worth to block this on upstreaming it, but could you file an issue pointing to this bug?

::: servo/components/style/build_gecko.rs:495
(Diff revision 1)
> -            }
> +                }
> -        }
> -        for &ArrayType { cpp_type, rust_type } in array_types.iter() {
> -            builder = builder.hide_type(format!("nsTArrayBorrowed_{}", cpp_type))
> -                .raw_line(format!("pub type nsTArrayBorrowed_{}<'a> = &'a mut ::gecko_bindings::structs::nsTArray<{}>;",
> +                builder
> +            })
> +            .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();

nit: Perhaps it's worth to leave a TODO: remove once we switch people to libclang 4.0.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8868939 [details]
Bug 1336540 part 3 - Remove --with-gecko from build-geckolib because it is not usable.

https://reviewboard.mozilla.org/r/140548/#review144020

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8868937 [details]
Bug 1336540 part 1 - Move config info from build_gecko.rs to a toml file in gecko.

https://reviewboard.mozilla.org/r/140544/#review144014

> I wonder if this would be a worthwile addition to bindgen itself, wdyt?
> 
> Presumably not worth to block this on upstreaming it, but could you file an issue pointing to this bug?

I have no idea how can we add this to bindgen itself... I mean, there is lots of code here pretty specific to stylo's usage. Perhaps you mean the `BuilderWithConfig` and its helper functions? That... maybe... I don't know...

One problem is that, toml is now 0.4.x on crates.io, while Servo's dependency is still on 0.2.x, and that's why I chose to use toml 0.2.x. Even if you add toml support to bindgen, I guess we would want the latest version of toml, which also means stylo cannot benefit from that very soon.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8868937 [details]
Bug 1336540 part 1 - Move config info from build_gecko.rs to a toml file in gecko.

https://reviewboard.mozilla.org/r/140544/#review144014

> I have no idea how can we add this to bindgen itself... I mean, there is lots of code here pretty specific to stylo's usage. Perhaps you mean the `BuilderWithConfig` and its helper functions? That... maybe... I don't know...
> 
> One problem is that, toml is now 0.4.x on crates.io, while Servo's dependency is still on 0.2.x, and that's why I chose to use toml 0.2.x. Even if you add toml support to bindgen, I guess we would want the latest version of toml, which also means stylo cannot benefit from that very soon.

Yeah, I meant the `BuilderWithConfig`... But you make a good point about the toml flags... nvm, then

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8868937 [details]
Bug 1336540 part 1 - Move config info from build_gecko.rs to a toml file in gecko.

I think emilio's review is probably sufficient here. Let me know if there's anything specific you want my review on.

Comment 33

[Nathan Froyd [:froydnj]]

Comment on attachment 8868938 [details]
Bug 1336540 part 2 - Vendor toml.

https://reviewboard.mozilla.org/r/140546/#review144174

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8868937 [details]
Bug 1336540 part 1 - Move config info from build_gecko.rs to a toml file in gecko.

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

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8868938 [details]
Bug 1336540 part 2 - Vendor toml.

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

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Servo side: servo/servo#16941

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8868937 [details]
Bug 1336540 part 1 - Move config info from build_gecko.rs to a toml file in gecko.

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

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8868938 [details]
Bug 1336540 part 2 - Vendor toml.

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

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8868937 [details]
Bug 1336540 part 1 - Move config info from build_gecko.rs to a toml file in gecko.

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

Comment 40

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8868938 [details]
Bug 1336540 part 2 - Vendor toml.

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

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42feca9f0e17
part 1 - Move config info from build_gecko.rs to a toml file in gecko. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1bef3928a127
part 2 - Vendor toml. r=froydnj

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Gecko side landed too soon and caused bustage, backed out in https://hg.mozilla.org/integration/autoland/rev/14c54e401230b1dfe8008925cf7f4a22cb609d6d

Comment 43

[Xidorn Quan [:xidorn] UTC+10]

Thanks for backing that out. I thought we can land the two parts independently but when I checked the change again, I found I was wrong...

I'll land it when Servo side gets merged.

Comment 44

[Mozilla Autoland]

We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee8da8ae8fb4
Move config info from build_gecko.rs to a toml file in gecko. r=emilio

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee8da8ae8fb4