Closed
Bug 1429875
Opened 6 years ago
Closed 6 years ago
Remove expandlibs (re-write linkage logic in moz.build)
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
(Depends on 2 open bugs)
Details
Attachments
(5 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
Bug 1429875 - Implement OBJ_SUFFIX overriding for the profile generation phase on linux in mozbuild.
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
This isn't a strict blocker for bug 1419892, but I have a prototype grade version of linkage for that bug doesn't use expandlibs that is worth extending to other platforms. The general idea will be to associate sources, object files, and libraries in mozbuild (mostly the emitter, a bit in the backend), to the extent we can write out list files and have close to a final command line for linking from the build backend.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → cmanchester
Assignee | ||
Comment 1•6 years ago
|
||
glandium: this a pretty big change that removes some things you implemented, so I'd appreciate your taking an initial look. I can redirect to the shared queue for final review if you prefer.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
Hm, I just realized I didn't update link.py, and it wasn't therefore used in any of my try builds. I wonder why my pgo builds didn't fail, or whether Taskcluster cares about an output timeout at all...
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Chris Manchester (:chmanchester) from comment #8) > Hm, I just realized I didn't update link.py, and it wasn't therefore used in > any of my try builds. I wonder why my pgo builds didn't fail, or whether > Taskcluster cares about an output timeout at all... Filed bug 1440507 to figure out if we can get rid of the linker wrapper.
Assignee | ||
Updated•6 years ago
|
Attachment #8953240 -
Flags: review?(mh+mozilla)
Attachment #8953241 -
Flags: review?(mh+mozilla)
Attachment #8953242 -
Flags: review?(mh+mozilla)
Attachment #8953243 -
Flags: review?(mh+mozilla)
Attachment #8953244 -
Flags: review?(mh+mozilla)
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8953240 [details] Bug 1429875 - Add a "name" property to Library and Program objects that corresponds to the output basename. https://reviewboard.mozilla.org/r/222164/#review229434
Attachment #8953240 -
Flags: review?(mh+mozilla) → review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8953241 [details] Bug 1429875 - Do not take DIST_INSTALL into account when deciding to build static libraries. https://reviewboard.mozilla.org/r/222166/#review229436 ::: python/mozbuild/mozbuild/frontend/data.py:625 (Diff revision 1) > + # If set, dist_install may end up forcing us to build a real > + # static libray, so this is relevant to the backend. > + self.dist_install = context['DIST_INSTALL'] I think we can actually get rid of the DIST_INSTALL = True for static libraries that are NO_EXPAND_LIBS = True. That used to be necessary for the SDK, but we don't ship that anymore. And libraries that *need* to be real static libraries should be using NO_EXPAND_LIBS anyways (and afair, that was only necessary for the standalone xpcom glue, which is now so small that it's not actually necessary anymore ; well, except spidermonkey when we build with --disable-shared-js). Anyways, I think this makes this patch unnecessary.
Attachment #8953241 -
Flags: review?(mh+mozilla)
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8953242 [details] Bug 1429875 - Add a unit test for linkage variables in the make backend. https://reviewboard.mozilla.org/r/222168/#review229438
Attachment #8953242 -
Flags: review?(mh+mozilla) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8953243 [details] Bug 1429875 - Remove expandlibs and instead generate list files in the mozbuild backend. https://reviewboard.mozilla.org/r/222170/#review229440 ::: python/mozbuild/mozbuild/backend/common.py:246 (Diff revision 1) > + shared_libs.append(lib) > + > + add_objs(input_bin) > + > + for lib in input_bin.linked_libraries: > + if isinstance(input_bin, RustLibrary): seems like this test should be outside the loop ::: python/mozbuild/mozbuild/backend/common.py:249 (Diff revision 1) > + > + for lib in input_bin.linked_libraries: > + if isinstance(input_bin, RustLibrary): > + continue > + elif isinstance(lib, StaticLibrary): > + system_libs = not isinstance(input_bin, StaticLibrary) likewise ::: python/mozbuild/mozbuild/backend/recursivemake.py:1321 (Diff revision 1) > - % (pretty_relpath(l), l.import_name)) > - for l in lib.linked_system_libs: > - backend_file.write_once('OS_LIBS += %s\n' % l) > - > def pretty_relpath(lib): > - return '$(DEPTH)/%s' % mozpath.relpath(lib.objdir, topobjdir) > + return mozpath.relpath(lib.objdir, obj.objdir) I think this would be better left alone and changed in an entirely separate bug. ::: python/mozbuild/mozbuild/backend/recursivemake.py:1346 (Diff revision 1) > + (obj.name, > + ' '.join(os.path.relpath(o, obj.objdir) for o in objs))) > + elif not isinstance(obj, StaticLibrary): > + list_file_ref = self._make_list_file(obj.objdir, objs, '%s.list' % > + obj.name.replace('.', '_')) > + backend_file.write_once('%s_OBJS := %s\n' % (obj.name, list_file_ref)) The list file should be added to the corresponding make dependencies. That's also true for all those objects you're linking. If I'm reading all this correctly, you only end up with the object files from the current directory being dependencies of libs/executables.
Attachment #8953243 -
Flags: review?(mh+mozilla)
Updated•6 years ago
|
Product: Core → Firefox Build System
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8953241 [details] Bug 1429875 - Do not take DIST_INSTALL into account when deciding to build static libraries. https://reviewboard.mozilla.org/r/222166/#review234928
Attachment #8953241 -
Flags: review?(mh+mozilla) → review+
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8953243 [details] Bug 1429875 - Remove expandlibs and instead generate list files in the mozbuild backend. https://reviewboard.mozilla.org/r/222170/#review234934
Attachment #8953243 -
Flags: review?(mh+mozilla) → review+
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8953244 [details] Bug 1429875 - Implement OBJ_SUFFIX overriding for the profile generation phase on linux in mozbuild. https://reviewboard.mozilla.org/r/222172/#review234936
Attachment #8953244 -
Flags: review?(mh+mozilla) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8953245 -
Attachment is obsolete: true
Comment 34•6 years ago
|
||
Pushed by cmanchester@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/36f505b0e0da Add a "name" property to Library and Program objects that corresponds to the output basename. r=glandium https://hg.mozilla.org/integration/autoland/rev/4cdc60a9f06e Do not take DIST_INSTALL into account when deciding to build static libraries. r=glandium https://hg.mozilla.org/integration/autoland/rev/decbe4a4cdc7 Add a unit test for linkage variables in the make backend. r=glandium https://hg.mozilla.org/integration/autoland/rev/7547d66e0f51 Remove expandlibs and instead generate list files in the mozbuild backend. r=glandium https://hg.mozilla.org/integration/autoland/rev/cd90d8ccc5be Implement OBJ_SUFFIX overriding for the profile generation phase on linux in mozbuild. r=glandium
Comment 35•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/36f505b0e0da https://hg.mozilla.org/mozilla-central/rev/4cdc60a9f06e https://hg.mozilla.org/mozilla-central/rev/decbe4a4cdc7 https://hg.mozilla.org/mozilla-central/rev/7547d66e0f51 https://hg.mozilla.org/mozilla-central/rev/cd90d8ccc5be
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 36•6 years ago
|
||
A note to myself: I backported this to the last revision before we removed --disable-stylo and it didn't seem to affect the MinGW x86 Build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9733be47bc275a23a776291b99872aecff201e2&selectedJob=171939101
You need to log in
before you can comment on or make changes to this bug.
Description
•