1419892 - Linkage in Tup

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Chris Manchester (limited bugmail, email directly)]

What we see from the emitter for libraries and programs has almost everything we need to build out link command lines.

There's a small amount of logic to aggregate linked libraries we'll want to replicate or consolidate from recursivemake.py.

There are also several variables in rules.mk and config.mk that are contributing to the link command line, for instance EXPAND_* variables, that can be derived from the data in mozbuild, but might be better off moved to the frontend where possible.

Comment 1

[Mike Hommey [:glandium]]

Maybe you could kill all the expandlibs stuff first.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1419892 - Link programs and libraries in the tup backend.

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

Comment 4

[Michael Shal [:mshal]]

Comment on attachment 8961582 [details]
Bug 1419892 - Link programs and libraries in the tup backend.

https://reviewboard.mozilla.org/r/230408/#review236736

Looks good! This should be fine to land after the issues are addressed.

::: python/mozbuild/mozbuild/backend/tup.py:35
(Diff revision 1)
>      GeneratedFile,
>      GeneratedSources,
>      HostDefines,
>      HostSources,
>      JARManifest,
> +    Library,

This is unused (or do we need it later for libxul?)

::: python/mozbuild/mozbuild/backend/tup.py:48
(Diff revision 1)
>      VariablePassthru,
>  )
>  from ..util import (
>      FileAvoidWrite,
>      expand_variables,
> +    mkdir,

This is unused.

::: python/mozbuild/mozbuild/backend/tup.py:269
(Diff revision 1)
> +        objs, _, shared_libs, os_libs, static_libs = self._expand_libs(backend_file.shared_lib)
> +        static_libs = self._lib_paths(backend_file.objdir, static_libs)
> +        shared_libs = self._lib_paths(backend_file.objdir, shared_libs)
> +
> +        list_file_name = '%s.list' % backend_file.shared_lib.name.replace('.', '_')
> +        list_file = self._make_list_file(backend_file.objdir, objs, list_file_name)

For some reason these .list files don't appear in objdir/backend.TupBackend, though it looks like they should since _make_file_list calls _write_file. Any idea why they aren't there?

::: python/mozbuild/mozbuild/backend/tup.py:348
(Diff revision 1)
> +
> +        objs, _, shared_libs, _, static_libs = self._expand_libs(backend_file.static_lib)
> +        static_libs = self._lib_paths(backend_file.objdir, static_libs)
> +        shared_libs = self._lib_paths(backend_file.objdir, shared_libs)
> +
> +        inputs = objs + shared_libs + static_libs

I think we only want objs + static_libs here. Right now .so files are getting added to eg: libjs_static.a. The make backend's version only has .o files in it.

::: python/mozbuild/mozbuild/backend/tup.py:359
(Diff revision 1)
> +
> +        backend_file.rule(
> +            cmd=cmd,
> +            inputs=inputs,
> +            outputs=[backend_file.static_lib.name],
> +            display='AR %f'

We should probably make this 'AR %o' so that it just displays the output file instead of all the inputs. For example, libjs_static.a has a bunch of inputs, so the display fills the screen with filenames. The full list is always displayed when using verbose mode, so developers can see what's included in the command-line if necessary.

::: python/mozbuild/mozbuild/backend/tup.py:457
(Diff revision 1)
>  
>          for objdir, backend_file in sorted(self._backend_files.items()):
> +            backend_file.gen_sources_rules([self._installed_files])
> +            if any([backend_file.shared_lib, backend_file.static_lib,
> +                    backend_file.program]):
> +                backend_file.export_shell()

This appears to export the shell variables in cases where no link rules are actually generated. For example, accessible/aom/Tupfile exports those variables and has no actual rules afterward in the Tupfile to use them.

I'm not marking this an issue since it's not a huge deal. Also we could consider just always exporting SHELL/COMSPEC/MOZILLABUILD in every Tupfile rather than trying to target specific rules with it. The downside to that approach is if any of those variables change, the entire tree will rebuild. It's possible that effectively happens already anyway, though. Just something to keep in mind in case those exports become an annoyance.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8961582 [details]
Bug 1419892 - Link programs and libraries in the tup backend.

https://reviewboard.mozilla.org/r/230408/#review236736

> For some reason these .list files don't appear in objdir/backend.TupBackend, though it looks like they should since _make_file_list calls _write_file. Any idea why they aren't there?

Hmm... weirdly I have them in my local `backend.TupBackend`.

> This appears to export the shell variables in cases where no link rules are actually generated. For example, accessible/aom/Tupfile exports those variables and has no actual rules afterward in the Tupfile to use them.
> 
> I'm not marking this an issue since it's not a huge deal. Also we could consider just always exporting SHELL/COMSPEC/MOZILLABUILD in every Tupfile rather than trying to target specific rules with it. The downside to that approach is if any of those variables change, the entire tree will rebuild. It's possible that effectively happens already anyway, though. Just something to keep in mind in case those exports become an annoyance.

Right, I guess we're not going to end up generating rules for static libs that aren't `NO_EXPAND_LIBS`... I'll refactor this slightly to take that into account.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8961582 [details]
Bug 1419892 - Link programs and libraries in the tup backend.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25bffacec257
Link programs and libraries in the tup backend. r=mshal

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/25bffacec257