999260 - Ship both d3dcompiler _43 and _46+ for use by ANGLE

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Right now, only ship _43, though there's a build flag to try to pull in _46+. However, _46 and newer don't work on XP, so we can't strictly upgrade to a newer version.
We can, however, ship both _43 and _46+, and use the newest one the client supports.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: dual-d3dc - WIP

Decision time: Should we:
* Require both _43 and _46+ for builds, opt-out for disabling the _46 req.
OR:
* Only require _43, and just make sure that our build slaves use the magic switch to also build _46+.

Comment 2

[Masatoshi Kimura [:emk]]

Is _43 always required? I would not like to install DXSDK again...

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

How is this different from bug 989205?

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Created attachment 8409970 [details] [diff] [review]
> dual-d3dc - WIP
> 
> Decision time: Should we:
> * Require both _43 and _46+ for builds, opt-out for disabling the _46 req.
> OR:
> * Only require _43, and just make sure that our build slaves use the magic
> switch to also build _46+.

Who will care?

Comment 5

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

How about: require either for builds, make sure build slaves do both.

We want to make it easy for developers; this means really requiring only 46/47 from the standard Windows SDK.  If they want to use an old SDK, then they need to have 43 available from the old dx sdk.  It doesn't make sense to require 43, though.

For the build slaves, it should package both 43 and 46.  I would add a switch to require both to get packaged, and make the build slave scripts use that.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: dual-d3dc

Comment 7

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=00b68eb34bdc

Comment 8

[Masatoshi Kimura [:emk]]

Comment on attachment 8410582 [details] [diff] [review]
dual-d3dc

>+# Check for valid WinSDK
>+MOZ_HAS_WINSDK_WITH_D3D=
>+
>+# On mingw, check if headers are provided by toolchain.
>+if test "$OS_TARGET" = "WINNT" -a -n "$GNU_CC"; then
>+  MOZ_CHECK_HEADER(d3d10.h, MOZ_HAS_WINSDK_WITH_D3D=1)
>+fi
>+
>+if test -n "$WINDOWSSDKDIR"; then
>+  MOZ_WINSDK_TEST_PATH="$WINDOWSSDKDIR/Include/um/d3d10.h"
>+  if test -f "$MOZ_WINSDK_TEST_PATH"; then
>+    MOZ_HAS_WINSDK_WITH_D3D=1
>+  fi
>+else
>+  AC_MSG_RESULT([WINDOWSSDKDIR is missing.])
>+fi

Who is using MOZ_HAS_WINSDK_WITH_D3D after the patch?

Comment 10

[Kelsey Gilbert [:jgilbert]]

