Closed
Bug 1310852
Opened 8 years ago
Closed 7 years ago
Make mach bootstrap always install libclang packages for Stylo
Categories
(Firefox Build System :: General, defect, P1)
Firefox Build System
General
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: cpeterson, Assigned: froydnj)
References
Details
Attachments
(4 files, 2 obsolete files)
(deleted),
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•8 years 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 3•8 years ago
|
||
We will need to update `mach bootstrap`, various parts of build automation, and configure checks for libclang.
Comment 4•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
Reporter | ||
Comment 7•8 years ago
|
||
P2 because rust-bindgen and libclang are a high priority but they are not immediate blockers for Stylo.
Priority: -- → P2
Reporter | ||
Comment 8•8 years 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)
Comment 9•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
I /think/ the .dll.a is a .lib, with a different extension. Surely, you can check what dumpbin says about it.
Comment 11•8 years 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.
Comment 12•8 years ago
|
||
I checked the symbols of c2.dll. It doesn't seem to expose libclang's functions we need.
Reporter | ||
Comment 13•8 years 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
Comment 14•8 years ago
|
||
(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•8 years ago
|
||
That would need some hack, though, because rustc hardcoded ".lib" suffix for Windows.
Comment 16•8 years ago
|
||
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
Reporter | ||
Comment 17•8 years 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: stylo-nightly-build
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•8 years 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•8 years 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•8 years 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•8 years ago
|
||
Not super keen on baking those locations into moz.configure, though...
Comment 22•8 years 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•8 years 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 25•7 years 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
Reporter | ||
Comment 26•7 years 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 | |
Comment 27•7 years ago
|
||
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•7 years ago
|
||
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 29•7 years ago
|
||
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 30•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
Attachment #8880524 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 32•7 years ago
|
||
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•7 years ago
|
||
We decided to install the necessary packages through the appropriate Mac package manager instead.
Attachment #8883098 -
Flags: review?(giles)
![]() |
Assignee | |
Comment 34•7 years ago
|
||
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•7 years ago
|
Attachment #8880525 -
Attachment is obsolete: true
Comment 35•7 years ago
|
||
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 36•7 years ago
|
||
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 37•7 years ago
|
||
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+
Comment 38•7 years ago
|
||
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•7 years 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•7 years 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)
Comment 41•7 years ago
|
||
(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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 44•7 years 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•7 years 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)
![]() |
Assignee | |
Comment 46•7 years 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)
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•