1495669 - Share bindgen flags globally

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

bindgen requires all sorts of per-platform, per-architecture, per-compiler flags to run correctly. For Servo, there's a unique toml configuration file containing all these flags (ServoBindings.toml); for Cranelift I've put them all directly in the build.rs file because there were not so many of them. But this is very bad: in practice, the flags are repeated, and can be out of sync (while they ought to be commonized); see for instance bug 1495610.

We should try to reuse the flags for all the projects using bindgen. There are several ways to approach this:
- have a configuration file à la ServoBindings.toml file, but only with the bindgen flags, not the structures/functions to bind etc. This would mean also share the Rust code that parses this file. If we go down this road, we should also put this file under a more global directory, so that the JS tarball can include it and refer to it.
- have the build system set all these flags for us, and pass them as part of BINDGEN_CFLAGS: https://searchfox.org/mozilla-central/source/build/moz.configure/toolchain.configure#1119-1129
Android partially uses this already, and both Servo and Cranelift read these supplementary flags anyway.
- have bindgen, or another (new) crate export a function that sets these flags for us. It seems at least *some* bindgen flags would need to be the same from a project to another; not sure it applies to all of these flags, though.

Thoughts?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I agree it'd be nice. ServoBindings.toml grew quite organically and could get some cleanup overall. This could be a good first step for that.

Comment 2

[Nathan Froyd [:froydnj]]

I think either the build system should determine all the necessary flags for bindgen or we should have a crate with the necessary logic (i.e. what's in cranelift's build.rs).  I am slightly partial to the separate crate route, but can certainly see the logic for the build system doing it, since then it can just get rolled into the existing bindgen flag determination.

I am ambivalent on whether cranelift should specify structs/types/enums/etc. in a .toml file as Servo does.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

> I am ambivalent on whether cranelift should specify structs/types/enums/etc. in a .toml file as Servo does.

Right, considering that Cranelift only exports a few entities, I think it'd be overkill.

I just realized that having a single place would also make it easier to add the BINDGEN_SYSTEM_FLAGS, and not have two different substituted files that get this information and add it to the flags their own way.

That being said, it seems the BINDGEN_SYSTEM_FLAGS embeds more than just flags that can be easily inferred in the build.rs file; I am thinking about all the flags that aren't _BINDGEN_CFLAGS here: https://searchfox.org/mozilla-central/source/old-configure.in#3833 . It would probably result in code duplication with the moz configure build system, if we tried to reproduce these flags from build.rs. (should we just switch the entire build system over to build.rs files, y/n)

Also, not sure how we could easily infer these flags for Android builds within a build.rs file: https://searchfox.org/mozilla-central/source/build/moz.configure/android-ndk.configure#322

So it seems two plausible alternatives are:
1. either make a crate that contains what the cranelift's build.rs contains, in addition to all the flags specified in ServoBindings.toml; and keep the BINDGEN_SYSTEM_FLAGS in addition to this.
2. or just try to move everything to the build system, implementing a bigger Python function that would switch on target/os/compiler, etc. infer the bindgen flags, and put them in the _BINDGEN_CFLAGS variable. Then Cranelift/Servo can just read this variable easily and split based on spaces or something like this (there could be a very small Rust crate to help for this).

I think #2 is the cleanest, because it doesn't scatter the configuration in several different places. Does it make sense to go this way?

Comment 4

[(not currently active) Ted Mielczarek]

As someone who works on the build system, #2 sounds nicer to me, certainly! Computing compiler flags is a thing we do a lot of in configure, and having random bits of that hidden in Rust build scripts is not great. If your crate needs to have some sensible defaults for building outside of Firefox that's fine, but when building as part of Firefox we should be passing down flags from configure.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Try-running this patch now.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

A better patch, that seems to work on all platforms (at least 80% work, try running the remaining 20% with a fix that seems to work fine locally).

Asking for review to Emilio for the build_gecko.rs changes, and to a build peer for the moz.configure goo.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9015589 [details] [diff] [review]
fix.patch

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

The build_gecko / ServoBindings stuff looks great to me, thanks for cleaning this stuff up!

::: build/moz.configure/toolchain.configure
@@ +1129,5 @@
> +@depends(host, target, target_is_unix, c_compiler, bindgen_cflags_android)
> +def basic_bindgen_cflags(host, target, is_unix, compiler_info, android_cflags):
> +    args = [
> +        "-x", "c++", "-std=gnu++14", "-fno-sized-deallocation",
> +        "-DTRACING=1", "-DIMPL_LIBXUL", "-DMOZ_STYLO_BINDINGS=1",

Looks like nobody is using MOZ_STYLO_BINDINGS, so you could drop it.

::: servo/components/style/build_gecko.rs
@@ +61,5 @@
>              let path = PathBuf::from(env::var_os("MOZ_TOPOBJDIR").unwrap())
>                  .join("layout/style/bindgen.toml");
>              read_config(&path)
>          };
>          static ref TARGET_INFO: HashMap<String, String> = {

You just removed the only usage of TARGET_INFO, so this can go away.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the first review!

For what it's worth, this passes build on all platforms \o/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=86badcd8bca2d0b278dd3a5652b709f78f846bd2

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Updated per Emilio's comments.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 9016227 [details] [diff] [review]
fix.patch

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

Such build changes, much wow.

I have a small preference for putting all of the bindgen-cflag-stuff from toolchain.configure in bindgen.configure, but whatever you feel like doing here.

::: build/moz.configure/toolchain.configure
@@ +1130,5 @@
> +def basic_bindgen_cflags(host, target, is_unix, compiler_info, android_cflags):
> +    args = [
> +        "-x", "c++", "-std=gnu++14", "-fno-sized-deallocation",
> +        "-DTRACING=1", "-DIMPL_LIBXUL", "-DMOZILLA_INTERNAL_API",
> +        "-DRUST_BINDGEN"

Nit: Please use single-quotes for all the strings in this function.  (Here and many other places below.)

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the review! Agreed it makes sense to move all of this bindgen stuff to bindgen.configure, so I've done that.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5478a63db8
Share bindgen flags globally; r=emilio, r=froydnj

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1f5478a63db8