1302028 - stylo: build time binding generating (bindgen)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Xidorn Quan [:xidorn] UTC+10]

As Clang 3.9 released several days ago, bindgen is now able to run on Windows with release version of LLVM.

We should consider making bindgen to be run on build time rather than including the generated files as source files. We may also want to make atoms binding be generated by bindgen rather than a custom script.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: bindings.diff

It is the diff between the current version in the repo and what generated locally.

As far as I can see, the only differences are
* type of MozExternalRefCountType (which is expected because it does have a different type on Windows, see [1])
* piecewise_construct_t, pair, and array types
* the default type of enum
* the link_name of static members

So it seems the generated code is pretty reliable now.


[1] https://dxr.mozilla.org/mozilla-central/rev/7e873393cc11d326338779e5a3ed2da031e30936/mfbt/RefCountType.h

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

To get this, we may want to convert regen.py into a build script of gecko_bindings crate.

It seems it is currently not possible to pass an argument as input for build script. Not sure how can we inform the build script about the objdir.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

We should be able to use environment variables, is the standard way cargo uses to communicate the output directory, see[1].

[1]: https://github.com/servo/servo/blob/e273517fe06720afe0ef74815cce5e007df7dc3d/components/servo/build.rs#L28

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Environment variables probably work. Before doing that, I guess we need to investigate the memory issue of bindgen first. It seems to take 10GB memory when generating bindings.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

According to emilio, the huge memory usage is because of docopt [1], which means we don't really need to address it for build time generating, as we can just get rid of the whole argument parsing phase and use bindgen as a library directly.

[1] https://github.com/servo/rust-bindgen/issues/46

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

This would make bindgen a dev-dependency of gecko_bindings, and thus we would need to either merge bindgen with upstream and put it onto crates.io so that we can vendor it, or keep an in-tree copy of that.

Probably not a very high priority thing at this moment.

Comment 7

[Bobby Holley (:bholley)]

I think we need to wait until the CI integration is done before doing bindings at build-time, otherwise we'll lose our ability to build geckolib on Servo's CI.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Build time bindgen is landing in servo/servo#14244.

Comment 9

[Nathan Froyd [:froydnj]]

I have this working locally with a LLVM 3.9 tarball.  In my limited understanding, there are actually two parts here:

1. Make the stylo repo actually use this (very straightforward with patches)
2. Actually make developers use build-time bindgen, either via manual "flip this mozconfig option" or enforced at build time via configure-time checks or something.

Is it useful to separate the two?  Or do we want to enforce the developer workflow at the same time that things turn on in treeherder?

Comment 10

[Bobby Holley (:bholley)]

Good question.

I think that until we iron out the kinks, an opt-out (--disable-bindgen or somesuch) to use the in-tree bindings would probably be helpful. But it shouldn't be the default, because that's a footgun.

If it's easy, a mach command to re-vendor the build-time-generated bindings into the source tree would be helpful for when we upstream patches to servo. But it's less urgent, and doesn't need to happen in this bug.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> I think that until we iron out the kinks, an opt-out (--disable-bindgen or
> somesuch) to use the in-tree bindings would probably be helpful. But it
> shouldn't be the default, because that's a footgun.

Hm.  The current patchset I have is actually opt-in, as it requires you to specify the particularly clang bits that bindgen should use.  If those bits aren't specified, then manual bindings generation is required.  I guess that means you're not a fan?  I'm not sure what --disable-bindgen would be useful for, though...

Having automation automatically generate bindings while requiring local developers to generate their own bindings means that things could rapidly get out of sync.  I guess we have to land the whole thing (automation + defaulting to build-time bindgen) to get maximal usefulness out of it, then?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> > I think that until we iron out the kinks, an opt-out (--disable-bindgen or
> > somesuch) to use the in-tree bindings would probably be helpful. But it
> > shouldn't be the default, because that's a footgun.
> 
> Hm.  The current patchset I have is actually opt-in, as it requires you to
> specify the particularly clang bits that bindgen should use.  If those bits
> aren't specified, then manual bindings generation is required.  I guess that
> means you're not a fan?

Can't we just detect it like we do with everything else in the toolchain?

> I'm not sure what --disable-bindgen would be useful
> for, though...

If some developer is having trouble getting libclang on their platform in a way that the build system can recognize, which seems like it might happen in the first few days of rolling this out.

> Having automation automatically generate bindings while requiring local
> developers to generate their own bindings means that things could rapidly
> get out of sync.

I don't parse this. It seems like you're talking about build-time bindgen everywhere, which is what we want.

Basically, I would like for both Treeherder and local builds to use build-time bindgen. The only reason we'd need the checked-in bindings for now is for Servo's Homu CI, which doesn't have a copy of m-c handy. We can then let those checked-in bindings drift a bit without much problem, and only update them when we try to land patches to servo/servo that require them.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> If it's easy, a mach command to re-vendor the build-time-generated bindings
> into the source tree would be helpful for when we upstream patches to servo.
> But it's less urgent, and doesn't need to happen in this bug.

Note: it looks like this is already happening in https://github.com/servo/servo/pull/14602

Comment 14

[Gregory Szorc [:gps]]

I think bholley already answered your needinfo questions?

We definitely want bindgen to be enabled by default in automation and developer builds.

To minimize risk before it is turned on, we may want to have configure enforce the presence of libclang before bindgen is enabled (while providing a --disable-libclang or some such as an escape hatch). That will enable us to flush out issues with libclang generation before it is actually required.

