Add build-time dependency on libclang 3.9 so we can run Stylo bindgen at build-time

NEW
Assigned to

Status

()

Core
Build Config
P1
normal
7 months ago
6 days ago

People

(Reporter: cpeterson, Assigned: froydnj, NeedInfo)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Comment hidden (empty)
(Reporter)

Comment 1

7 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

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

Comment 3

7 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

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

Comment 7

7 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

7 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

7 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

7 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

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

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

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

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

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

Comment 23

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

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

Comment 25

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

16 days ago
Depends on: 1364588
Depends on: 1364458
(Reporter)

Updated

7 days ago
Blocks: 1363655
(Reporter)

Updated

6 days ago
Depends on: 1333036
You need to log in before you can comment on or make changes to this bug.