Make mach bootstrap always install libclang packages for Stylo

RESOLVED FIXED in Firefox 56

Status

()

Core
Build Config
P1
normal
RESOLVED FIXED
9 months ago
a day ago

People

(Reporter: cpeterson, Assigned: froydnj)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

9 months ago
Xidorn says we need clang 3.9 or later to handle VC2015's headers.
Summary: Add build-time dependency on libclang so we can run Stylo bindgen at build-time → Add build-time dependency on libclang 3.9+ so we can run Stylo bindgen at build-time
(Reporter)

Comment 2

9 months ago
Emilio says we can stick with clang 3.8 on OS X and Linux.
(Reporter)

Comment 3

9 months ago
We will need to update `mach bootstrap`, various parts of build automation, and configure checks for libclang.
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.
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).
There are no current plans to release a new MozillaBuild, FYI. We've been hoping to be able to switch people over to msys2.
(Reporter)

Updated

9 months ago
Blocks: 1314352
No longer blocks: 1302028
Depends on: 1314355
(Reporter)

Comment 7

9 months ago
P2 because rust-bindgen and libclang are a high priority but they are not immediate blockers for Stylo.
Priority: -- → P2
(Reporter)

Comment 8

9 months ago
(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?
Flags: needinfo?(xidorn+moz)
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
Flags: needinfo?(xidorn+moz)
I /think/ the .dll.a is a .lib, with a different extension. Surely, you can check what dumpbin says about it.

Comment 11

9 months ago
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.
I checked the symbols of c2.dll. It doesn't seem to expose libclang's functions we need.
(Reporter)

Comment 13

9 months ago
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
(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.
That would need some hack, though, because rustc hardcoded ".lib" suffix for Windows.
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
Blocks: 1302028
(Reporter)

Comment 17

3 months ago
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.
Blocks: 1356991
Flags: needinfo?(ted)
Flags: needinfo?(nfroyd)
Priority: P2 → P1
Summary: Add build-time dependency on libclang 3.9+ so we can run Stylo bindgen at build-time → Add build-time dependency on libclang 3.9 so we can run Stylo bindgen at build-time (mach bootstrap llvm39)
(Assignee)

Comment 18

3 months ago
(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.
Flags: needinfo?(ted)
Flags: needinfo?(nfroyd)
(Reporter)

Comment 19

3 months ago
(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?
Flags: needinfo?(nfroyd)
Summary: Add build-time dependency on libclang 3.9 so we can run Stylo bindgen at build-time (mach bootstrap llvm39) → Add build-time dependency on libclang 3.9 so we can run Stylo bindgen at build-time
(Assignee)

Comment 20

3 months ago
(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?
Flags: needinfo?(nfroyd) → needinfo?(gps)
(Assignee)

Comment 21

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

Comment 22

3 months ago
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.
Flags: needinfo?(gps)
(Assignee)

Comment 23

3 months ago
(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.
Flags: needinfo?(mh+mozilla)
(Reporter)

Comment 24

3 months ago
Nathan or glandium, any updates on this Stylo bootstrap work?
(Reporter)

Comment 25

3 months ago
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).
Assignee: nobody → nfroyd
(Assignee)

Updated

2 months ago
Depends on: 1364588
Depends on: 1364458
(Reporter)

Updated

2 months ago
Blocks: 1363655
(Reporter)

Updated

2 months ago
Depends on: 1333036
(Reporter)

Comment 26

2 months ago
> 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.
Summary: Add build-time dependency on libclang 3.9 so we can run Stylo bindgen at build-time → Make mach bootstrap always install libclang packages for Stylo
(Assignee)

Updated

a month ago
Depends on: 1375168
(Assignee)

Comment 27

a month ago
Created attachment 8880524 [details] [diff] [review]
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.
Attachment #8880524 - Flags: review?(giles)
(Assignee)

Comment 28

a month ago
Created attachment 8880525 [details] [diff] [review]
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.
Attachment #8880525 - Flags: review?(giles)
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.
Attachment #8880524 - Flags: review?(giles) → review+
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!
Attachment #8880525 - Flags: review?(giles) → review+
(Assignee)

Comment 31

22 days ago
Created attachment 8883096 [details] [diff] [review]
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.
Attachment #8883096 - Flags: review?(giles)
(Assignee)

Updated

22 days ago
Attachment #8880524 - Attachment is obsolete: true
(Assignee)

Comment 32

22 days ago
Created attachment 8883097 [details] [diff] [review]
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!
Attachment #8883097 - Flags: review?(giles)
(Assignee)

Comment 33

22 days ago
Created attachment 8883098 [details] [diff] [review]
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.
Attachment #8883098 - Flags: review?(giles)
(Assignee)

Comment 34

22 days ago
Created attachment 8883099 [details] [diff] [review]
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+.
Attachment #8883099 - Flags: review+
(Assignee)

Updated

22 days ago
Attachment #8880525 - Attachment is obsolete: true
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.
Attachment #8883096 - Flags: review?(giles) → review+
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.
Attachment #8883097 - Flags: review?(giles) → review+
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?
Attachment #8883098 - Flags: review?(giles) → review+
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.
(Assignee)

Comment 39

20 days ago
(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.
(Assignee)

Comment 40

20 days ago
(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?
Flags: needinfo?(mh+mozilla) → needinfo?(giles)
(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.
Flags: needinfo?(giles)

Comment 42

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

20 days ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 20 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 44

20 days ago
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?
(Reporter)

Comment 45

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

Nathan ^
Flags: needinfo?(nfroyd)
Blocks: 1379341
(Assignee)

Comment 46

a day ago
(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?
Flags: needinfo?(nfroyd)
(Reporter)

Updated

a day ago
Depends on: 1383929
You need to log in before you can comment on or make changes to this bug.