That being said, if bindgen is only relevant to stylo builds, the impacted audience should be small, so we should be able to land this whenever?

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 1 - add clang/llvm repacks to linux64 build manifest for stylo;

We'll need these for building with build-time bindgen.

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

Comment 16

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

We assume that the llvm-config in the user's PATH is recent enough; it
can be overridden with the environment variable LLVM_CONFIG.  We also
add rudimentary checking for versions, though we don't go so far as
checking whether all the necessary libraries are there or whether the
clang llvm-config points us to is runnable.

Discussions indicated that a --disable-bindgen option might be useful,
so that tags along in this commit as well.  Build-time bindgen is the
default, however.

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

Comment 17

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 3 - add bindgen feature to gecko-side Cargo.toml files for propagation to geckoservo;

Everything that transitively depends on geckoservo needs the bindgen
feature exposed.

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

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 4 - pass LIBCLANG_PATH and CLANG_PATH environment variables to cargo build;

We need both of these: LIBCLANG_PATH is necessary when building the
clang-sys crate or its dependencies, and CLANG_PATH is necessary when
actually using the clang-sys crate.

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

Comment 19

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 5 - conditionally add bindgen features to gkrust* libraries;

We need the bindgen feature if we're building with Stylo and we've
enabled build-time bindgen.  (Build-time bindgen is the default.)

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

Comment 20

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 6 - vendor libbindgen and associated libraries;

Holy package changes, Batman!

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

Comment 21

[Nathan Froyd [:froydnj]]

Patches up for review.  My tree might be slightly out of date with respect to vendored code, but it would be helpful if people tried things out and see what they thought.

--enable-stylo should now default to build-time bindgen, and it assumes that you have a 3.9 llvm-config in your PATH.  Like most things in the build system, you can point it to an appropriate llvm-config by setting the LLVM_CONFIG environment variable.  --disable-build-bindgen disables build-time bindgen and assumes that you know what you're doing.

Comment 22

[Bobby Holley (:bholley)]

Nice - thanks for working on this Nathan!

Comment 23

[Nathan Froyd [:froydnj]]

I'm also assuming that contributors are smart enough to get clang 3.9 from somewhere appropriate; the |mach bootstrap| support can be added at a later point.

Comment 24

[Manish Goregaokar [:manishearth]]

Comment on attachment 8819017 [details]
Bug 1302028 - part 3 - add bindgen feature to gecko-side Cargo.toml files for propagation to geckoservo;

https://reviewboard.mozilla.org/r/98886/#review99152

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Ah, the libbindgen on crates.io is still using lazy_static 0.1.6... That one embeds a binary file. We should publish the new version of libbindgen which has updated the dependency to 0.2.1, and use it here.

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8819015 [details]
Bug 1302028 - part 1 - add clang/llvm repacks to linux64 build manifest for stylo;

https://reviewboard.mozilla.org/r/98882/#review99176

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8819016 [details]
Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

https://reviewboard.mozilla.org/r/98884/#review99178

Just nits on this one. You may want to ask glandium or chmanchester for a drive-by review, as my moz.configure knowledge isn't as strong as theirs.

::: toolkit/moz.configure:569
(Diff revision 1)
> -    if value:
> -        return True
> +option('--disable-build-bindgen', default=False,
> +       help='Disable build-time bindgen for Stylo')

Should *stylo* be in the name of the option since "binding generation" is generic? How about `--disable-stylo-binding-generation`?

::: toolkit/moz.configure:597
(Diff revision 1)
> +            lines = subprocess.check_output(
> +                [llvm_config, '--version']
> +            ).splitlines()
> +            version = Version(lines[0])

This could be extracted to its own function. But since it is only 4 lines, meh.

::: toolkit/moz.configure:615
(Diff revision 1)
> +        # LLVM is new enough.  We need the path to the LLVM libraries and
> +        # the path to the clang binary.
> +        try:
> +            libclang_path = subprocess.check_output(
> +                [llvm_config, '--libdir']
> +            ).splitlines()[0]
> +            clang_path = subprocess.check_output(
> +                [llvm_config, '--bindir']
> +            ).splitlines()[0]
> +            clang_path = os.path.join(clang_path, 'clang')

Now I'm starting to think we'll want an ``llvm_info()`` or some such. Again, this can be deferred to a follow-up unless you feel it is important to abstract now.

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8819018 [details]
Bug 1302028 - part 4 - pass LIBCLANG_PATH and CLANG_PATH environment variables to cargo build;

https://reviewboard.mozilla.org/r/98888/#review99180

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8819019 [details]
Bug 1302028 - part 5 - conditionally add bindgen features to gkrust* libraries;

https://reviewboard.mozilla.org/r/98890/#review99182

I wonder if we'll eventually have enough features to warrant separating feature setting to a standaline/included mozbuild file. I reckon you'll be involved in code review for that, so please keep it in mind if you see lots of duplicate code being written.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8819020 [details]
Bug 1302028 - part 6 - vendor libbindgen and associated libraries;

https://reviewboard.mozilla.org/r/98892/#review99186

This is a rubber stamp review. I didn't audit the code nor the licenses. We can do that when these crates get vendored into mozilla-central. I'm fine with the stylo repository being the wild west for now in the interest of developer productivity.

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8819020 [details]
Bug 1302028 - part 6 - vendor libbindgen and associated libraries;

https://reviewboard.mozilla.org/r/98892/#review99188

