Closed Bug 1076743 Opened 5 years ago Closed 5 years ago

Fixed angle compilation with mingw.

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jacek, Assigned: jacek)

Details

Attachments

(3 files)

Attached patch moz.build partSplinter Review
Upstream fixes are already reviewed and on their way to angle:
https://chromium-review.googlesource.com/#/c/220761/

On Mozilla side, moz.build files also need a few fixes:

- EXTRA_DSO_LDOPTS in libEGL/moz.build is not needed, because it already has USE_LIBS
- Use OS_LIBS instead of EXTRA_DSO_LDOPTS in libGLESv2/moz.build
- ANGLE_PRELOADED_D3DCOMPILER_MODULE_NAMES is problematic when passed to windres (resource compiler, rc.exe replacement), which doesn't seem to support " and ' quotes combination correctly. Since renderer/d3d/HLSLCompiler.cpp is the only file needing the define, I just limited it to it.

Here is try push (without ANGLE_PRELOADED_D3DCOMPILER_MODULE_NAMES, which I added later):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6601a4ed2d3f
Attachment #8498747 - Flags: review?(jmuizelaar)
Attached patch upstream partSplinter Review
This is upstream part. AFAIU upstream approval is enough for the review, please let me know if I'm wrong.
Upstream part has been committed to ANGLE.
Comment on attachment 8498747 [details] [diff] [review]
moz.build part

Review of attachment 8498747 [details] [diff] [review]:
-----------------------------------------------------------------

It's probably better to have a build peer review this.
Attachment #8498747 - Flags: review?(jmuizelaar) → review?(gps)
Comment on attachment 8498747 [details] [diff] [review]
moz.build part

Review of attachment 8498747 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/angle/src/libGLESv2/moz.build
@@ +224,5 @@
>  
>  RCFILE = SRCDIR + '/libGLESv2.rc'
>  DEFFILE = SRCDIR + '/libGLESv2.def'
>  
> +SOURCES['renderer/d3d/HLSLCompiler.cpp'].flags += ['-DANGLE_PRELOADED_D3DCOMPILER_MODULE_NAMES=\'{ TEXT("d3dcompiler_47.dll"), TEXT("d3dcompiler_46.dll"), TEXT("d3dcompiler_43.dll") }\'']

This block is weird: it appears to be referencing a .dll on all platforms, not just Windows. But it existed before, so I guess it is OK. I appreciate you moving this to a per-source cflag as opposed to global.
Attachment #8498747 - Flags: review?(gps) → review+
Thanks for the review. The whole dir is Windows only, so I think it's fine.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d22e3162123
https://hg.mozilla.org/integration/mozilla-inbound/rev/80bf7cee307e
https://hg.mozilla.org/mozilla-central/rev/80bf7cee307e
https://hg.mozilla.org/mozilla-central/rev/0d22e3162123
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(In reply to Jacek Caban from comment #1)
> Created attachment 8498749 [details] [diff] [review]
> upstream part
> 
> This is upstream part. AFAIU upstream approval is enough for the review,
> please let me know if I'm wrong.

Changes to ANGLE should go through our updating process:
http://hg.mozilla.org/mozilla-central/file/0c8ae792f1c0/gfx/angle/README.mozilla

As it stands these changes are likely to get unintentionally backed out during the next ANGLE update. Maybe that's ok because they'll come back for the next one...
Yes, for ANGLE changes it was fine, because they were upstreamed. However, I didn't notice that moz.build files are also generated by a script living only in mozilla branch of angle, so they were backed out during update. The attached patch fixes that. Generated files are exactly the same as with patches committed to m-c earlier. I don't know the process to get it to github repo.
Attachment #8503599 - Flags: review?(jmuizelaar)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8503599 [details] [diff] [review]
generate_mozbuild.diff

Review of attachment 8503599 [details] [diff] [review]:
-----------------------------------------------------------------

I landed this fix on inbound in bug 1089199 and have pushed it to the github repo
Attachment #8503599 - Flags: review?(jmuizelaar) → review+
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.