Closed Bug 1419892 Opened 2 years ago Closed 2 years ago

Linkage in Tup

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 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Depends on: 1419908
Maybe you could kill all the expandlibs stuff first.
Depends on: 1423815
Depends on: 1429875
Product: Core → Firefox Build System
Assignee: nobody → cmanchester
Duplicate of this bug: 1319224
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.
Attachment #8961582 - Flags: review?(mshal) → review+
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.
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25bffacec257
Link programs and libraries in the tup backend. r=mshal
https://hg.mozilla.org/mozilla-central/rev/25bffacec257
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.