Closed Bug 1076743 Opened 5 years ago Closed 5 years ago

Fixed angle compilation with mingw.


(Core :: Graphics, defect)

Windows 7
Not set





(Reporter: jacek, Assigned: jacek)



(3 files)

Attached patch partSplinter Review
Upstream fixes are already reviewed and on their way to angle:

On Mozilla side, files also need a few fixes:

- EXTRA_DSO_LDOPTS in libEGL/ is not needed, because it already has USE_LIBS
- Use OS_LIBS instead of EXTRA_DSO_LDOPTS in libGLESv2/
- 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):
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] 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] part

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

::: gfx/angle/src/libGLESv2/
@@ +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.
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:

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 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)
Resolution: FIXED → ---
Comment on attachment 8503599 [details] [diff] [review]

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+
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.