Also, note Xidorn's comment on the bug about taking a newer version. I trust you to do whatever is appropriate. You can carry forward my r+ on this commit.

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8819016 [details]
Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

https://reviewboard.mozilla.org/r/98884/#review99190

::: toolkit/moz.configure:567
(Diff revision 1)
>  option('--enable-stylo', env='STYLO_ENABLED', nargs=0,
>         help='Enables experimental integration with the servo style system. '
>              'This requires either building servo within Gecko\'s cargo phase '
>              'or passing --with-servo')
>  
> -@depends('--enable-stylo')
> +llvm_config = check_prog('LLVM_CONFIG', ('llvm-config',), allow_missing=True)

One issue that, the official LLVM Windows build doesn't have llvm-config at all. Should we rely on something different?

Comment 33

[Gregory Szorc [:gps]]

Comment on attachment 8819016 [details]
Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

https://reviewboard.mozilla.org/r/98884/#review99190

> One issue that, the official LLVM Windows build doesn't have llvm-config at all. Should we rely on something different?

Good catch.

We don't have Windows automation enabled for `--enable-stylo` yet, so this isn't a problem in automation. But developers on Windows could certainly trip over this. So we best deal with it :/

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> Ah, the libbindgen on crates.io is still using lazy_static 0.1.6... That one
> embeds a binary file. We should publish the new version of libbindgen which
> has updated the dependency to 0.2.1, and use it here.

emilio, could you publish a new version of libbindgen?

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8819016 [details]
Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

https://reviewboard.mozilla.org/r/98884/#review99190

> Good catch.
> 
> We don't have Windows automation enabled for `--enable-stylo` yet, so this isn't a problem in automation. But developers on Windows could certainly trip over this. So we best deal with it :/

clang-sys can correctly find clang in default install location on Windows without LIBCLANG_PATH. I wonder whether we can simply rely on that on Windows...

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8819018 [details]
Bug 1302028 - part 4 - pass LIBCLANG_PATH and CLANG_PATH environment variables to cargo build;

https://reviewboard.mozilla.org/r/98888/#review99198

::: config/rules.mk:950
(Diff revision 1)
> -CARGO_BUILD = env $(rustflags_override) CARGO_TARGET_DIR=. RUSTC=$(RUSTC) MOZ_DIST=$(ABS_DIST) $(CARGO) build $(cargo_build_flags)
> +CARGO_BUILD = env $(rustflags_override) \
> +	CARGO_TARGET_DIR=. \
> +	RUSTC=$(RUSTC) \
> +	MOZ_DIST=$(ABS_DIST) \
> +	LIBCLANG_PATH=$(MOZ_LIBCLANG_PATH) \
> +	CLANG_PATH=$(MOZ_CLANG_PATH) \

I don't see why this is necessary. As far as I can see, the only code which queries `CLANG_PATH` in clang-sys is its `support::Clang::find`, and the only callsite of this function is inside `test` directory.

I don't have this environment variable set when I use it, and bholley's `setup_bindgen.sh` doesn't seem to have anything related either.

Comment 37

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #36)
> Comment on attachment 8819018 [details]
> Bug 1302028 - part 4 - pass LIBCLANG_PATH and CLANG_PATH environment
> variables to cargo build;
> 
> https://reviewboard.mozilla.org/r/98888/#review99198
> 
> ::: config/rules.mk:950
> (Diff revision 1)
> > -CARGO_BUILD = env $(rustflags_override) CARGO_TARGET_DIR=. RUSTC=$(RUSTC) MOZ_DIST=$(ABS_DIST) $(CARGO) build $(cargo_build_flags)
> > +CARGO_BUILD = env $(rustflags_override) \
> > +	CARGO_TARGET_DIR=. \
> > +	RUSTC=$(RUSTC) \
> > +	MOZ_DIST=$(ABS_DIST) \
> > +	LIBCLANG_PATH=$(MOZ_LIBCLANG_PATH) \
> > +	CLANG_PATH=$(MOZ_CLANG_PATH) \
> 
> I don't see why this is necessary. As far as I can see, the only code which
> queries `CLANG_PATH` in clang-sys is its `support::Clang::find`, and the
> only callsite of this function is inside `test` directory.

I see:

https://github.com/servo/rust-bindgen/blob/master/libbindgen/src/lib.rs#L520

and I was certainly having issues when I was not setting this.  Perhaps your clang on $PATH is new enough that clang-sys winds up using that one instead?

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Nathan Froyd [:froydnj] from comment #37)
> I see:
> 
> https://github.com/servo/rust-bindgen/blob/master/libbindgen/src/lib.rs#L520
> 
> and I was certainly having issues when I was not setting this.  Perhaps your
> clang on $PATH is new enough that clang-sys winds up using that one instead?

Oh, hmm, ok. I guess it is just not necessary for Windows because we specify "--target" in build script for Windows build, so that part is ignored anyway. Not sure about what happens to Mac and Linux.

Comment 39

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #34)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> emilio, could you publish a new version of libbindgen?

Done. Also, I gave publish rights to you, fitzgen, and the servo/cargo-publish time, if in the future you need better turnaround time when it's night in Europe :)

Comment 40

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #35)
> Comment on attachment 8819016 [details]
> Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;
> 
> https://reviewboard.mozilla.org/r/98884/#review99190
> 
> > Good catch.
> > 
> > We don't have Windows automation enabled for `--enable-stylo` yet, so this isn't a problem in automation. But developers on Windows could certainly trip over this. So we best deal with it :/
> 
> clang-sys can correctly find clang in default install location on Windows
> without LIBCLANG_PATH. I wonder whether we can simply rely on that on
> Windows...

