Closed
Bug 1224450
(fast-compiledb)
Opened 7 years ago
Closed 7 years ago
Make the CompileDB backend work off moz.build data instead of recursing the make build system
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox47 fixed)
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: glandium, Unassigned)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files)
The way the CompileDB backend works at the moment is that it essentially goes through all directories and does make showbuild for all of them, and this is very slow. There are various things to overcome to make this possible, so this is likely to become a forest of bug dependencies. The goal is also not limited to the CompileDB backend: the real goal is to feed the FasterMake backend with everything needed to build C++, but making CompileDB faster is a good proxy for that because it forces to do all the things required to make the end goal actually happen. I have done preliminary work (in the form of a very awful hack) that is able to generate, for a linux opt build, about 70% of the same command lines the current CompileDB backend generates. I will file dependent bugs according to my findings from this preliminary work.
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Alias: fast-compiledb
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 1•7 years ago
|
||
The current status is that I'm able to generate 99.51% (2023 of 2033) of all target compile commands (so, no host compile) on a linux64 non-debug build. The remaining ones are: - build/unix/elfhack/dummy.c - build/unix/elfhack/test-array.c - build/unix/elfhack/test-ctors.c - $objdir/build/unix/elfhack/inject/x86_64.c Those are actually tricky for multiple reasons, and I will ignore them for now. - toolkit/xre/nsAppRunner.cpp - toolkit/xre/nsEmbedFunctions.cpp - toolkit/xre/ProfileReset.cpp - $objdir/toolkit/xre/Unified_cpp_toolkit_xre0.cpp All those add something like `-DTOOLKIT_EM_VERSION="46.0a1" -DGRE_BUILDID=20151230084421` through their Makefile. - webapprt/gtk/webapprt.cpp This one adds something like `-DGRE_BUILDID=20151230084421` through its Makefile. But it's set to go away soon, since webapprt is going to be removed. - browser/app/nsBrowserApp.cpp This one adds `-DPBMODE_ICO="../../dist/branding/pbmode.ico" -DNEWTAB_ICO="../../dist/branding/newtab.ico" -DNEWWINDOW_ICO="../../dist/branding/newwindow.ico" '-DDOCUMENT_ICO="../../dist/branding/document.ico" -DFIREFOX_ICO="../../dist/branding/firefox.ico"`, but those defines are actually not used for the C++ file, only for the RC file in the same directory.
Reporter | ||
Updated•7 years ago
|
Blocks: fastermake
Reporter | ||
Comment 3•7 years ago
|
||
Time for a status update: above 98% of target C, C++, ObjC, ObjC++ on Android, Mac and Linux. (no rust, and no host compilations ; the former is not currently supported by CompileDB anyways, and the latter is supported, but plenty wrong) The closest to 100% is Android, where it's down to 4 files, all exclusively because of -DGRE_BUILDID.
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #3) > The closest to 100% is Android, where it's down to 4 files, all exclusively > because of -DGRE_BUILDID. (with a yet-to-be-filed patch that I'm thoroughly testing on try because it removes a Makefile containing a workaround that I think is not necessary anymore, but needs douple checking in different configurations)
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 5•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/34399/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34399/
Attachment #8718092 -
Flags: review?(gps)
Reporter | ||
Comment 6•7 years ago
|
||
There are a few difficult directories to handle, with limited usefulness compared to having the CompileDB properly filled for everything else in a timely manner, so skip them for now. Review commit: https://reviewboard.mozilla.org/r/34401/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34401/
Attachment #8718093 -
Flags: review?(gps)
Reporter | ||
Comment 7•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/34403/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34403/
Attachment #8718094 -
Flags: review?(gps)
Reporter | ||
Comment 8•7 years ago
|
||
The moz.build data is now sufficient to, with some convolution, generate the same compilation database that recursing the tree with the showbuild target does. The resulting code is not the prettiest, and exposes the shortcomings of the current moz.build data model. It is however a first step towards fixing those shortcomings, because they are now more clearly identified. This was validated on all platforms on try by checking the output of mach build-backend -b CompileDB -d -n is empty when backing out the patch after running mach build-backend -b CompileDB once. Review commit: https://reviewboard.mozilla.org/r/34405/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34405/
Attachment #8718095 -
Flags: review?(gps)
Updated•7 years ago
|
Attachment #8718092 -
Flags: review?(gps) → review+
Comment 9•7 years ago
|
||
Comment on attachment 8718092 [details] MozReview Request: Bug 1224450 - Ignore host compilations in the CompileDB backend. r=gps https://reviewboard.mozilla.org/r/34399/#review31131
Comment 10•7 years ago
|
||
Comment on attachment 8718093 [details] MozReview Request: Bug 1224450 - Skip difficult directories in the CompileDB. r=gps https://reviewboard.mozilla.org/r/34401/#review31133 Sure.
Attachment #8718093 -
Flags: review?(gps) → review+
Comment 11•7 years ago
|
||
Comment on attachment 8718094 [details] MozReview Request: Bug 1224450 - Ignore COMPILE_PDB_FLAG for the CompileDB. r=gps https://reviewboard.mozilla.org/r/34403/#review31137
Attachment #8718094 -
Flags: review?(gps) → review+
Comment 12•7 years ago
|
||
Comment on attachment 8718095 [details] MozReview Request: Bug 1224450 - Make the CompileDB derive its commands from the moz.build data. r=gps https://reviewboard.mozilla.org/r/34405/#review31143 Very, very nice. It's really difficult to audit that all this behaves like all the logic in config.mk and rules.mk. However, verifying the output didn't change as a result of this patch is verification enough for me.
Attachment #8718095 -
Flags: review?(gps) → review+
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5160753dedb2 https://hg.mozilla.org/integration/mozilla-inbound/rev/bc6f6b34333e https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ff3274d147 https://hg.mozilla.org/integration/mozilla-inbound/rev/241e53cd1012
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5160753dedb2 https://hg.mozilla.org/mozilla-central/rev/bc6f6b34333e https://hg.mozilla.org/mozilla-central/rev/c7ff3274d147 https://hg.mozilla.org/mozilla-central/rev/241e53cd1012
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•