Closed Bug 1495669 Opened Last year Closed Last year

Share bindgen flags globally

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(1 file, 2 obsolete files)

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?
See Also: → 1495610
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.
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.
> 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?
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.
Attached patch fix.patch (obsolete) — Splinter Review
Try-running this patch now.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attached patch fix.patch (obsolete) — Splinter Review
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.
Attachment #9015537 - Attachment is obsolete: true
Attachment #9015589 - Flags: review?(emilio)
Attachment #9015589 - Flags: review?(core-build-config-reviews)
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.
Attachment #9015589 - Flags: review?(emilio) → review+
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
Attached patch fix.patchSplinter Review
Updated per Emilio's comments.
Attachment #9015589 - Attachment is obsolete: true
Attachment #9015589 - Flags: review?(core-build-config-reviews)
Attachment #9016227 - Flags: review?(core-build-config-reviews)
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.)
Attachment #9016227 - Flags: review?(core-build-config-reviews) → review+
Thanks for the review! Agreed it makes sense to move all of this bindgen stuff to bindgen.configure, so I've done that.
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5478a63db8
Share bindgen flags globally; r=emilio, r=froydnj
https://hg.mozilla.org/mozilla-central/rev/1f5478a63db8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1499359
You need to log in before you can comment on or make changes to this bug.