(In reply to Masatoshi Kimura [:emk] from comment #8)
> Comment on attachment 8410582 [details] [diff] [review]
> dual-d3dc
> 
> >+# Check for valid WinSDK
> >+MOZ_HAS_WINSDK_WITH_D3D=
> >+
> >+# On mingw, check if headers are provided by toolchain.
> >+if test "$OS_TARGET" = "WINNT" -a -n "$GNU_CC"; then
> >+  MOZ_CHECK_HEADER(d3d10.h, MOZ_HAS_WINSDK_WITH_D3D=1)
> >+fi
> >+
> >+if test -n "$WINDOWSSDKDIR"; then
> >+  MOZ_WINSDK_TEST_PATH="$WINDOWSSDKDIR/Include/um/d3d10.h"
> >+  if test -f "$MOZ_WINSDK_TEST_PATH"; then
> >+    MOZ_HAS_WINSDK_WITH_D3D=1
> >+  fi
> >+else
> >+  AC_MSG_RESULT([WINDOWSSDKDIR is missing.])
> >+fi
> 
> Who is using MOZ_HAS_WINSDK_WITH_D3D after the patch?

The Gamepad API section below this one.

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8410582 [details] [diff] [review]
dual-d3dc

>-MOZ_D3DCOMPILER_DLL=

This still has some references in the tree:

./browser/installer/Makefile.in:DEFINES += -DMOZ_D3DCOMPILER_DLL=$(MOZ_D3DCOMPILER_DLL)
./browser/installer/package-manifest.in:@BINPATH@/@MOZ_D3DCOMPILER_DLL@

>-MOZ_D3DCOMPILER_CAB=

This too:

./gfx/angle/angle-long-ident-hash.patch: ifdef MOZ_D3DCOMPILER_CAB

Any idea if the ValueError in the try log is related to one of these?

The rest looks good to me, but you'll need someone else to review the GLLibraryEGL.cpp changes.

Comment 12

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=dff07aba0bd7

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: dual-d3dc

Comment 14

[Kelsey Gilbert [:jgilbert]]

With this change, the test slaves should pick up both versions automatically, but we can enforce this with "ac_add_options --enable-require-all-d3dc-versions". I don't know where to put this, though. :mshal?

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8413952 [details] [diff] [review]
dual-d3dc

I don't know if Ted is the right person, but I touch some of the Gamepad API configure.in section here, so let's get sign-off on that change, too.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8413952 [details] [diff] [review]
dual-d3dc

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

Meh, I can't see a problem, so r+, but I also don't recognize this code: it seems to have received patches since I last looked at it, and I didn't review these patches, so now I don't feel 100% competent to do this review. Glad that Ted is also flagged.

Comment 17

[Michael Shal [:mshal]]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> With this change, the test slaves should pick up both versions
> automatically, but we can enforce this with "ac_add_options
> --enable-require-all-d3dc-versions". I don't know where to put this, though.
> :mshal?

I'm not sure exactly what you're asking for here. Why would a test slave care about a configure flag?

Comment 18

[Kelsey Gilbert [:jgilbert]]

(In reply to Michael Shal [:mshal] from comment #17)
> (In reply to Jeff Gilbert [:jgilbert] from comment #14)
> > With this change, the test slaves should pick up both versions
> > automatically, but we can enforce this with "ac_add_options
> > --enable-require-all-d3dc-versions". I don't know where to put this, though.
> > :mshal?
> 
> I'm not sure exactly what you're asking for here. Why would a test slave
> care about a configure flag?
Sorry, I meant build slaves. We want to have our build slaves build with this flag, so they fail to build if they don't have both DLLs we want them to have.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 8413952 [details] [diff] [review]
dual-d3dc

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

::: build/mozconfig.common
@@ +15,5 @@
>  ac_add_options --enable-crashreporter
>  
>  ac_add_options --enable-release
> +
> +ac_add_options --enable-require-all-d3dc-versions

I don't think you want this here, this mozconfig is for all platforms. You probably want:
http://mxr.mozilla.org/mozilla-central/source/browser/config/mozconfigs/win32/common-opt
(and if you want it in debug:
http://mxr.mozilla.org/mozilla-central/source/browser/config/mozconfigs/win32/debug )

Comment 20

[Michael Shal [:mshal]]

I think #c19 addresses the question - if not let me know!

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> The Gamepad API section below this one.

Not needed anymore due to bug 996078. (Also, it should have checked dinput.h rather than d3d10.h.)

Comment 22

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

Comment on attachment 8413952 [details] [diff] [review]
dual-d3dc

MOZ_HAS_WINSDK_WITH_D3D is never AC_SUBST'd with this patch, even though it's referenced in gfx/angle/src/libEGL/Makefile.in and libGLESv2.

We actually *want* to take that path as much as possible, because including the DirectX SDK's include files causes conflicts and a bunch of warnings due to duplicate definitions in the Windows SDK.  Basically, the only time we want to use the DirectX SDK's includes is if you're building with VS2010 and the Windows 7 SDK.  If someone is using the Windows 8/8.1 SDKs, we shouldn't be touching the DX SDK at all, except perhaps to pull out d3dcompiler_43.dll.

This can be fixed in a followup patch though, but it should be done soon.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

As a side question: remind me why we don't just put the DXSDK DLLs that we need in MozillaBuild?

Comment 24

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

Putting them in MozillaBuild is not a bad idea. I don't think we went down that route.

However, after this patch, developers should never need the DXSDK any more, unless they're using VS2010/Win7 SDK and want ANGLE.  Only the build machines will need to have somewhere to get d3dcompiler_43.dll from.

Comment 25

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=617eb022f3c2

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: dual-d3dc

Alright, one more time. Green on try, and build slaves are pulling both DLLs. (verified both by checking the build log, where we claim to find both, and the built ZIP, which contains both _43 and _46.

At some point, we should try to move this to _47.

Comment 27

[Michael Shal [:mshal]]

Comment on attachment 8416701 [details] [diff] [review]
dual-d3dc

>-MOZ_DIRECTX_SDK_CPU_SUFFIX=

1) How come this doesn't break this flag in libxul.mk?

EXTRA_DSO_LDOPTS += \
  -LIBPATH:'$(MOZ_DIRECTX_SDK_PATH)/lib/$(MOZ_DIRECTX_SDK_CPU_SUFFIX)' \
  $(NULL)

2) And I'm not sure if you saw this from my earlier comment, or if we just don't care to update the patch:

./gfx/angle/angle-long-ident-hash.patch: ifdef MOZ_D3DCOMPILER_CAB

Comment 28

[Kelsey Gilbert [:jgilbert]]

(In reply to Michael Shal [:mshal] from comment #27)
> Comment on attachment 8416701 [details] [diff] [review]
> dual-d3dc
> 
> >-MOZ_DIRECTX_SDK_CPU_SUFFIX=
> 
> 1) How come this doesn't break this flag in libxul.mk?
> 
> EXTRA_DSO_LDOPTS += \
>   -LIBPATH:'$(MOZ_DIRECTX_SDK_PATH)/lib/$(MOZ_DIRECTX_SDK_CPU_SUFFIX)' \
>   $(NULL)
It probably does break that, but I think this line is redundant with at least what we do in gfx/angle.
> 
> 2) And I'm not sure if you saw this from my earlier comment, or if we just
> don't care to update the patch:
> 
> ./gfx/angle/angle-long-ident-hash.patch: ifdef MOZ_D3DCOMPILER_CAB
I'll try fixing this, but I'm not going to guarantee the patch survives this migration without scratches.

Comment 29

[Kelsey Gilbert [:jgilbert]]

Attachment: dual-d3dc

Fixed the libxul.mk instance, and decided to ignore the angle patch. It'll just mean it doesn't apply cleanly, but that's fine. (It probably doesn't already. We don't keep them up-to-date)

Comment 30

[Michael Shal [:mshal]]

Comment on attachment 8416726 [details] [diff] [review]
dual-d3dc

Sounds fine - thanks!

Comment 31

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b48386fc226

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8b48386fc226