Closed Bug 1310852 Opened 4 years ago Closed 3 years ago

Make mach bootstrap always install libclang packages for Stylo

Categories

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

defect

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: cpeterson, Assigned: froydnj)

References

Details

Attachments

(4 files, 2 obsolete files)

No description provided.
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
Emilio says we can stick with clang 3.8 on OS X and Linux.
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.
Blocks: 1314352
No longer blocks: 1302028
Depends on: 1314355
P2 because rust-bindgen and libclang are a high priority but they are not immediate blockers for Stylo.
Priority: -- → P2
(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.
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.
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
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.
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)
(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)
(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
(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)
Not super keen on baking those locations into moz.configure, though...
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)
(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)
Nathan or glandium, any updates on this Stylo bootstrap work?
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
Depends on: 1364588
Depends on: 1364458
Blocks: 1363655
Depends on: 1333036
> 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
Depends on: 1375168
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)
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+
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)
Attachment #8880524 - Attachment is obsolete: true
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)
We decided to install the necessary packages through the appropriate Mac
package manager instead.
Attachment #8883098 - Flags: review?(giles)
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+
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.
(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.
(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)
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 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?
(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
(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)
Depends on: 1383929
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.