Closed Bug 1429875 Opened 2 years ago Closed 2 years ago

Remove expandlibs (re-write linkage logic in moz.build)

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

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)

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: nobody → cmanchester
Depends on: 1431229
Depends on: 1435096
Depends on: 1435889
Depends on: 1437182
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.
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...
(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.
Depends on: 1440507
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 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 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 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 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)
Product: Core → Firefox Build System
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 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 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+
Attachment #8953245 - Attachment is obsolete: true
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
Depends on: 1447907
Depends on: 1448659
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
Depends on: 1449965
Depends on: 1455504
Blocks: 1487595
You need to log in before you can comment on or make changes to this bug.