1310852 - Make mach bootstrap always install libclang packages for Stylo

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P1)

Comment 1

[Chris Peterson [:cpeterson]]

Xidorn says we need clang 3.9 or later to handle VC2015's headers.

Comment 2

[Chris Peterson [:cpeterson]]

Emilio says we can stick with clang 3.8 on OS X and Linux.

Comment 3

[Chris Peterson [:cpeterson]]

We will need to update `mach bootstrap`, various parts of build automation, and configure checks for libclang.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

It would probably be better to also include Clang in the new MozillaBuild for Windows. I don't think we can rely on msys2 to get it. Before the next MozillaBuild with Clang gets released, we can do detection, but probably should not make that as a requirement.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Per discussion with vlad on IRC, we can probably try the clang provided by msys2 first. If that doesn't work well with rust-msvc, we can add the msvc build of it to mozilla's msys2 repo (assuming we will have one).

Comment 6

[(not currently active) Ted Mielczarek]

There are no current plans to release a new MozillaBuild, FYI. We've been hoping to be able to switch people over to msys2.

Comment 7

[Chris Peterson [:cpeterson]]

P2 because rust-bindgen and libclang are a high priority but they are not immediate blockers for Stylo.

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5)
> Per discussion with vlad on IRC, we can probably try the clang provided by
> msys2 first. If that doesn't work well with rust-msvc, we can add the msvc
> build of it to mozilla's msys2 repo (assuming we will have one).

Xidorn, can you please test whether msys2's clang is adequate for our rust-bindgen needs on Windows?

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

I had a brief look at msys2's clang, and there are some result (without actually trying to build rust-bindgen yet):

1. msys2's clang, as well as mingw's clang, are still on 3.8.0, which is not enough for Windows build. I'm not sure whether they are going to upgrade that.

2. msys2's clang seems to be unnecessarily larger than a native build of clang. The clang installer for Win64 downloaded from LLVM official is about 53MB. The download size of msys2's clang (with its llvm dependency) is 163MB, which triples the download size. It is probably not a big issue I guess.

3. msys2's clang has different file names (and format for static link library):
==============================================================
                | static link lib        | dynamic link lib
================|========================|====================
 official build | lib/libclang.lib       | bin/libclang.dll
----------------+------------------------+--------------------
    msys2 clang | lib/libclang.dll.a     | bin/msys-clang.dll
----------------+------------------------+--------------------
  mingw64 clang | lib/libclang.dll.a and | bin/libclang.dll
                | lib/libclang.a         |
==============================================================
It seems like MSVC linker reqires the static link stub library [1], although I have no idea why. Then the question is, can the MSVC linker recognize those .a files? If we do need the static stub library for MSVC toolchain, and MSVC linker cannot recognize .a files, we would probably need the official build of libclang.


[1] https://github.com/KyleMayes/clang-sys/blob/master/build.rs#L247

Comment 10

[Mike Hommey [:glandium]]

I /think/ the .dll.a is a .lib, with a different extension. Surely, you can check what dumpbin says about it.

Comment 11

[Gregory Szorc [:gps]]

If anyone was wondering, the Clang that can be installed as part of the Visual Studio installer only ships clang.exe and a "c2.dll." I didn't look at the symbols, but I doubt libclang parts are in there.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

I checked the symbols of c2.dll. It doesn't seem to expose libclang's functions we need.

Comment 13

[Chris Peterson [:cpeterson]]

Bobby says the Stylo team currently checks the generated bindings into the tree. We regenerate them by running these two scripts in order:

https://hg.mozilla.org/incubator/stylo/file/tip/servo/components/style/binding_tools/setup_bindgen.sh
https://hg.mozilla.org/incubator/stylo/file/tip/servo/components/style/binding_tools/regen.py

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Mike Hommey [:glandium] from comment #10)
> I /think/ the .dll.a is a .lib, with a different extension. Surely, you can
> check what dumpbin says about it.

Yes, it seems the .dll.a is a .lib, and now clang in msys2 has been upgraded to 3.9.0. I can sucessfully build rust-bindgen with msvc toolchain and mingw64's clang lib.

It would need some changes to clang-sys crate. I'll work on that.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

That would need some hack, though, because rustc hardcoded ".lib" suffix for Windows.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

So, my pull request for using mingw64's clang lib has been merged into clang-sys crate. [1] rust-bindgen just needs to upgrade to be using clang-sys 0.11.1 to get that support.

[1] https://github.com/KyleMayes/clang-sys/pull/43

Comment 17

[Chris Peterson [:cpeterson]]

