1037667 - ANGLE update broke DX SDK + Win 7 SDK builds

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

[[e-mail]]

Attachment: 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.)

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

[:Paolo Amadini]

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

[Kestrel]

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

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 8

[Mike Hommey [:glandium]]

(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.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Hmm, the bug I duped also happened with --disable-angle, so you might want to check if that switch actually works.

Comment 10

[:Paolo Amadini]

(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.

Comment 11

[Avi Halachmi (:avih)]

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).

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

--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?

Comment 13

[Avi Halachmi (:avih)]

(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.

Comment 14

[Steve Fink [:sfink] [:s:]]

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.

Comment 15

[Avi Halachmi (:avih)]

It compiles for me. I had --disable-angle though I don't know if it took effect or not.

Thanks.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 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.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 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.

Comment 18

[Mike Hommey [:glandium]]

(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.

Comment 19

[[e-mail]]

Attachment: 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.

Comment 20

[[e-mail]]

(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?

Comment 21

[Robert Longson [:longsonr]]

Attachment: 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']),

Comment 22

[[e-mail]]

(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.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 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.

Comment 25

[Mike Hommey [:glandium]]

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)

Comment 26

[Edmund Wong (:ewong)]

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 27

[Edmund Wong (:ewong)]

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 28

[Edmund Wong (:ewong)]

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)

Comment 29

[Mike Hommey [:glandium]]

(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.

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(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

Comment 31

[[e-mail]]

(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].

Comment 32

[Daniel Stenberg [:bagder]]

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

[:Paolo Amadini]

(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.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/970f7c1069c2

Comment 35

[Avi Halachmi (:avih)]

(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.

Comment 36

[Daniel Stenberg [:bagder]]

(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 37

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8458852 [details] [diff] [review]
remove --disable-webgl

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

r++

Comment 38

[Justin Wood (:Callek)]

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.

Comment 39

[Justin Wood (:Callek)]

Comment on attachment 8458852 [details] [diff] [review]
remove --disable-webgl

err wrong patch to request approval for

Comment 40

[Justin Wood (:Callek)]

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

Comment 41

[Mike Hommey [:glandium]]

> [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.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1930a7b3c014