We could rely on that for Windows, but I'd really like to have some way of verifying the version of LLVM/Clang that we're about to use for bindgen at configure time.  The errors that I got sort of pointed to the problem, but they were definitely not at the simplicity of "You have the wrong version of clang.  Please fix."  They also occurred very late in the build process, rather than up-front at configure.

I could try parsing out a version from |clang --version|:

froydnj@thor:~/src/mi.hg$ clang --version
Debian clang version 3.5.0-10 (tags/RELEASE_350/final) (based on LLVM 3.5.0)
Target: x86_64-pc-linux-gnu
Thread model: posix

froydnj@froydnj-pc /c/m-c
$ clang --version
clang version 4.0.0
Target: i686-pc-windows-msvc
Thread model: posix
InstalledDir: c:\clang-cl-install\bin

# Admittedly we can't use this one because I'm pretty sure Apple doesn't really distribute all
# the libraries/headers/etc. we'd need...
jeangray:tmp froydnj$ clang --version
Apple LLVM Version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin14.5.0
Thread model: posix

It would, of course, be nice to use all the infrastructure we have for determining compiler versions here, but I'm not quite sure if trying to re-use the code from toolchain.configure is the right approach.  (Getting the version information out of predefined macros is not straightforward.)

Other options that make things better for the Windows case:

1. Provide configure options --with-llvm-libraries-path and --with-clang-binary-path and use those.  We probably need to punt on determining the validity of those?
2. As 1) above, but also with the current LLVM_CONFIG support, so you can just set one thing and be done.  LLVM_CONFIG would take precedence over the --with options.
3. Require our homebuilt clang-cl binaries via mach bootstrap, which are packaged up with llvm-config.exe.

I don't know if we can assume that Windows folks are necessarily using the installer, and we have to agree on a solution that works without the installer for automation builds.  Setting up environment variables would work in automation builds, but people might hand-roll their own toolchains...

I don't know what the Right Thing is for Windows users here.  Thoughts?

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Nathan Froyd [:froydnj] from comment #40)
> # Admittedly we can't use this one because I'm pretty sure Apple doesn't
> really distribute all
> # the libraries/headers/etc. we'd need...
> jeangray:tmp froydnj$ clang --version
> Apple LLVM Version 7.0.2 (clang-700.1.81)
> Target: x86_64-apple-darwin14.5.0
> Thread model: posix

Apple's clang doesn't have llvm-config either, FWIW. Actually I don't think we can use Apple's clang.

> It would, of course, be nice to use all the infrastructure we have for
> determining compiler versions here, but I'm not quite sure if trying to
> re-use the code from toolchain.configure is the right approach.  (Getting
> the version information out of predefined macros is not straightforward.)
> 
> Other options that make things better for the Windows case:
> 
> 1. Provide configure options --with-llvm-libraries-path and
> --with-clang-binary-path and use those.  We probably need to punt on
> determining the validity of those?
> 2. As 1) above, but also with the current LLVM_CONFIG support, so you can
> just set one thing and be done.  LLVM_CONFIG would take precedence over the
> --with options.
> 3. Require our homebuilt clang-cl binaries via mach bootstrap, which are
> packaged up with llvm-config.exe.
> 
> I don't know if we can assume that Windows folks are necessarily using the
> installer, and we have to agree on a solution that works without the
> installer for automation builds.  Setting up environment variables would
> work in automation builds, but people might hand-roll their own toolchains...
> 
> I don't know what the Right Thing is for Windows users here.  Thoughts?

I think providing configure options works for now, since only limited developers are hacking on stylo at the moment. We may want a more automatic solution, but that isn't that high priority I suppose.

I remembered that you said you'd like to deliver prebuilt clang package via "mach bootstrap"? I think that could be the solution to this.

Comment 42

[Nathan Froyd [:froydnj]]

Comment on attachment 8819016 [details]
Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

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

Comment 43

[Nathan Froyd [:froydnj]]

Apparently I am terrible at using mozreview.

Anyway, new commit up with the configure options so things work OK on Windows.  Trying things locally with various combinations of LLVM_CONFIG and the new configure options seemed to work properly.

Comment 44

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8819016 [details]
Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

https://reviewboard.mozilla.org/r/98884/#review100382

::: toolkit/moz.configure:587
(Diff revision 2)
> +    try:
> +        lines = subprocess.check_output(
> +            [llvm_config] + list(options)

Is `check_cmd_output` suitable for this?

::: toolkit/moz.configure:594
(Diff revision 2)
> +            [llvm_config] + list(options)
> +        ).splitlines()
> +        return lines[0]
> +    except subprocess.CalledProcessError as e:
> +        die('Failed to call llvm-config: %s', e.message)
> +    

Leading space.

::: toolkit/moz.configure:613
(Diff revision 2)
> +    if llvm_config:
> +        check_minimum_llvm_config_version(llvm_config)
> +        return invoke_llvm_config(llvm_config, '--libdir')
> +    elif option_path.origin != 'default':
> +        return option_path[0]

So if someone passes `--with-libclang-path` and has `llvm-config` in their path, we trust the value from `llvm-config`? Maybe this isn't going to happen, but if it did, I might expect the value for `--with-libclang-path` to take priority.

::: toolkit/moz.configure:616
(Diff revision 2)
> +    elif option_path.origin != 'default':
> +        return option_path[0]