Stylo bindgen is broken with llvm40 (bug 1349479). Emilio fixed llvm upstream, but his fix won't ship until llvm50. So we require exactly llvm39 for Stylo bindgen.

@ Nathan or Ted, is this something you can help us with? Checking for llvm39 and installing it via mach bootstrap (or whatever) is a prerequisite for building Stylo in Nightly by default (but pref'd off). That is bug 1356991.

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson [:cpeterson] from comment #17)
> @ Nathan or Ted, is this something you can help us with? Checking for llvm39
> and installing it via mach bootstrap (or whatever) is a prerequisite for
> building Stylo in Nightly by default (but pref'd off). That is bug 1356991.

Bug 1314355 covers the mach bootstrap side of things.  I can work on that tomorrow; the patches are pretty close.

Once we have that, it's just a matter of failing the build if we can't find the appropriate things at configure time.

Comment 19

[Chris Peterson [:cpeterson]]

(In reply to Nathan Froyd [:froydnj] from comment #18)
> (In reply to Chris Peterson [:cpeterson] from comment #17)
> > @ Nathan or Ted, is this something you can help us with? Checking for llvm39
> > and installing it via mach bootstrap (or whatever) is a prerequisite for
> > building Stylo in Nightly by default (but pref'd off). That is bug 1356991.
> 
> Bug 1314355 covers the mach bootstrap side of things.  I can work on that
> tomorrow; the patches are pretty close.
> 
> Once we have that, it's just a matter of failing the build if we can't find
> the appropriate things at configure time.

Thanks! I didn't see your work in bug 1357889 to forbid LLVM >= 4.0 until just now. With that bug now requiring llvm39 and bug 1314355 updating mach bootstrap, is there any real work left for this bug? Can we just dupe this bug to one of those other bugs?

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson [:cpeterson] from comment #19)
> Thanks! I didn't see your work in bug 1357889 to forbid LLVM >= 4.0 until
> just now. With that bug now requiring llvm39 and bug 1314355 updating mach
> bootstrap, is there any real work left for this bug? Can we just dupe this
> bug to one of those other bugs?

I think there is work left here.  Specifically, we have to have configure check for the existence of these things in default places if Stylo is enabled by default (but preffed off or whatever): I don't think we want to require people to add magic lines to their mozconfig just to accommodate Stylo.

If Stylo *isn't* enabled by default, then that's fine, and there's nothing to do here for the moment; people can continue adding special things to their mozconfig.  But if it is, then we need to decide on a plan for detecting the necessary packages that are currently installed in non-standard locations, and don't even come with MozillaBuild on Windows.

gps, what do you think is appropriate here?  `mach bootstrap` puts the necessary packages under ~/.mozbuild or equivalent on Windows.  Should we just have configure look there if there are no explicit options, and instruct people to run `mach bootstrap` if it can't find the appropriate packages?

Comment 21

[Nathan Froyd [:froydnj]]

Not super keen on baking those locations into moz.configure, though...

Comment 22

[Gregory Szorc [:gps]]

Strictly speaking, ~/.mozbuild isn't hard-coded: it can be overwritten by an environment variable. There's a helper on MozbuildBase, but it is equivalent to:

  os.environ.get('MOZBUILD_STATE_PATH',
                 os.path.expanduser(os.path.join('~', '.mozbuild')))

I think it is fine to look for toolchains in ~/.mozbuild. In fact, I'd prefer we start moving things so the default build configuration uses the same toolchain used by automation. That way, developers have to spend less time battling their system configuration and compiler and more time actually doing important work.

Before he left on holiday, glandium was working on tighter integration between toolchains built in automation and the local build system. See `mach artifact toolchain`. He has previously spoke of having configure download toolchains from automation and manage them - kind of like how artifact builds work except done at the configure level. I got the sense that his patches before he left were building up to this. But I'm not sure how far along he is or what his current vision is. You should definitely reach out to him to see how this bug relates to his in-progress work.

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Gregory Szorc [:gps] from comment #22)
> Before he left on holiday, glandium was working on tighter integration
> between toolchains built in automation and the local build system. See `mach
> artifact toolchain`. He has previously spoke of having configure download
> toolchains from automation and manage them - kind of like how artifact
> builds work except done at the configure level. I got the sense that his
> patches before he left were building up to this. But I'm not sure how far
> along he is or what his current vision is. You should definitely reach out
> to him to see how this bug relates to his in-progress work.

Ah, maybe that's why glandium was grousing at me to use tooltool properly for the Stylo bootstrap work.  ni? him to explain his glorious vision, then.

Comment 24

[Chris Peterson [:cpeterson]]

Nathan or glandium, any updates on this Stylo bootstrap work?

Comment 25

[Chris Peterson [:cpeterson]]

This bug is one of our few remaining blockers for building Stylo by default (but preffed off) in Nightly. Nathan said he would work on it this week.

IIUC, we just need to print an error message telling developers to rerun `mach bootstrap` if libclang isn't installed. Do we have a generic "bootstrap clobber" mechanism to force developers to rerun `mach bootstrap`? We wouldn't need to check ~/.mozbuild for libclang if we knew that people ran `mach bootstrap` (after bootstrap has been fixed to install libclang by default).

Comment 26

[Chris Peterson [:cpeterson]]

> Would you like to download packages for working on Stylo?  If you're not sure, select "No".

mach bootstrap currently asks developers if they would like to install the optional clang packages for Stylo (bug 1314355). We need to remove the prompt and make clang a required package, though there is still debate in bug 1363655 about whether we should allow developers to use their system libclang instead of tooltool's custom clang packages.

Comment 27

[Nathan Froyd [:froydnj]]

Attachment: part 1 - download LLVM/clang 4.0.1 for OS X in `mach bootstrap`

Using 4.0.1 fixes several hangs that people have experienced with our
3.9.0 packages.

This package was built via the following try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e40226331cfbc5b3ce25eb78891363ab200ef39

and the Mac package was repacked as a tar.bz2, since xz doesn't come standard
on Macs.  I also uploaded the Linux package from the same push, in case we want
to update the cross builds or the packages we download for Linux.

Open to other places to put the manifest, if you think another place would be
better.

Comment 28

[Nathan Froyd [:froydnj]]

Attachment: part 2 - always download packages for stylo during `mach bootstrap`

With configure now being able to auto-detect the presence of a `mach
bootstrap`-installed clang and libclang and the upcoming
build-by-defaultness of Stylo, we can start downloading these packages
all the time.

Comment 29

[Ralph Giles (:rillian)]

Comment on attachment 8880524 [details] [diff] [review]
part 1 - download LLVM/clang 4.0.1 for OS X in `mach bootstrap`

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

::: python/mozboot/mozboot/stylo.py
@@ +4,5 @@
>  
>  from __future__ import print_function, unicode_literals
>  
>  WINDOWS = 'browser/config/tooltool-manifests/win64/clang.manifest'
> +OSX = 'browser/config/tooltool-manifests/macosx64/bootstrap.manifest'

I would have just update macosx64/releng.manifest. Are you expecting the non-cross manifests to be removed? I guess this is less confusing.

Comment 30

[Ralph Giles (:rillian)]

Comment on attachment 8880525 [details] [diff] [review]
part 2 - always download packages for stylo during `mach bootstrap`

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

Yay!

Comment 31

[Nathan Froyd [:froydnj]]

Attachment: part 1 - install llvm homebrew/macports packages

LLVM/clang is needed for Stylo's bindgen, and Apple's clang is unusable
for such purposes.  For other platforms, we have installed LLVM/clang
from our tooltool archive on the supposition that those packages are
definitely known to work, as we use said packages in automation.  For
Mac, however, we haven't been able to generate packages for tooltool
that successfully build Stylo, and even if we had, those packages would
solely be used for developer builds of Stylo and would not be used in
automation.  The case for downloading LLVM/clang for Mac from tooltool,
therefore, is not as strong as for other platforms.

As a result, we'll rely on the installed package manager for LLVM/clang,
which many people may have installed anyway.

In passing, also delete some old code for OS X versions < 10.7; such
platforms are no longer supported for running or building Firefox.

Comment 32

[Nathan Froyd [:froydnj]]

Attachment: part 2 - search for Homebrew's llvm-config when appropriate

Since Homebrew doesn't automatically place clang and associated binaries
on PATH, we need to do the task ourselves.

I have not added the equivalent code for MacPorts because I haven't
played with MacPorts at all.  If you have suggestions since you've been
twiddling with the MacPorts bits, I would appreciate that!

Comment 33

[Nathan Froyd [:froydnj]]

Attachment: part 3 - don't install clang packages through tooltool for OS X `mach bootstrap`

We decided to install the necessary packages through the appropriate Mac
package manager instead.

Comment 34

[Nathan Froyd [:froydnj]]

Attachment: part 4 - always download packages for stylo during `mach bootstrap`

With configure now being able to auto-detect the presence of a `mach
bootstrap`-installed clang and libclang and the upcoming
build-by-defaultness of Stylo, we can start downloading these packages
all the time.

Already reviewed as part 2 previously, carrying over r+.

Comment 35

[Ralph Giles (:rillian)]

Comment on attachment 8883096 [details] [diff] [review]
part 1 - install llvm homebrew/macports packages

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

::: python/mozboot/mozboot/osx.py
@@ +406,5 @@
>          self.run_as_root([self.port, 'select', '--set', 'python', 'python27'])
>  
>      def ensure_macports_browser_packages(self, artifact_mode=False):
>          # TODO: Figure out what not to install for artifact mode
> +        packages = ['yasm', 'llvm']

For macports we need 'llvm-4.0', 'clang-4.0'. The packages all have versioned names instead of the @x.y suffix homebrew uses, and llvm installs llvm-config, but not libclang.

Comment 36

[Ralph Giles (:rillian)]

Comment on attachment 8883097 [details] [diff] [review]
part 2 - search for Homebrew's llvm-config when appropriate

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

::: toolkit/moz.configure
@@ +701,5 @@
> +            brew_config = subprocess.check_output([brew, 'config']).strip()
> +
> +            for line in brew_config.splitlines():
> +                if line.startswith('HOMEBREW_PREFIX'):
> +                    fields = line.split()

You might want to `line.split(None, 2)` here in case there are spaces in the path.

Comment 37

[Ralph Giles (:rillian)]

Comment on attachment 8883098 [details] [diff] [review]
part 3 - don't install clang packages through tooltool for OS X `mach bootstrap`

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

::: python/mozboot/mozboot/stylo.py
@@ +4,5 @@
>  
>  from __future__ import print_function, unicode_literals
>  
>  WINDOWS = 'browser/config/tooltool-manifests/win64/clang.manifest'
> +OSX = 'browser/config/tooltool-manifests/macosx64/bootstrap.manifest'

Do we still need a separate bootstrap.manifest?

Comment 38

[Ralph Giles (:rillian)]

macports helpfully has `/opt/local/bin/llvm-config-mp-4.0` whose --libdir points to the directory with libclang.dylib. If one is using macports, that should probably be in path, so we can just add the -mp- variant to the list of llvm-config filenames to try.

Comment 39

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #37)
> ::: python/mozboot/mozboot/stylo.py
> @@ +4,5 @@
> >  
> >  from __future__ import print_function, unicode_literals
> >  
> >  WINDOWS = 'browser/config/tooltool-manifests/win64/clang.manifest'
> > +OSX = 'browser/config/tooltool-manifests/macosx64/bootstrap.manifest'
> 
> Do we still need a separate bootstrap.manifest?

We don't!  I thought I removed that, but apparently rebasing proved me otherwise.  I can remove this.

Comment 40

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #38)
> macports helpfully has `/opt/local/bin/llvm-config-mp-4.0` whose --libdir
> points to the directory with libclang.dylib. If one is using macports, that
> should probably be in path, so we can just add the -mp- variant to the list
> of llvm-config filenames to try.

Thanks for the macports pointers!  OK to just roll that addition into part 2 along with the homebrew search logic?

Comment 41

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd [:froydnj] from comment #40)

> Thanks for the macports pointers!  OK to just roll that addition into part 2
> along with the homebrew search logic?

That's fine with me. I'll test it once the patches land and make sure it works.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a97a60a3dd5
part 1 - install llvm homebrew/macports packages; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c0fd2f12ad9
part 2 - search for Homebrew and MacPorts's llvm-config when appropriate; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/9acf7300c99a
part 3 - don't install clang packages through tooltool for OS X `mach bootstrap`; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c0d2df0b76
part 4 - always download packages for stylo during `mach bootstrap`; r=rillian

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a97a60a3dd5
https://hg.mozilla.org/mozilla-central/rev/3c0fd2f12ad9
https://hg.mozilla.org/mozilla-central/rev/9acf7300c99a
https://hg.mozilla.org/mozilla-central/rev/c8c0d2df0b76

Comment 44

[Jan Beich]

Comment on attachment 8883099 [details] [diff] [review]
part 4 - always download packages for stylo during `mach bootstrap`

> -            choice = self.instance.prompt_int(
[...]
> +            self.instance.ensure_stylo_packages(state_dir, checkout_root)

If you're removing the prompt why Stylo is still limited to interactive-only bootstrap?

Comment 45

[Chris Peterson [:cpeterson]]

(In reply to Jan Beich from comment #44)
> If you're removing the prompt why Stylo is still limited to interactive-only
> bootstrap?

Nathan ^

Comment 46

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson [:cpeterson] from comment #45)
> (In reply to Jan Beich from comment #44)
> > If you're removing the prompt why Stylo is still limited to interactive-only
> > bootstrap?
> 
> Nathan ^

Because I failed to notice that, whoops!  File a new bug?