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.

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

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.

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

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.

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

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.

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.

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

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?

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.

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

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

(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

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?

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.

Nice - thanks for working on this Nathan!

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

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 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 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 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 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 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 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 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 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 :/

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

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

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

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

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

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

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

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.

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 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 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 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 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 on attachment 8821585 [details] Bug 1302028 - part 5 - conditionally add bindgen features to gkrust* libraries; https://reviewboard.mozilla.org/r/100822/#review101678

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

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?

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.

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

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

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

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.

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

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

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.

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

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.

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.

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

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

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

So llvm 3.9 is now needed, but there are no llvm packages for the latest Fedora? How do I disable this stuff?

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

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.

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

Filed bug 1333054 for the issue in comment 77.