I expect `option_path` will be falsy if not set, we didn't provide a default above.

::: toolkit/moz.configure:669
(Diff revision 2)
> -set_config('MOZ_STYLO', stylo)
> -set_define('MOZ_STYLO', stylo)
> +set_config('MOZ_LIBCLANG_PATH', depends(stylo)(lambda info: info.libclang_path))
> +set_config('MOZ_CLANG_PATH', depends(stylo)(lambda info: info.clang_path))
> +set_config('MOZ_STYLO_BINDGEN', depends(stylo)(lambda info: info.bindgen_enabled))

We don't want to set any of this if we don't have stylo enabled, right? Maybe we can return `None` from `stylo` when it's not enabled, and replace these `depends(stylo)(...)` with `delayed_getattr(stylo, ...)`. Then we could leave `jemalloc` below alone.

Comment 45

[Nathan Froyd [:froydnj]]

ni? for the llvm-config issue below.

(In reply to Chris Manchester (:chmanchester) from comment #44)
> ::: toolkit/moz.configure:587
> (Diff revision 2)
> > +    try:
> > +        lines = subprocess.check_output(
> > +            [llvm_config] + list(options)
> 
> Is `check_cmd_output` suitable for this?

Yes, thanks for the pointer.

> ::: toolkit/moz.configure:594
> (Diff revision 2)
> > +            [llvm_config] + list(options)
> > +        ).splitlines()
> > +        return lines[0]
> > +    except subprocess.CalledProcessError as e:
> > +        die('Failed to call llvm-config: %s', e.message)
> > +    
> 
> Leading space.

/me grumbles at pythone-mode

> ::: toolkit/moz.configure:613
> (Diff revision 2)
> > +    if llvm_config:
> > +        check_minimum_llvm_config_version(llvm_config)
> > +        return invoke_llvm_config(llvm_config, '--libdir')
> > +    elif option_path.origin != 'default':
> > +        return option_path[0]
> 
> So if someone passes `--with-libclang-path` and has `llvm-config` in their
> path, we trust the value from `llvm-config`? Maybe this isn't going to
> happen, but if it did, I might expect the value for `--with-libclang-path`
> to take priority.

My reasoning here was that `llvm-config` can give us both pieces of information at once, and it's much more likely that those pieces of information are consistent with each other.  Whereas if you have `llvm-config` and one of the new `--with` options, it's much easier for those two things to not agree with one another.  WDYT?

> ::: toolkit/moz.configure:616
> (Diff revision 2)
> > +    elif option_path.origin != 'default':
> > +        return option_path[0]
> 
> I expect `option_path` will be falsy if not set, we didn't provide a default
> above.

Hm, I was trying to cargo-cult here; I will trust you, but also verify locally. ;)

> ::: toolkit/moz.configure:669
> (Diff revision 2)
> > -set_config('MOZ_STYLO', stylo)
> > -set_define('MOZ_STYLO', stylo)
> > +set_config('MOZ_LIBCLANG_PATH', depends(stylo)(lambda info: info.libclang_path))
> > +set_config('MOZ_CLANG_PATH', depends(stylo)(lambda info: info.clang_path))
> > +set_config('MOZ_STYLO_BINDGEN', depends(stylo)(lambda info: info.bindgen_enabled))
> 
> We don't want to set any of this if we don't have stylo enabled, right?
> Maybe we can return `None` from `stylo` when it's not enabled, and replace
> these `depends(stylo)(...)` with `delayed_getattr(stylo, ...)`. Then we
> could leave `jemalloc` below alone.

That sounds much nicer, let's do that.

Comment 46

[Chris Manchester (limited bugmail, email directly)]

I agree with the reasoning. So maybe if someone passes just one of the new `--with` options, that's an error, but if they pass both, these options take priority over any `llvm-config` hanging around.

Comment 47

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 1 - add clang/llvm repacks to linux64 build manifest for stylo;

We'll need these for building with build-time bindgen.

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

Comment 48

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 3 - add bindgen feature to gecko-side Cargo.toml files for propagation to geckoservo;

Everything that transitively depends on geckoservo needs the bindgen
feature exposed.

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

Comment 49

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 4 - pass LIBCLANG_PATH and CLANG_PATH environment variables to cargo build;

We need both of these: LIBCLANG_PATH is necessary when building the
clang-sys crate or its dependencies, and CLANG_PATH is necessary when
actually using the clang-sys crate.

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

Comment 50

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 5 - conditionally add bindgen features to gkrust* libraries;

We need the bindgen feature if we're building with Stylo and we've
enabled build-time bindgen.  (Build-time bindgen is the default.)

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

Comment 51

[Nathan Froyd [:froydnj]]

Attachment: Bug 1302028 - part 6 - vendor libbindgen and associated libraries;

Holy package changes, Batman!

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

Comment 52

[Nathan Froyd [:froydnj]]

Comment on attachment 8819016 [details]
Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

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

Comment 53

[Manish Goregaokar [:manishearth]]

Comment on attachment 8821583 [details]
Bug 1302028 - part 3 - add bindgen feature to gecko-side Cargo.toml files for propagation to geckoservo;

https://reviewboard.mozilla.org/r/100818/#review101366

Comment 54

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8819016 [details]
Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

https://reviewboard.mozilla.org/r/98884/#review101410

I should have noticed the enabled/disabled issue before, it's a common pitfall. If you're confident in how to resolve that we can call this r+ with that addressed.

