47 bytes, text/x-phabricator-request
|Details | Review|
I hit the problem with clang build that no longer work after bug 1496179 due to missing mozglue.dll manifest (we were missing all resources, but with bug 1506450 fixed that's no longer the case). The problem I found is, however, not exactly limited to mingw clang builds. mozglue.rc includes manifest using RT_MANIFEST as resource type, but it doesn't include any system headers. RT_MANIFEST is not a RC script keyword, it's a define inside winuser.rh to integer constant (24). When winuser.rh is not included, resource compiler considers it as a custom string identifier and adds it as resource of type (string) "RT_MANIFEST" instead. Windows expects resources of type (int) 24 to be manifests, so it ignores the manifest. The above is the main problem, but it does not explain why it works on clang-cl build (on which this manifest is ignored as well). I can see that clang-cl builds have two manifests. It contains both the problematic manifest and a second, slightly different, one. I don't see where does the second one come from. Here is a try push of a trivial fix so that mozglue.rc works as intended: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce54ef5a995c790430cb839a2f60bc47046986ed&selectedJob=218355768
I guess the second mafest is embedded by MSMANIFEST_TOOL if it is avilable (e.g. clang-cl builds): https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/config/rules.mk#718-733 The change looks good if it does not break clang-cl builds. It would be better to replace `RT_MANIFEST` with `24` to remove dependency on header files: https://dxr.mozilla.org/mozilla-central/search?q=RT_MANIFEST
(In reply to Masatoshi Kimura [:emk] from comment #1) > I guess the second mafest is embedded by MSMANIFEST_TOOL if it is avilable > (e.g. clang-cl builds): > https://dxr.mozilla.org/mozilla-central/rev/ > c2593a3058afdfeaac5c990e18794ee8257afe99/config/rules.mk#718-733 I see. LLVM provides llvm-mt that we could in theory try to use for mingw builds. I tried that and found that it doesn't support OUTPUTRESOURCE argument, so it would require adding support for that to LLVM first. Adding manifest resources doesn't really seem like something we need a dedicated tool through. .rc files for those are rather trivial, we could generate them from build system and link like any other resources (instead of depending on mt.exe to modify binaries after linking). > The change looks good if it does not break clang-cl builds. It would be > better to replace `RT_MANIFEST` with `24` to remove dependency on header > files: > https://dxr.mozilla.org/mozilla-central/search?q=RT_MANIFEST I will attach a patch doing that for mozglue (I'm not sure about some of other places).
It's a define that needs winuser.rh to be included. MozReview-Commit-ID: LPfJOwnNm6V
You need to log in before you can comment on or make changes to this bug.