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

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

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 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1429875 - Add a "name" property to Library and Program objects that corresponds to the output basename.

Review commit: https://reviewboard.mozilla.org/r/222164/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222164/

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1429875 - Do not take DIST_INSTALL into account when deciding to build static libraries.

Review commit: https://reviewboard.mozilla.org/r/222166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222166/

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1429875 - Add a unit test for linkage variables in the make backend.

Review commit: https://reviewboard.mozilla.org/r/222168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222168/

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1429875 - Remove expandlibs and instead generate list files in the mozbuild backend.

Review commit: https://reviewboard.mozilla.org/r/222170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222170/

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1429875 - Implement OBJ_SUFFIX overriding for the profile generation phase on linux in mozbuild.

Review commit: https://reviewboard.mozilla.org/r/222172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222172/

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1429875 - Do not land - hacks to preserve object file order

Review commit: https://reviewboard.mozilla.org/r/222174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222174/

Comment 8

[Chris Manchester (limited bugmail, email directly)]

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...

Comment 9

[Chris Manchester (limited bugmail, email directly)]

(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.

Comment 10

[Mike Hommey [:glandium]]

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

Comment 11

[Mike Hommey [:glandium]]

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.

Comment 12

[Mike Hommey [:glandium]]

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

Comment 13

[Mike Hommey [:glandium]]

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.

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953240 [details]
Bug 1429875 - Add a "name" property to Library and Program objects that corresponds to the output basename.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222164/diff/1-2/

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953241 [details]
Bug 1429875 - Do not take DIST_INSTALL into account when deciding to build static libraries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222166/diff/1-2/

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953242 [details]
Bug 1429875 - Add a unit test for linkage variables in the make backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222168/diff/1-2/

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953243 [details]
Bug 1429875 - Remove expandlibs and instead generate list files in the mozbuild backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222170/diff/1-2/

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953244 [details]
Bug 1429875 - Implement OBJ_SUFFIX overriding for the profile generation phase on linux in mozbuild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222172/diff/1-2/

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953245 [details]
Bug 1429875 - Do not land - hacks to preserve object file order

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222174/diff/1-2/

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953240 [details]
Bug 1429875 - Add a "name" property to Library and Program objects that corresponds to the output basename.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222164/diff/2-3/

Comment 21

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953241 [details]
Bug 1429875 - Do not take DIST_INSTALL into account when deciding to build static libraries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222166/diff/2-3/

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953242 [details]
Bug 1429875 - Add a unit test for linkage variables in the make backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222168/diff/2-3/

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953243 [details]
Bug 1429875 - Remove expandlibs and instead generate list files in the mozbuild backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222170/diff/2-3/

Comment 24

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953244 [details]
Bug 1429875 - Implement OBJ_SUFFIX overriding for the profile generation phase on linux in mozbuild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222172/diff/2-3/

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953245 [details]
Bug 1429875 - Do not land - hacks to preserve object file order

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222174/diff/2-3/

Comment 26

[Mike Hommey [:glandium]]

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

Comment 27

[Mike Hommey [:glandium]]

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

Comment 28

[Mike Hommey [:glandium]]

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

Comment 29

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953240 [details]
Bug 1429875 - Add a "name" property to Library and Program objects that corresponds to the output basename.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222164/diff/3-4/

Comment 30

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953241 [details]
Bug 1429875 - Do not take DIST_INSTALL into account when deciding to build static libraries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222166/diff/3-4/

Comment 31

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953242 [details]
Bug 1429875 - Add a unit test for linkage variables in the make backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222168/diff/3-4/

Comment 32

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953243 [details]
Bug 1429875 - Remove expandlibs and instead generate list files in the mozbuild backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222170/diff/3-4/

Comment 33

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8953244 [details]
Bug 1429875 - Implement OBJ_SUFFIX overriding for the profile generation phase on linux in mozbuild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222172/diff/3-4/

Comment 34

[Pulsebot]

Pushed by [e-mail]:
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

[Arthur Iakab [arthur_iakab]]

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

Comment 36

[Tom Ritter [:tjr]]

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