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.

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

(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)

(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.

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.

(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.

(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!

(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.

(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.

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

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

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

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.

Awesome, thanks Nathan!

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?

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.

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.

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.

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.

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.

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 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 |//|).

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.

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.

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 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 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 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 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 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 on attachment 8868938 [details] Bug 1336540 part 2 - Vendor toml. https://reviewboard.mozilla.org/r/140546/#review144174

Servo side: servo/servo#16941

Pushed by xquan@mozilla.com: 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

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

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.

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.

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee8da8ae8fb4 Move config info from build_gecko.rs to a toml file in gecko. r=emilio