Missing resources in mingw builds.

RESOLVED FIXED in Firefox -esr60

Status

defect
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: jacek, Assigned: jacek)

Tracking

(Blocks 1 bug)

Trunk
mozilla65
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox65 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

7 months ago
I recently got an assertion when testing accessibility builds, where we assert because we can't load a manifest from DLL resources. The problem, however, is more general. It looks like all object files are missing resources (a quick test is to see if firefox.exe has its icon in Windows Explorer).

I think this worked before. I found an old build from July that had resources right, so this is a regression.

Comment 1

6 months ago
Hrm. The earliest successful build I can find myself has having sent to try is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae5d52b409ce46a1f469b105e6a4c255270c411f which I recreated at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ef77754a1d898f1f3a28f48b359be26ca975542

It is missing the icon in Windows Explorer.  But yea I agree I seem to remember the icon from some time ago...

Does about:buildconfig on that build give you anything useful to narrow down the mingw or clang version it was built with?
Flags: needinfo?(jacek)
Assignee

Comment 2

6 months ago
Before looking at regression in current build config, I experimented with using llvm-rc (instead of binutils windres) and results seem promising. Technically, we could invoke llvm-rc directly (like we invoke rc.exe in clang-cl builds), but the assumption that mingw uses windres is pretty deep in build system. I used Martin's windres wrapper around llvm-rc from mingw-llvm:
https://github.com/mstorsjo/llvm-mingw/blob/master/wrappers/windres-wrapper.c

Such toolchain generates binaries that include resources:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6983298b55203dc98962fcf38b42ac9658f6e840
Patch for the toolchain looks like this:
https://hg.mozilla.org/try/rev/9c228997570864a286f8cf59f108622465ab1ffa

It has two problems that need to be solved first:
- Windows doesn't like something about the manifest, which prevents firefox.exe to be ran
- llvm-rc doesn't know about EXSTYLE and NOT keywords in .rc files. This should be easy to add, a workaround I used for testing is:
https://hg.mozilla.org/try/rev/42db220ddec6013bb25e0adbda1980e637daad7f
Flags: needinfo?(jacek)
Assignee

Comment 3

6 months ago
EXSTYLE is supported by llvm-rc now:
https://reviews.llvm.org/rL347858
Assignee

Comment 6

6 months ago
We don't need binutils for that anymore (and using binutils windres was broken for some reason).

Comment 7

6 months ago
Pushed by jacek@codeweavers.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e75f80fd784
Bump LLVM and mingw-w64 versions. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dcdfbc80345
Use llvm-rc via mingw-llvm windres wrapper as resource compiler. r=froydnj

Comment 8

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3e75f80fd784
https://hg.mozilla.org/mozilla-central/rev/8dcdfbc80345
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee: nobody → jacek

Comment 9

6 months ago
Here is a backported version; but it may need rebasing depending on what the order of patches landing is.

Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fafd2ead717c43016c2ef1cfcc241ff95f4015b2
Target Milestone: Firefox 65 → mozilla65

Comment 10

5 months ago

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: The mingwclang build will not run without this patch.

User impact if declined: We won't be able to run tests for mingwclang; and tor will have to carry the patch.

Fix Landed on Version: 65.0a1 / 20181206092619

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Affects only mingwclang build

String or UUID changes made by this patch:

Attachment #9030147 - Attachment is obsolete: true
Attachment #9035550 - Flags: approval-mozilla-esr60?

Comment on attachment 9035550 [details] [diff] [review]
Bug 1506450 - Bump LLVM and use llvm-rc via mingw-llvm windres wrapper as resource compiler. (esr60) r=froydnj

compiler update for mingwclang build, approved for 60.5esr

Attachment #9035550 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.