1656141 - Stop using MT to insert manifests in binaries

Status: RESOLVED FIXED

(Firefox Build System :: General, task)

Description

[Mike Hommey [:glandium]]

There are essentially 2 ways the build system currently builds manifests into binaries:

• through a reference to the manifest in a .rc file
• through calling mt for manifest files matching the name of the binary with the .manifest suffix.

The interesting thing with this strategy is that in some cases, like mozglue.dll, firefox.exe, xpcshell.exe, etc., we actually do both.

There is actually a third way to handle this, which is to pass the right flags to link.exe (lld-link.exe), which also avoids requiring mt (which is good, because we're currently requiring MSVC's mt and for cross builds have to use wine, and llvm-mt doesn't support what we use mt for).

Comment 1

[Mike Hommey [:glandium]]

Uniformization is probably too much for now.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Remove EMBED_MANIFEST_AT.

It was only ever set to the same value as its default, except in
comm-central, where it is unset, but in directories that now don't link
anything (they did back when binary components were a thing).

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Stop using MT to insert manifests in binaries.

There are several things going on here:

• Replace the use of MT post-linking to passing the same manifest
  manually to the linker. While we passed a resource ID to MT (1 for
  programs, 2 for libraries), these values are the default when using
  -MANIFEST:EMBED, so we just skip giving an ID.
• At the same time, add an explicit dependency on the manifest for the
  link, which removes the need for EXTRA_DEPS related to manifests.
• The linker actually doesn't like when a resource file sets a manifest
  with the same ID as the manifest it's given, so we need to remove
  the reference to the manifest in those resource files.
• In some cases, the reference to the manifest was the only thing in the
  resource file. In practice that means we were adding the manifest
  through the resource file and then re-adding it with MT. We're now
  only adding it via the linker.
• In the case of updater.exe.manifest, because we're using the file in
  two places, and only for one of them the link rules would find the
  manifest, and because both places use the same resource file, and it
  would be confusing if one place had a manifest defined in the
  resource file and one not, rename the resource file so that it's not
  picked up by the link rule.
• In the case of DefaultBrowserAgent, the manifest file didn't match the
  program name, so it required extra manual work, and by renaming it, we
  simplify things a little.

Ideally, the manifests would be declared in moz.build, but that
complicates things for cases like TestDllInterceptor, where there are
multiple binaries in the same directory, but only one of them needs the
manifest. This keeps the status quo of getting the manifest
automatically from the source directory, while simplifying things
(slightly), and more importantly, removing the use of MT, which, in
cross-builds, required wrapping with wine.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Remove EXTRA_DEPS now that it is unused.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Remove dav1d.rc.

RCFILE is only actually used when the moz.build that contains it defines
a binary, which is not the case for dav1d, which ends up in gkmedias.dll.
Which also means that moving the definition to gkmedias would also not
make sense, since all dav1d.rc does is add descriptors to the dll that it
contains dav1d and what version, but gkmedias.dll contains other things
too.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Rename version_win.py to create_rc.py.

Because while the original perl script was added to add version info
to Windows binaries, it does more and will do even more with upcoming
changes.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Remove RESFILE.

The resource file is always generated so being able to configure its name
is not useful. On the other hand, the way things are currently implemented,
the lack of RESFILE also makes RCFILE ignored, which we fix at the same
time.

And remove a spurious RESFILE in widget/windows/moz.build, where no binary
is produced, which means RESFILE had no meaning.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Create res and rc files based on the name of the binary they are linked into.

This will allow creating separate res files for e.g. SIMPLE_PROGRAMS.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Remove rc include files that only add a manifest.

That was redundant with the manifest being added with MT (although useful
for mingw builds, that don't use MT), but is also now handled by the
automatic creation of rc files.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Remove redundant manifest references in rc include files.

The build system will add these manifests whether they are referenced
explicitly in theses rc include files or not.

Comment 11

[Phabricator Automation]

Comment on attachment 9166998 [details]
Bug 1656141 - Remove EXTRA_DEPS now that it is unused.

Revision D85383 was moved to bug 1498414. Setting attachment 9166998 [details] to obsolete.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Bug 1656141 - Remove support for manifests on host programs.

We're currently not using the feature, and host programs ought only
to be used during the build, so I don't expect the feature to ever
be necessary.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b133eda46aa
Remove EMBED_MANIFEST_AT. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/de8899453150
Remove dav1d.rc. r=achronop
https://hg.mozilla.org/integration/autoland/rev/704f28486bda
Rename version_win.py to create_rc.py. r=firefox-build-system-reviewers,mhentges,rstewart
https://hg.mozilla.org/integration/autoland/rev/29653ea85d49
Remove RESFILE. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/ce6831acb5e3
Create res and rc files based on the name of the binary they are linked into. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/194a994cf9c9
Remove support for manifests on host programs. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/eb450457a9b7
Stop using MT to insert manifests in binaries. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/e43dd57dc61a
Remove rc include files that only add a manifest. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/9033b0400339
Remove redundant manifest references in rc include files. r=firefox-build-system-reviewers,rstewart

Comment 14

[Alexandru Michis [:malexandru]]

Backed out 9 changesets for causing multiple failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/9b48053f30035789d9c05aa2847c70b9bb892103

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&tochange=9b48053f30035789d9c05aa2847c70b9bb892103&fromchange=d1268fb4ae8ae803af87ca0c127e292bf6a8f930&selectedTaskRun=JyUXSmSFRaWmmD0R59BEMw.0

Failure logs:

• Spidermonkey: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312312655&repo=autoland&lineNumber=4370
• Windows asan build: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312312971&repo=autoland&lineNumber=31005
• "UnboundLocalError": https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312317272&repo=autoland&lineNumber=1260
• "No tests run or test summary not found": https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312319179&repo=autoland&lineNumber=1025

(Update) Also seeing xpcshell and mozrunner failures:

• XPC: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312317259&repo=autoland&lineNumber=6269
• mozrunner:
  https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312317670&repo=autoland&lineNumber=1052
  https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312317654&repo=autoland&lineNumber=968

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/363f9f07a3d4
Remove EMBED_MANIFEST_AT. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/c505d45152cb
Remove dav1d.rc. r=achronop
https://hg.mozilla.org/integration/autoland/rev/dc8c25ba621c
Rename version_win.py to create_rc.py. r=firefox-build-system-reviewers,mhentges,rstewart
https://hg.mozilla.org/integration/autoland/rev/e6aeae4a0406
Remove RESFILE. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/778d638bc2b5
Create res and rc files based on the name of the binary they are linked into. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/b512bdbba96c
Remove support for manifests on host programs. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/2b521f292b35
Stop using MT to insert manifests in binaries. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/d29d863efda9
Remove rc include files that only add a manifest. r=firefox-build-system-reviewers,rstewart
https://hg.mozilla.org/integration/autoland/rev/6661b7ac99c4
Remove redundant manifest references in rc include files. r=firefox-build-system-reviewers,rstewart

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/363f9f07a3d4
https://hg.mozilla.org/mozilla-central/rev/c505d45152cb
https://hg.mozilla.org/mozilla-central/rev/dc8c25ba621c
https://hg.mozilla.org/mozilla-central/rev/e6aeae4a0406
https://hg.mozilla.org/mozilla-central/rev/778d638bc2b5
https://hg.mozilla.org/mozilla-central/rev/b512bdbba96c
https://hg.mozilla.org/mozilla-central/rev/2b521f292b35
https://hg.mozilla.org/mozilla-central/rev/d29d863efda9
https://hg.mozilla.org/mozilla-central/rev/6661b7ac99c4