"configury"?

::: toolkit/moz.configure:575
(Diff revision 3)
> +option('--disable-stylo-build-bindgen', default=False,
> +       help='Disable build-time bindgen for Stylo')

Leave off `default=False`.

The `--disable` prefix just means we get a default of `True` for the option, we're still intended to interpret the option value being set as the thing being enabled.

Due to how this is handled below, we succeed in turning this on by default, but we only end up disabling build time bindgen if `--enable-stylo-build-bindgen` is passed.

::: toolkit/moz.configure:632
(Diff revision 3)
> +        clang_path=clang_path[0],
> +    )
> +
> +@depends('--enable-stylo', bindgen_config_paths, '--disable-stylo-build-bindgen')
> +@imports(_from='textwrap', _import='dedent')
> +def stylo(stylo_enabled, bindgen_config_paths, disable_bindgen):

...so call this third parameter 'enable_bindgen' and use it accordingly.

::: toolkit/moz.configure:639
(Diff revision 3)
> +
> +    if not stylo_enabled:
> +        return None
> +    elif disable_bindgen:
> +        return namespace(
> +            enabled=bool(stylo_enabled)

`enabled=True`?

::: toolkit/moz.configure:649
(Diff revision 3)
> +        bindgen.  Please put 'llvm-config' in your PATH, specify the
> +        'LLVM_CONFIG' environment variable, or pass the '--with-libclang-path'
> +        and '--with-clang-path' options to configure.'''))
> +
> +    return namespace(
> +        stylo_enabled=bool(stylo_enabled),

Typo? `enabled=...`

Comment 55

[Gregory Szorc [:gps]]

Comment on attachment 8821582 [details]
Bug 1302028 - part 1 - add clang/llvm repacks to linux64 build manifest for stylo;

https://reviewboard.mozilla.org/r/100816/#review101674

I can't wait until we don't have to worry about Linux on buildbot so we can roll these toolchains into the Docker images used by TaskCluster: that should cut down on overhead required to install things from tooltool and will make the toolchain compilation process more transparent. Somewhere there is a bug tracking this (we want to build toolchains in their own TC tasks and have the artifacts consumed during Docker image building - that way we have a DAG of all the dependencies that go into the build image).

Comment 56

[Gregory Szorc [:gps]]

Comment on attachment 8821584 [details]
Bug 1302028 - part 4 - pass LIBCLANG_PATH and CLANG_PATH environment variables to cargo build;

https://reviewboard.mozilla.org/r/100820/#review101676

Thank you for improving the formatting while you were here.

Comment 57

[Gregory Szorc [:gps]]

Comment on attachment 8821585 [details]
Bug 1302028 - part 5 - conditionally add bindgen features to gkrust* libraries;

https://reviewboard.mozilla.org/r/100822/#review101678

Comment 58

[Gregory Szorc [:gps]]

Comment on attachment 8821586 [details]
Bug 1302028 - part 6 - vendor libbindgen and associated libraries;

https://reviewboard.mozilla.org/r/100824/#review101684

/me uses rubber stamp

Comment 59

[Nathan Froyd [:froydnj]]

In the quantum productivity meeting earlier this week, we discussed the possibility of landing these patches on mozilla-central, with bindgen defaulted to off, so stylo developers didn't have to wait on the import of servo/servo.  That sounded pretty reasonable to me, so I looked into doing that.

Unfortunately, for these patches to land, we'd have to declare an optional dependency on geckoservo...which doesn't actually exist in-tree yet.  AFAICT, you cannot declare an optional dependency that Cargo cannot see; Cargo might not need to compile that dependency, but it does need to parse its Cargo.toml.

I guess we could land a dummy geckoservo, and then overwrite that when servo/servo gets imported.  gps, WDYT about that idea?

Comment 60

[Gregory Szorc [:gps]]

I'm fine with landing a dummy geckoservo as long as it isn't like 1+ MB or so. We still have a few transformations on the servo import I need to implement. And the Stylo teams says they really want ongoing landing and the initial one-time import doesn't buy us much. So both will likely land around the same time.

Comment 61

[Nathan Froyd [:froydnj]]

(In reply to Gregory Szorc [:gps] from comment #60)
> I'm fine with landing a dummy geckoservo as long as it isn't like 1+ MB or
> so. We still have a few transformations on the servo import I need to
> implement. And the Stylo teams says they really want ongoing landing and the
> initial one-time import doesn't buy us much. So both will likely land around
> the same time.

The dummy geckoservo crate would be about 15 lines of code, so it's not going to be very big.

Will having this empty crate cause havoc when mozilla-central is merged to stylo-incubator, since stylo-incubator has an actual crate at the same path, with some of the same files?  Or will the merge be handled manually, so we can force the stylo-incubator side to take precedence?

Comment 62

[Bobby Holley (:bholley)]

(In reply to Nathan Froyd [:froydnj] from comment #61)
> (In reply to Gregory Szorc [:gps] from comment #60)
> > I'm fine with landing a dummy geckoservo as long as it isn't like 1+ MB or
> > so. We still have a few transformations on the servo import I need to
> > implement. And the Stylo teams says they really want ongoing landing and the
> > initial one-time import doesn't buy us much. So both will likely land around
> > the same time.
> 
> The dummy geckoservo crate would be about 15 lines of code, so it's not
> going to be very big.
> 
> Will having this empty crate cause havoc when mozilla-central is merged to
> stylo-incubator, since stylo-incubator has an actual crate at the same path,
> with some of the same files?  Or will the merge be handled manually, so we
> can force the stylo-incubator side to take precedence?

The merge will be done manually, probably by heycam (who does one every few days).

Comment 63

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #54)
> I should have noticed the enabled/disabled issue before, it's a common
> pitfall. If you're confident in how to resolve that we can call this r+ with
> that addressed.
> 
> "configury"?

I'm not sure where I picked that up, but that's what I've always called configure-y things.  I can change the wording. :)

> ::: toolkit/moz.configure:575
> (Diff revision 3)
> > +option('--disable-stylo-build-bindgen', default=False,
> > +       help='Disable build-time bindgen for Stylo')
> 
> Leave off `default=False`.
> 
> The `--disable` prefix just means we get a default of `True` for the option,
> we're still intended to interpret the option value being set as the thing
> being enabled.
> 
> Due to how this is handled below, we succeed in turning this on by default,
> but we only end up disabling build time bindgen if
> `--enable-stylo-build-bindgen` is passed.

Hah, I thought you made a typo; experimenting proves you didn't.  But we actually want the flag to be enabled by default, and require people to provide `--disable-stylo-build-bindgen` if they want to turn it off.  So we want:

option('--enable-stylo-build-bindgen', default=True,
       help='Enable build-time bindgen for Stylo')

Right?

Comment 64

[Nathan Froyd [:froydnj]]

Attachment: part 3a - add dummy geckoservo crate

Build-time bindgen requires that the geckoservo crate exist, both so
Cargo can have an accurate picture of the dependency graph and so we can
specify features to pass into it.  Unfortunately, geckoservo won't exist
until the servo/servo import is done, which is going to take a little
while.

To land build-time bindgen on mozilla-central, therefore, we specify
this dummy geckoservo crate that does nothing.  Our Rust code can then
depend on this crate, and things will magically continue to work when
the servo/servo import lands.

Comment 65

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #63)
> (In reply to Chris Manchester (:chmanchester) from comment #54)
> > I should have noticed the enabled/disabled issue before, it's a common
> > pitfall. If you're confident in how to resolve that we can call this r+ with
> > that addressed.
> > 
> > "configury"?
> 
> I'm not sure where I picked that up, but that's what I've always called
> configure-y things.  I can change the wording. :)

Ok, no need to change it. I hadn't seen it before, I was curious if we were minting the term in the commit message.

> 
> > ::: toolkit/moz.configure:575
> > (Diff revision 3)
> > > +option('--disable-stylo-build-bindgen', default=False,
> > > +       help='Disable build-time bindgen for Stylo')
> > 
> > Leave off `default=False`.
> > 
> > The `--disable` prefix just means we get a default of `True` for the option,
> > we're still intended to interpret the option value being set as the thing
> > being enabled.
> > 
> > Due to how this is handled below, we succeed in turning this on by default,
> > but we only end up disabling build time bindgen if
> > `--enable-stylo-build-bindgen` is passed.
> 
> Hah, I thought you made a typo; experimenting proves you didn't.  But we
> actually want the flag to be enabled by default, and require people to
> provide `--disable-stylo-build-bindgen` if they want to turn it off.  So we
> want:
> 
> option('--enable-stylo-build-bindgen', default=True,
>        help='Enable build-time bindgen for Stylo')
> 
> Right?

Well, we might, but

option('--disable-stylo-build-bindgen',
       help='Disable build-time bindgen for Stylo')

ought to do the same thing, and follows the convention of '--disable-' prefix options for things that are turned on by default.

Comment 66

[Nathan Froyd [:froydnj]]

Argh, so if we have a servo/ directory, as this patch series adds, stylo builds are automagically built:

http://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/filter_tasks.py#41

I guess we want to make this check a bit smarter, perhaps inspecting the servo/ports/geckoservo/Cargo.toml file to make sure that it's "really Servo"?  Or checking for an alternative directory?

ni? gps since it looks like he wrote this code

Comment 67

[Gregory Szorc [:gps]]

Checking for an alternate directory is likely easier.

Also, does this patch series mean that the servo/ directory won't match exactly the upstream repo? If so, that's problematic for VCS "syncing." If we need to create mozilla-central specific files related to the servo repo, I'd highly prefer they not exist in the servo/ directory of mozilla-central.

Comment 68

[Nathan Froyd [:froydnj]]

(In reply to Gregory Szorc [:gps] from comment #67)
> Checking for an alternate directory is likely easier.
> 
> Also, does this patch series mean that the servo/ directory won't match
> exactly the upstream repo? If so, that's problematic for VCS "syncing." If
> we need to create mozilla-central specific files related to the servo repo,
> I'd highly prefer they not exist in the servo/ directory of mozilla-central.

That's what comment 59 proposed to do, yes.  My intent was--probably not explained clearly enough:

1. Land patches in this bug with a dummy servo/ports/geckoservo/ directory.
2. When Cameron or whoever merges m-c to stylo-integration the first time after part 1, that servo/ports/geckoservo/ would be overwritten with the actual servo/ports/geckoservo/.
3. Future merges to stylo-integration wouldn't have to care, assuming I understand VCS merging.
4. Merges to mozilla-central from stylo-integration would require some manual intervention the first time (I think?  unsure here), but should be OK after that, subject to the same assumptions in 3).

We could land the dummy geckoservo crate from this bug in a different path.  But then stylo-integration would need modifications to the Cargo.{toml,lock} files to re-point the paths for geckoservo at the proper path.  Picking the right side of the merge to use seems easier that modifying files for the merge, but I am not an expert here.  Is that clearer, and do you have advice on the way forward?

Comment 69

[Gregory Szorc [:gps]]

The first merge of servo into mozilla-central will be manual. We can overwrite these dummy files with the real deal. I wouldn't worry about the dummy files in the servo/ directory. But we will need to make the taskgraph path logic look somewhere else.

Comment 70

[Nathan Froyd [:froydnj]]

Attachment: part 3b - modify path for filtering out servo tasks

We're adding a dummy servo/ports/geckoservo/ directory, which would make
the filtering logic for Taskcluster tasks think that we want to run
Servo tasks all the time.  But we don't have a complete installation of
Servo, so things will inevitably fall over if we did that.  To avoid
this situation, change the path that the filter checks for to something
a little more specific and less likely to cause conflicts.

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee5f618279a
part 1 - add clang/llvm repacks to linux64 build manifest for stylo; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/210b8d21be06
part 2 - add configury for stylo build-time bindgen needs; r=gps,chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/84cf4267a006
part 3 - modify path for filtering out servo tasks; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd2b221b990b
part 4 - add dummy geckoservo crate; r=Manishearth
https://hg.mozilla.org/integration/mozilla-inbound/rev/92a3c7536c67
part 5 - add bindgen feature to gecko-side Cargo.toml files for propagation to geckoservo; r=Manishearth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e44e0b52a832
part 6 - pass LIBCLANG_PATH and CLANG_PATH environment variables to cargo build; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/b356280b2066
part 7 - conditionally add bindgen features to gkrust* libraries; r=gps

Comment 72

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eee5f618279a
https://hg.mozilla.org/mozilla-central/rev/210b8d21be06
https://hg.mozilla.org/mozilla-central/rev/84cf4267a006
https://hg.mozilla.org/mozilla-central/rev/dd2b221b990b
https://hg.mozilla.org/mozilla-central/rev/92a3c7536c67
https://hg.mozilla.org/mozilla-central/rev/e44e0b52a832
https://hg.mozilla.org/mozilla-central/rev/b356280b2066

Comment 73

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8819016 [details]
Bug 1302028 - part 2 - add configury for stylo build-time bindgen needs;

https://reviewboard.mozilla.org/r/98884/#review107326

::: toolkit/moz.configure:578
(Diff revision 3)
> +option('--with-libclang-path', nargs=1,
> +       help='Absolute path to Clang/LLVM libraries for Stylo (at least version 3.9.0')
> +option('--with-clang-path', nargs=1,
> +       help='Absolute path to a Clang binary for Stylo bindgen (at least version 3.9.0)')

This isn't clear enough... libclang-path is a path of the directory of libclang.so / libclang.dll file, but clang-path is the path of the clang executable file itself.

It should probably check whether the pathes really contain the required files, rather than trusting it and waiting for cargo to find that out.

Comment 74

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #73)
> ::: toolkit/moz.configure:578
> (Diff revision 3)
> > +option('--with-libclang-path', nargs=1,
> > +       help='Absolute path to Clang/LLVM libraries for Stylo (at least version 3.9.0')
> > +option('--with-clang-path', nargs=1,
> > +       help='Absolute path to a Clang binary for Stylo bindgen (at least version 3.9.0)')
> 
> This isn't clear enough... libclang-path is a path of the directory of
> libclang.so / libclang.dll file, but clang-path is the path of the clang
> executable file itself.

That's a fair point.  Please open another bug for fixing the documentation here.

> It should probably check whether the pathes really contain the required
> files, rather than trusting it and waiting for cargo to find that out.

We could probably do that, assuming that the list of libraries isn't terribly long.  Follow-up bug, please.

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

So llvm 3.9 is now needed, but there are no llvm packages for the latest Fedora?

How do I disable this stuff?

Comment 76

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] (review queue closed until backlog cleared) from comment #75)
> So llvm 3.9 is now needed, but there are no llvm packages for the latest
> Fedora?
> 
> How do I disable this stuff?

I suppose this is not enabled by default for non-stylo build? I guess it is a bug if it requires llvm 3.8 unconditionally...

Comment 77

[Olli Pettay [:smaug][[e-mail]]]

I have nothing stylo related in my .mozconfig.

I tried adding 
ac_add_options --disable-bindgen
ac_add_options --disable-build-bindgen
ac_add_options --disable-stylo
ac_add_options --disable-stylo-build-bindgen
or whatever was mentioned here, but no luck. 
And I didn't immediately see anything else in source code to disable this.

Comment 78

[Nathan Froyd [:froydnj]]

(In reply to Olli Pettay [:smaug] (review queue closed until backlog cleared) from comment #77)
> I have nothing stylo related in my .mozconfig.
> 
> I tried adding 
> ac_add_options --disable-bindgen
> ac_add_options --disable-build-bindgen
> ac_add_options --disable-stylo
> ac_add_options --disable-stylo-build-bindgen
> or whatever was mentioned here, but no luck. 
> And I didn't immediately see anything else in source code to disable this.

smaug and I debugged this: it turns out that we eagerly check the version of llvm-config (if it exists) before we determine whether you're building stylo or you're building bindgen.  I have a patch that fixes things, but I'm trying to think if we can avoid duplicating some logic.

Comment 79

[Nathan Froyd [:froydnj]]

Filed bug 1333054 for the issue in comment 77.