ANGLE update broke DX SDK + Win 7 SDK builds

RESOLVED FIXED in Firefox 33

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

unspecified
mozilla34
x86
Windows 8.1
Points:
---

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

The ANGLE update in bug 1010371 broke builds with the DX SDK (June 2010) + Win 7 SDK.  DX SDK is not in the Win 7 SDK.  I *think* all that's needed is to add the direct x SDK's path to INCLUDES/-I if the dx sdk is present.
looks like we also need to disable the D3D11 renderer, because it depends on DXGI 1.2.  So:

if not CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
    CXXFLAGS += [ "-I'" + CONFIG['MOZ_DIRECTX_SDK_PATH'] + "/include'" ]

and disable the d3d11 renderer (files and flags)

Comment 2

4 years ago
Created attachment 8454732 [details] [diff] [review]
1037667.diff

This is how I locally hacked my moz.build files to get it working... I was worried that the sources had to be in alphabetical order so I didn't move anything around. I also don't know if my string manipulation is best practice. (The actual files are generated so this won't be the final patch.)
Attachment #8454732 - Flags: feedback?(gps)
I'll make these changes in the upstream generator code.  libEGL/moz.build doesn't need to know about ANGLE_ENABLE_D3D*, so I'll nuke that stuff out of there as well.

Comment 4

4 years ago
This worked for me after I removed the --disable-webgl switch again, which prevented the DirectX SDK path from being detected at all. Thanks Neil!

Comment 5

4 years ago
Simple way out is to switch to the Win 8.1 SDK if you can.

Comment 6

4 years ago
Comment on attachment 8454732 [details] [diff] [review]
1037667.diff

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

::: gfx/angle/src/libEGL/moz.build
@@ +15,5 @@
>      'Surface.cpp',
>  ]
>  
> +if CONFIG['MOZ_DIRECTX_SDK_PATH'] and not CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
> +    CXXFLAGS += ['-I\'%s/include/\'' % CONFIG['MOZ_DIRECTX_SDK_PATH']]

You should be using LOCAL_INCLUDES for -I arguments.

::: gfx/angle/src/libGLESv2/moz.build
@@ +118,5 @@
> +        'renderer/d3d11/VertexBuffer11.cpp',
> +    ]
> +    EXTRA_DSO_LDOPTS += [ 'd3d9.lib', 'dxguid.lib' ]
> +else:
> +    CXXFLAGS += ['-I\'%s/include/\'' % CONFIG['MOZ_DIRECTX_SDK_PATH']]

LOCAL_INCLUDES

@@ +122,5 @@
> +    CXXFLAGS += ['-I\'%s/include/\'' % CONFIG['MOZ_DIRECTX_SDK_PATH']]
> +    EXTRA_DSO_LDOPTS += [
> +        '\'%s/lib/%s/d3d9.lib\'' % (CONFIG['MOZ_DIRECTX_SDK_PATH'], CONFIG['MOZ_D3D_CPU_SUFFIX']),
> +        '\'%s/lib/%s/dxguid.lib\'' % (CONFIG['MOZ_DIRECTX_SDK_PATH'], CONFIG['MOZ_D3D_CPU_SUFFIX']),
> +    ]

This will likely change after bug 1036894. glandium may have opinions here. Please give him final review so he knows about conflict with his patch, if any.
Attachment #8454732 - Flags: feedback?(gps) → feedback+
Duplicate of this bug: 1038140
(In reply to Gregory Szorc [:gps] from comment #6)
> Comment on attachment 8454732 [details] [diff] [review]
> 1037667.diff
> 
> Review of attachment 8454732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/angle/src/libEGL/moz.build
> @@ +15,5 @@
> >      'Surface.cpp',
> >  ]
> >  
> > +if CONFIG['MOZ_DIRECTX_SDK_PATH'] and not CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
> > +    CXXFLAGS += ['-I\'%s/include/\'' % CONFIG['MOZ_DIRECTX_SDK_PATH']]
> 
> You should be using LOCAL_INCLUDES for -I arguments.

LOCAL_INCLUDES doesn't take file-system absolute paths.
Hmm, the bug I duped also happened with --disable-angle, so you might want to check if that switch actually works.

Comment 10

4 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #9)
> Hmm, the bug I duped also happened with --disable-angle, so you might want
> to check if that switch actually works.

I know that at least --disable-webgl doesn't work anymore, and in fact having the switch in the configuration breaks the build for me. It should probably be removed from the available build options, if there is no interest in supporting builds without the DirectX SDK.
Is there a way around this?

I tried --disable-angle which still fails, and the last m-c I can build successfully is from around 3rd of July (win7, VS2010, DXSDK).
--disable-webgl will go away, but --disable-angle will be made to work.  Avi, the patch in this bug should get you a workaround; did it not work for you?
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #12)
> --disable-webgl will go away, but --disable-angle will be made to work. 
> Avi, the patch in this bug should get you a workaround; did it not work for
> you?

Will try it now and report back. Thanks.
The patch is working for me. Though come to think of it, I also locally modified my startup script to put the dxsdk into %INCLUDE% (which isn't enough by itself). So my test is a little tainted.

It would be very nice to have a cleaned up patch landed.
It compiles for me. I had --disable-angle though I don't know if it took effect or not.

Thanks.
Created attachment 8458851 [details] [diff] [review]
update angle moz.build files

Ok, here's updated moz.build files from the git repo that should fix this.  Roughly the same as what was here before, but updated from the generator tool.
Attachment #8458851 - Flags: review?(jgilbert)
Created attachment 8458852 [details] [diff] [review]
remove --disable-webgl

WebGL is a core part of the web platform now; we should not have a way of disabling it.  At worst, if there is no underlying GL context provider, we should fail to create a webgl context on some platforms.
Attachment #8458852 - Flags: review?(jgilbert)
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #17)
> Created attachment 8458852 [details] [diff] [review]
> remove --disable-webgl
> 
> WebGL is a core part of the web platform now; we should not have a way of
> disabling it.  At worst, if there is no underlying GL context provider, we
> should fail to create a webgl context on some platforms.

FWIW, I've been using that option not to disable webgl per se, but to avoid having to download and install the direct X sdk "just" to do a windows build and check some build system problem. That also allows faster builds in combination for other --disable flags when you're making changes that aren't relevant. Also, I don't see a reason to enforce apps like Lightning to have webgl support.
Created attachment 8459278 [details] [diff] [review]
moz.build update without line ending changes

Attachment 8458851 [details] [diff] additionally corrects the line endings from DOS to Unix. Unfortunately this obscures the actual changes in the patch. This patch is a diff between the previous version with the line endings corrected, and the previous version with attachment 8458851 [details] [diff] [review] applied, thus only showing the actual changes.
Attachment #8459278 - Flags: feedback?(jgilbert)
(In reply to Mike Hommey from comment #18)
> FWIW, I've been using that option not to disable webgl per se, but to avoid
> having to download and install the direct X sdk "just" to do a windows build
> and check some build system problem.

Out of interest, is --disable-angle also supposed to achieve this?
Created attachment 8459297 [details] [diff] [review]
fixed for me

The existing patch doesn't work for me. I've modified a few lines where the patch didn't cope with paths appropriately. Specifically

from

+if CONFIG['MOZ_DIRECTX_SDK_PATH'] and not CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
+    CXXFLAGS += ['-I'%s/include/'' % CONFIG['MOZ_DIRECTX_SDK_PATH']]

to

+if CONFIG['MOZ_DIRECTX_SDK_PATH'] and not CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
+    CXXFLAGS += ['\'-I%s/include/\'' % CONFIG['MOZ_DIRECTX_SDK_PATH']]

and

+    ''%s/lib/%s/d3d9.lib'' % (CONFIG['MOZ_DIRECTX_SDK_PATH'], CONFIG['MOZ_D3D_CPU_SUFFIX']),
+    ''%s/lib/%s/dxguid.lib'' % (CONFIG['MOZ_DIRECTX_SDK_PATH'], CONFIG['MOZ_D3D_CPU_SUFFIX']),

to

+    '\'%s/lib/%s/d3d9.lib\'' % (CONFIG['MOZ_DIRECTX_SDK_PATH'], CONFIG['MOZ_D3D_CPU_SUFFIX']),
+    '\'%s/lib/%s/dxguid.lib\'' % (CONFIG['MOZ_DIRECTX_SDK_PATH'], CONFIG['MOZ_D3D_CPU_SUFFIX']),
Attachment #8459297 - Flags: feedback?(jgilbert)
(In reply to Robert Longson from comment #21)
> The existing patch doesn't work for me. I've modified a few lines where the
> patch didn't cope with paths appropriately.

Ah, it looks like vlad's generator ate some backslashes. Oops.
Whoops, thought I caught all the missing backslashes; missed some.  Will fix shortly and commit, since it looks like it works with those fixes.

(In reply to Mike Hommey [:glandium] from comment #18)
> FWIW, I've been using that option not to disable webgl per se, but to avoid
> having to download and install the direct X sdk "just" to do a windows build
> and check some build system problem. That also allows faster builds in
> combination for other --disable flags when you're making changes that aren't
> relevant.

You actually no longer need the Direct X SDK as long as you have the Win 8 or 8.1 SDKs.  We'll use the DX headers that are present in there.  One of the main reasons for doing a bunch of ANGLE stuff over the past while was to get rid of that old Dx SDK dependency.

> Also, I don't see a reason to enforce apps like Lightning to have webgl support.

That's arguable for many things though; e.g. they probably don't need SPDY support either.  It's simpler to have a consistent Gecko platform.
Created attachment 8460298 [details] [diff] [review]
fixed patch, with diff -w

Ok, updated autogenerated moz.build files, with a full passing tryserver run.  This is the diff -w version of the patch.
Attachment #8454732 - Attachment is obsolete: true
Attachment #8458851 - Attachment is obsolete: true
Attachment #8459278 - Attachment is obsolete: true
Attachment #8459297 - Attachment is obsolete: true
Attachment #8458851 - Flags: review?(jgilbert)
Attachment #8459278 - Flags: feedback?(jgilbert)
Attachment #8459297 - Flags: feedback?(jgilbert)
Attachment #8460298 - Flags: review?(jgilbert)
Attachment #8460298 - Flags: review+
Comment on attachment 8460298 [details] [diff] [review]
fixed patch, with diff -w

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

::: gfx/angle/moz.build
@@ +106,5 @@
>      if CONFIG['CLANG_CXX']:
>          CXXFLAGS += ['-Wno-unused-private-field']
>  
> +if CONFIG['MOZ_DIRECTX_SDK_PATH'] and not CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
> +    CXXFLAGS += ['-I\'%s/include/\'' % CONFIG['MOZ_DIRECTX_SDK_PATH']]

You'll probably find me picky, but really, those escaped quotes shouldn't be here. The right thing to do is to make the recursivemake backend do the quoting itself, not do it in moz.build. (.replace(' ', '\ ') when adding them to backend.mk should be enough)
Attachment #8460298 - Flags: review?(jgilbert) → review-
Comment on attachment 8460298 [details] [diff] [review]
fixed patch, with diff -w

-- a/gfx/angle/moz.build
+++ b/gfx/angle/moz.build

 DEFINES['ANGLE_ENABLE_D3D9'] = True
+if CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
 DEFINES['ANGLE_ENABLE_D3D11'] = True
+

There's an indent missing here.
Comment on attachment 8460298 [details] [diff] [review]
fixed patch, with diff -w

--- a/gfx/angle/src/libEGL/moz.build
+++ b/gfx/angle/src/libEGL/moz.build

DEFINES['ANGLE_ENABLE_D3D9'] = True
+if CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
 DEFINES['ANGLE_ENABLE_D3D11'] = True
+

--- a/gfx/angle/src/libGLESv2/moz.build
+++ b/gfx/angle/src/libGLESv2/moz.build

 DEFINES['ANGLE_ENABLE_D3D9'] = True
+if CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
 DEFINES['ANGLE_ENABLE_D3D11'] = True
+

indents missing in both code.

(should build now)
Comment on attachment 8460298 [details] [diff] [review]
fixed patch, with diff -w

--- a/gfx/angle/src/libGLESv2/moz.build
+++ b/gfx/angle/src/libGLESv2/moz.build
+
+
+if CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
 EXTRA_DSO_LDOPTS += [ 'd3d9.lib', 'dxguid.lib' ]
+else:

(guess I was wrong.  Missing an indent here as well)
(In reply to Edmund Wong (:ewong) from comment #28)
> Comment on attachment 8460298 [details] [diff] [review]
> fixed patch, with diff -w
> 
> --- a/gfx/angle/src/libGLESv2/moz.build
> +++ b/gfx/angle/src/libGLESv2/moz.build
> +
> +
> +if CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
>  EXTRA_DSO_LDOPTS += [ 'd3d9.lib', 'dxguid.lib' ]
> +else:
> 
> (guess I was wrong.  Missing an indent here as well)

That's what diff -w does.
(In reply to Mike Hommey [:glandium] from comment #25)
> ::: gfx/angle/moz.build
> @@ +106,5 @@
> >      if CONFIG['CLANG_CXX']:
> >          CXXFLAGS += ['-Wno-unused-private-field']
> >  
> > +if CONFIG['MOZ_DIRECTX_SDK_PATH'] and not CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
> > +    CXXFLAGS += ['-I\'%s/include/\'' % CONFIG['MOZ_DIRECTX_SDK_PATH']]
> 
> You'll probably find me picky, but really, those escaped quotes shouldn't be
> here. The right thing to do is to make the recursivemake backend do the
> quoting itself, not do it in moz.build. (.replace(' ', '\ ') when adding
> them to backend.mk should be enough)

It's a totally valid point -- but it's more than just spaces.  It can be parens "Program Files (x86)" and even forward slashes, as in "C:\Program Files (x86)\Microsoft DirectX SDK\blah blah blah".

I landed it as-is, but I agree with you that we should add full param escaping in the generator.  That's a bigger change though and I didn't want to risk breaking something else along the way to getting this fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/970f7c1069c2
(In reply to Mike Hommey from comment #29)
> (In reply to Edmund Wong from comment #28)
> > (From update of attachment 8460298 [details] [diff] [review])
> > > +if CONFIG['MOZ_HAS_WINSDK_WITH_D3D']:
> > >  EXTRA_DSO_LDOPTS += [ 'd3d9.lib', 'dxguid.lib' ]
> > > +else:
> > 
> > (guess I was wrong.  Missing an indent here as well)
> 
> That's what diff -w does.

Which is why I explicitly didn't use diff -w for attachment 8459278 [details] [diff] [review].
I took the raw patch from the m-i link above and applied it to my local m-c tree but I still get the d3dcompiler.h missing problem. MSVC 10 on Windows 7.

Comment 33

4 years ago
(In reply to Daniel Stenberg [:bagder] from comment #32)
> I took the raw patch from the m-i link above and applied it to my local m-c
> tree but I still get the d3dcompiler.h missing problem. MSVC 10 on Windows 7.

Do you still have the --disable-webgl option in .mozconfig? That breaks the build, but the patch that removes the option hasn't landed yet.
https://hg.mozilla.org/mozilla-central/rev/970f7c1069c2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(In reply to Carsten Book [:Tomcat] from comment #34)
> https://hg.mozilla.org/mozilla-central/rev/970f7c1069c2

I can confirm that with this patch, VS2010 on win7, m-c compiles cleanly with the following mozconfig build params:

> --disable-update-packaging
> --enable-jemalloc
> --enable-signmar
> --enable-profiling
> --enable-js-diagnostics
> --disable-crashreporter
> --disable-gamepad
> --disable-angle
> --disable-debug-symbols
> --enable-optimize
> --disable-tests

Thanks.
(In reply to :Paolo Amadini from comment #33)

> Do you still have the --disable-webgl option in .mozconfig? That breaks the
> build, but the patch that removes the option hasn't landed yet.

I don't know how I messed it up first, but now when I retried it works fine. Thanks!
Comment on attachment 8458852 [details] [diff] [review]
remove --disable-webgl

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

r++
Attachment #8458852 - Flags: review?(jgilbert)
Attachment #8458852 - Flags: review+
Comment on attachment 8458852 [details] [diff] [review]
remove --disable-webgl

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: SeaMonkey can't Build a release on windows
[Describe test coverage new/current, TBPL]: Been on trunk/aurora for a long time now
[Risks and why]: Low risk, has baked for quite a long time, and the files this touches are the same between aurora and beta (same angle version), just aurora has this patch and beta doesn't.
[String/UUID change made/needed]: None

Alternatively SeaMonkey can create a relbranch for each and every release it tries to make this cycle, and transplant this ontop of each one. This is added manual work, and error-prone. And adds hours to each release, so would MUCH rather get this in now, rather than need to do the relbranch thing.

--- Additional Note, I have not cross-checked this will fix our beta builds, but afaict it will indeed. I will not even land this even if it has approval without verifying that first.
Attachment #8458852 - Flags: approval-mozilla-beta?
Comment on attachment 8458852 [details] [diff] [review]
remove --disable-webgl

err wrong patch to request approval for
Attachment #8458852 - Flags: approval-mozilla-beta?
Comment on attachment 8460298 [details] [diff] [review]
fixed patch, with diff -w

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

see: https://bugzilla.mozilla.org/show_bug.cgi?id=1037667#c38 --> and the explicit cset I'd transplant is https://hg.mozilla.org/releases/mozilla-aurora/rev/970f7c1069c2 to be clear
Attachment #8460298 - Flags: approval-mozilla-beta?
> [Risks and why]: Low risk, has baked for quite a long time, and the files this touches are the same between aurora and beta (same angle version), just aurora has this patch and beta doesn't.

Note this is mostly NPOTB. The only thing it effectively changes for Firefox builds is the order in which angle files are unified, and, well, we don't do unified builds on aurora/beta/release. So, in practical terms, this is NPOTB.
Attachment #8460298 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/1930a7b3c014
status-firefox33: --- → fixed
status-firefox34: --- → fixed

Updated

3 years ago
Duplicate of this bug: 1069114
You need to log in before you can comment on or make changes to this bug.