Closed Bug 896762 Opened 7 years ago Closed 1 year ago

Use a fixed version of d3dcompiler dll in the tree for builds

Categories

(Core :: Canvas: WebGL, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: emk, Assigned: jgilbert)

References

Details

(Whiteboard: webgl-internal)

Attachments

(3 files, 7 obsolete files)

Windows 8 Kit has d3dcompiler_46.dll in the Redist folder, and Windows 8.1 Kit has d3dcompiler_47.dll. We could use them so that we don't have to require installing DirectX SDK anymore.
I don't think we require the Win 8 kit; or are you suggesting that we look for them if the Win 8 SDK is installed?
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #1)
> or are you suggesting that we look
> for them if the Win 8 SDK is installed?

Yes. But it looks like DXSDK was required anyway because Win 8 Kit didn't contain d3d9.h.

That being said, newer version would have fewer bugs.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #1)
> I don't think we require the Win 8 kit; or are you suggesting that we look
> for them if the Win 8 SDK is installed?

It's pretty easy to add. There's a list of compilers that ANGLE looks for, so we should just expand that list.
I'm not sure what to say here? Being up to date is good! :) If we have it we should use it!
Flags: needinfo?(bas)
I didn't know if there are any build related issues with adding this.
(In reply to Masatoshi Kimura [:emk] from comment #2)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #1)
> > or are you suggesting that we look
> > for them if the Win 8 SDK is installed?
> 
> Yes. But it looks like DXSDK was required anyway because Win 8 Kit didn't
> contain d3d9.h.
> 
> That being said, newer version would have fewer bugs.

according to https://code.google.com/p/angleproject/wiki/DevSetup  DirectX SDK are not required when using win8 sdk
Attached patch Bug896762.patch (obsolete) — Splinter Review
I added --enable-winsdk-directx switch.
if you enable this switch on and use winsdk 8 or 8.1, its built with only winsdk.
else its built with winsdk and directx sdk.
(In reply to Masatoshi Kimura [:emk] from comment #2)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #1)
> > or are you suggesting that we look
> > for them if the Win 8 SDK is installed?
> 
> Yes. But it looks like DXSDK was required anyway because Win 8 Kit didn't
> contain d3d9.h.
> 
> That being said, newer version would have fewer bugs.

winsdk 8 contain d3d9.h in C:\Program Files (x86)\Windows Kits\8.0\Include\shared .
8.1 in C:\Program Files (x86)\Windows Kits\8.1\Include\shared .
and this include path set by vcvars32.bat and the other.
We should switch to using d3dcompiler_46.dll and distribute it with our builds. Investigating FF hangs with http://shadertoy.com website, replacing d3dcompiler_43.dll with d3dcompiler_46.dll was the difference between the main page taking seconds to load versus hanging FF.
Is there anything gating us from using higher-versioned compilers than _43? Can XP still use _46 and _47?
not sure if _46 is supported on XP.  Also, I think _43 is the last one where we could extract directly aybe... We can also still *ship* _43, but dynamically load higher-versioned ones (but vetted, so not arbitrary) in case the user already has them installed.  Most will. (Though note that > 43 gets installed to WinSxS not just system32.)
I don't see why we can't ship _46 on XP. It's just a HLSL -> bytecode compiler. (I had a quick look at the DLL dependencies with OllyDbg and nothing looks untoward)
If the best thing to do is to get rid of the DirectX SDK dependency, Bas, do we have a bug for that?  We could then just make this one depend on it, if that simplifies things.
Flags: needinfo?(bas)
(In reply to Milan Sreckovic [:milan] from comment #13)
> If the best thing to do is to get rid of the DirectX SDK dependency, Bas, do
> we have a bug for that?  We could then just make this one depend on it, if
> that simplifies things.

Well, we depend on it for the HLSL->bytecode conversion. It's not an easy dependency to drop.
Flags: needinfo?(bas)
Bas, I think Milan was referring to the dependency upon the June 2010 DirectX SDK.

At the moment we pull d3dcompiler_43.dll out of the June 2010 SDK (from a redist cab). Is it not possible for us to upgrade our requirement, to something like the Windows SDK for Windows 8, that has d3dcompiler_46.dll, or somehow package the dll ourselves?

Looking at http://blogs.msdn.com/b/chuckw/archive/2013/10/03/a-brief-history-of-windows-sdks.aspx, We'd need to require Windows SDK 7.1 because that is the last Windows SDK to target Windows XP.

On further reading (http://blogs.msdn.com/b/chuckw/archive/2012/05/07/hlsl-fxc-and-d3dcompile.aspx), d3dcompiler from version 44 onwards, is now to be redistributed in the same directory as your application.
(In reply to Dan Glastonbury :djg :kamidphish from comment #15)
> Looking at
> http://blogs.msdn.com/b/chuckw/archive/2013/10/03/a-brief-history-of-windows-sdks.aspx,
> We'd need to require Windows SDK 7.1 because that is the last
> Windows SDK to target Windows XP.

We are using Windows 8 SDK to build Metro Firefox which shares the same binary with desktop Firefox. So binaries built with Windows 8 SDK should be compatible with Windows XP.
(In reply to Dan Glastonbury :djg :kamidphish from comment #15)
> On further reading
> (http://blogs.msdn.com/b/chuckw/archive/2012/05/07/hlsl-fxc-and-d3dcompile.
> aspx), d3dcompiler from version 44 onwards, is now to be redistributed in
> the same directory as your application.

That's fantastic.  In this case, we can just check it in and ship it.
I'll take a shot at this, then?
Assignee: nobody → jgilbert
Attached patch patch 1: Add d3dcompiler_47.dll (obsolete) — Splinter Review
Attachment #813838 - Attachment is obsolete: true
Attachment #8379903 - Flags: review?(vladimir)
Attachment #8379911 - Flags: review?(vladimir)
With these three, it built and ran for me. I haven't verified with a debugger that it's running with version 17, but without patch 2, ANGLE fails to initialize. With it, everything's working again. \o/
Attachment #8379914 - Flags: review?(vladimir)
(In reply to Jeff Gilbert [:jgilbert] from comment #19)
> Created attachment 8379903 [details] [diff] [review]
> patch 1: Add d3dcompiler_47.dll

This patch removes the detection for older versions of d3dcompiler DLLs. Are we going to drop support for Windows SDK versions lesser than 8.1? Only SDK 8.1 will have d3dcompiler_47. SDK 8.0 has d3dcompiler_46. Prior to 8.0 SDK, d3dcompiler DLLs were not in the redistributable.

(In reply to Jeff Gilbert [:jgilbert] from comment #20)
> Created attachment 8379911 [details] [diff] [review]
> patch 2: Tell ANGLE about version 17 of the DLL

version *47*?
(In reply to Masatoshi Kimura [:emk] from comment #22)
> (In reply to Jeff Gilbert [:jgilbert] from comment #19)
> > Created attachment 8379903 [details] [diff] [review]
> > patch 1: Add d3dcompiler_47.dll
> 
> This patch removes the detection for older versions of d3dcompiler DLLs. Are
> we going to drop support for Windows SDK versions lesser than 8.1? Only SDK
> 8.1 will have d3dcompiler_47. SDK 8.0 has d3dcompiler_46. Prior to 8.0 SDK,
> d3dcompiler DLLs were not in the redistributable.
> 
Me too. This patch binds sdk version to 8.1 .
now, windows sdk 8.0 or later is optional.
https://developer.mozilla.org/en-US/docs/Windows_8
(In reply to Masatoshi Kimura [:emk] from comment #22)
> (In reply to Jeff Gilbert [:jgilbert] from comment #19)
> > Created attachment 8379903 [details] [diff] [review]
> > patch 1: Add d3dcompiler_47.dll
> 
> This patch removes the detection for older versions of d3dcompiler DLLs. Are
> we going to drop support for Windows SDK versions lesser than 8.1? Only SDK
> 8.1 will have d3dcompiler_47. SDK 8.0 has d3dcompiler_46. Prior to 8.0 SDK,
> d3dcompiler DLLs were not in the redistributable.
We remove detection because with this patch, it's not needed. This patch pulls {x86,x64}/d3dcompiler_47.dll into the tree. I grepped for other uses of these constants, but didn't find anything, and so removed them.

> 
> (In reply to Jeff Gilbert [:jgilbert] from comment #20)
> > Created attachment 8379911 [details] [diff] [review]
> > patch 2: Tell ANGLE about version 17 of the DLL
> 
> version *47*?

Ok, for some reason 47 became 17 in my mind. Sorry about that!
Attachment #8379911 - Attachment description: patch 2: Tell ANGLE about version 17 of the DLL → patch 2: Tell ANGLE about version 47 of the DLL
Comment on attachment 8379914 [details] [diff] [review]
patch 3: Add the ANGLE patch to gfx/angle

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

I'll s/17/47/ here after review from Vlad.

::: gfx/angle/README.mozilla
@@ +57,5 @@
>    angle-fix-vc12.patch:
>      Fixes angle to build on Visual Studio 2013
>  
> +  angle-add-d3dcompiler17.patch:
> +    Add version 17 of the compiler DLL to the auto-load list.

47!
Attachment #8379914 - Flags: review-
Comment on attachment 8379903 [details] [diff] [review]
patch 1: Add d3dcompiler_47.dll

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

Include some info in these directories saying where the DLL came from, and some info on updating them.
Attachment #8379903 - Flags: review?(vladimir) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #24)
> We remove detection because with this patch, it's not needed. This patch
> pulls {x86,x64}/d3dcompiler_47.dll into the tree. I grepped for other uses
> of these constants, but didn't find anything, and so removed them.

Sorry, I don't understand. If 8.1 SDK is not installed, where d3dcompiler_47.dll is pulled from?
Ah, the first patch containied d3dcompiler_47.dll itself!
Is it legally allowed? That is, is d3dcompiler_47.dll licensed with an OSS license? It's very questionable given that we don't include mscvr*.dll itself in our tree.
Asking Gerv about the d3dcompiler license.
Flags: needinfo?(gerv)
(In reply to Masatoshi Kimura [:emk] from comment #28)
> Ah, the first patch containied d3dcompiler_47.dll itself!
> Is it legally allowed? That is, is d3dcompiler_47.dll licensed with an OSS
> license? It's very questionable given that we don't include mscvr*.dll
> itself in our tree.

That's a good question. From blogs.msdn.com/b/chuckw/archive/2012/05/07/hlsl-fxc-and-d3dcompile.aspx:

   "[...] Instead, you should copy it into your applications folder and deploy it 'application local' with your Win32 desktop application."

But that doesn't say what if you can redist with source code.
We shouldn't be pulling MS proprietary code into our tree. If it's needed during the build, or needed to go into the install package, then it should be pulled from the appropriate SDK or other Microsoft-installed location at the time it's needed. 

And we should check that we have the rights to distribute any bits we are distributing. At the moment, at least according to the Binary Components Policy <http://www.mozilla.org/en-US/foundation/licensing/binary-components/> and <http://www.mozilla.org/en-US/foundation/licensing/binary-components/rationale/> we are only shipping the D3D9X DLL. If we are now shipping more stuff, did it go through legal review?

Gerv
We need to redist this, and we need to do it in a way that doesn't introduce any pointless heavyweight dependencies that serve no purpose other than add complexity.  Please follow up with me over email if there are any questions.
vlad: was that in response to me? 

I have no plans to stop you redistributing this as part of Firefox, unless it's illegal :-) I don't think we have the right to check it into our tree and let all-comers download it from Hg, though. Do you think I'm wrong?

Not sure what you are referring to re: heavyweight dependencies.

Gerv
Flags: needinfo?(gerv)
As an aside, we don't redistribute the D3D9X DLL anymore, but redistribute the D3DCompiler DLL. The example rationale is a bit out of date.
1. If the 8.1 SDK is installed, pull d3dcompiler_47.dll from the redist directory of the SDK, just like msvcr*.dll.
2. Otherwise, if the 8.0 SDK is installed, pull d3dcompiler_46.dll from the redist directory of the SDK.
3. Otherwise, extract d3dcompiler_43.dll from the redisributable cab of the DirectX SDK (same as the current code).

Developers will have to install either SDKs anyway, so I don't think this is "heavyweight dependencies". I admit this will add complexity, but it is unavoidable cost to solve legal implications.
Comment on attachment 813838 [details] [diff] [review]
Bug896762.patch

The obsoleted patch is already doing the most job. We should unbitrot and take this.
Attachment #813838 - Attachment is obsolete: false
(In reply to Masatoshi Kimura [:emk] from comment #36)
> Comment on attachment 813838 [details] [diff] [review]
> Bug896762.patch
> 
> The obsoleted patch is already doing the most job. We should unbitrot and
> take this.

my patch is buggy when x86_amd64 and amd64_x86.
I'll fix and rebase. please wait.
No, we don't want to depend on particular versions of the SDK installed; there's really no point, especially no point in getting people's builds with different versions of this dll.  We'll put it in other-licenses; it's redistributable.

:emk, gerv, Hiroki, we can discuss this over email if you'd like, but let's leave the discussion out of the bug.  I don't want to derail this.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #38)
> No, we don't want to depend on particular versions of the SDK installed;
> there's really no point, especially no point in getting people's builds with
> different versions of this dll.  We'll put it in other-licenses; it's
> redistributable.

Why don't we add redistributable msvcr*.dll into our tree, then? If it's impossible, please prove d3dcompiler is redistributable with our source tree unlike msvcr*.dll. I'm fine as long as we are consistent.

> :emk, gerv, Hiroki, we can discuss this over email if you'd like, but let's
> leave the discussion out of the bug.  I don't want to derail this.

I would like to make the discussion public as much as possible.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #38)
> No, we don't want to depend on particular versions of the SDK installed;
> there's really no point, especially no point in getting people's builds with
> different versions of this dll.

There's a point in preventing us from being sued by Microsoft.

> I don't want to derail this.

This is not derailment. We can't ignore the legal issue to include any copyrighted bits.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #38)
> No, we don't want to depend on particular versions of the SDK installed;
> there's really no point, especially no point in getting people's builds with
> different versions of this dll.  We'll put it in other-licenses; it's
> redistributable.
> 
I'm afraid of dll version differs from header version.

(In reply to Masatoshi Kimura [:emk] from comment #39)
> > :emk, gerv, Hiroki, we can discuss this over email if you'd like, but let's
> > leave the discussion out of the bug.  I don't want to derail this.
> 
> I would like to make the discussion public as much as possible.
:-)
Attachment #8379903 - Attachment is obsolete: true
Attachment #8381889 - Flags: review?(vladimir)
r=vlad
Attachment #8379911 - Attachment is obsolete: true
Attachment #8381891 - Flags: review+
Attachment #8381891 - Attachment description: angle-compiler-nows → patch 2: Tell ANGLE about d3dcompiler_47.dll.
r=vlad
Attachment #8379914 - Attachment is obsolete: true
Attachment #8381892 - Flags: review+
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #38)
> :emk, gerv, Hiroki, we can discuss this over email if you'd like, but let's
> leave the discussion out of the bug.  I don't want to derail this.

Making sure we are legally allowed to do something before doing it is not derailing.

Microsoft has a habit of writing a new variant of REDIST.TXT file every time they ship some sort of SDK, so we'd need to check specifically for this one, but IIRC it's a common restriction that their redistributables can only be redistributed in the context of shipping a binary release of your software to your customers, and with appropriate EULA-like terms attached for those redistributables.

See https://mxr.mozilla.org/mozilla-central/source/toolkit/content/license.html?force=1#4353 for the current language we use in about:license for Windows builds only. Note:

4383 (i)   You may use, copy, and distribute the Distributable Code only as part of
4384       this product;

This is a restriction on us that MS require us to pass on.

So unless we can get our legal team to consent (because their legal opinion, as actual lawyers, overrides mine) then we should not do this.

Gerv
I opened bug 976976 to cover the issue that our current Binary Components Policy rationale does not reflect what we are currently doing.

Gerv
Attachment #813838 - Attachment is obsolete: true
Attachment #8382010 - Flags: review?(gerv)
(In reply to Gervase Markham [:gerv] from comment #45)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #38)
> > :emk, gerv, Hiroki, we can discuss this over email if you'd like, but let's
> > leave the discussion out of the bug.  I don't want to derail this.
> 
> Making sure we are legally allowed to do something before doing it is not
> derailing.
> 
> Microsoft has a habit of writing a new variant of REDIST.TXT file every time
> they ship some sort of SDK, so we'd need to check specifically for this one,
> but IIRC it's a common restriction that their redistributables can only be
> redistributed in the context of shipping a binary release of your software
> to your customers, and with appropriate EULA-like terms attached for those
> redistributables.
Yep, the current redist list is linked to in the new README in patch 1.
The license for this on my system is at:
Program Files (x86)\Microsoft Visual Studio 12.0\Licenses\Windows 8.1 SDK License.rtf
Comment on attachment 8382010 [details] [diff] [review]
patch 1: use windows sdk for directx headers and libraries

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

::: configure.in
@@ +5694,5 @@
>  dnl = * enabled by default (shipping build); requires explicit --disable to disable
>  dnl ========================================================
>  MOZ_ANGLE_RENDERER=
>  MOZ_DIRECTX_SDK_PATH=
> +MOZ_WINSDK_DIRECTX=

Is this variable needed? Can you pick the latest D3DCompiler DLL automatically?

@@ +5790,5 @@
> +    MOZ_D3DX9_VERSION=46
> +    AC_MSG_RESULT([Found DirectX SDK in Windows SDK 8.0 .])
> +  ;;
> +  *)
> +    AC_MSG_ERROR([DirectX SDK does not included Older Windows SDK. Either install DirectX SDK (June 2010 or Windows SDK 8.0 or newer), or reconfigure with --disable-webgl.])

"DirectX SDK is not included in older Windows SDKs."
Attachment #8382010 - Attachment is obsolete: true
Attachment #8382010 - Flags: review?(gerv)
Attachment #8382254 - Flags: review?(gerv)
I'm not the right guy to r+ these patches from a technical POV. If vlad accepts my argument, then r+ from a licensing viewpoint. If not, then we'll need to open a legal bug to seek further advice.

Gerv
Comment on attachment 8382254 [details] [diff] [review]
patch 1: use windows sdk for directx headers and libraries

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

::: gfx/angle/Makefile.in
@@ +9,5 @@
>  ifdef MOZ_D3DCOMPILER_CAB
>  	expand '$(MOZ_D3DCOMPILER_CAB)' -F:$(MOZ_D3DCOMPILER_DLL) '$(DIST)/bin'
>  endif
> +else
> +	cp -p "$${WINDOWSSDKDIR}/Redist/D3D/$(MOZ_DIRECTX_SDK_CPU_SUFFIX)/$(MOZ_D3DCOMPILER_DLL)" "$(DIST)/bin"

Is $${SDKDIR} unusable? MozillaBuild patch will not be needed with that.
I would love to include all the relevant MS DLLs in our tree (basically, ms*crt and d3dcompiler, I think).  I'm clarifying stuff with legal, will post an update when I have one, though people are at MWC so it's going to take a few more days.
(In reply to Masatoshi Kimura [:emk] from comment #53)
> Comment on attachment 8382254 [details] [diff] [review]
> patch 1: use windows sdk for directx headers and libraries
> 
> Review of attachment 8382254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/angle/Makefile.in
> @@ +9,5 @@
> >  ifdef MOZ_D3DCOMPILER_CAB
> >  	expand '$(MOZ_D3DCOMPILER_CAB)' -F:$(MOZ_D3DCOMPILER_DLL) '$(DIST)/bin'
> >  endif
> > +else
> > +	cp -p "$${WINDOWSSDKDIR}/Redist/D3D/$(MOZ_DIRECTX_SDK_CPU_SUFFIX)/$(MOZ_D3DCOMPILER_DLL)" "$(DIST)/bin"
> 
> Is $${SDKDIR} unusable? MozillaBuild patch will not be needed with that.

Can we use $${SDKDIR} in source tree?
I think this value is just a transient in MozillaBuild startup batch.
$${WINDOWSSDKDIR} comes from vcvars32.bat and other. we should use $${WINDOWSSDKDIR}.
and when msvc2010, we switch windows sdk to 8.0 or 8.1. $${WINDOWSSDKDIR} should be set 8.0 or 8.1 too.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #54)
> I would love to include all the relevant MS DLLs in our tree (basically,
> ms*crt and d3dcompiler, I think).  I'm clarifying stuff with legal, will
> post an update when I have one, though people are at MWC so it's going to
> take a few more days.

As the person who ends up dealing with this stuff day-to-day, I'd like to be CCed on that bug, please. (Such a decision should definitely be recorded in a bug for future reference, not just contained in a private email trail.)

Gerv
(In reply to ABE Hiroki (hATrayflood) from comment #55)
> I think this value is just a transient in MozillaBuild startup batch.

Oops. You're right.
Let's split this in two:
This bug is now about importing the DLL directly into the tree.

Bug 980697 is about allowing for more recent versions of the DLL to be pulled from installed Windows SDKs.
Summary: Consider using the latest d3dcompiler dll → Use a fixed version of d3dcompiler dll in the tree for builds
Comment on attachment 8382255 [details] [diff] [review]
patch 4: mozilla-build: overwrite WINDOWSSDKDIR to windows sdk 8.0 or 8.1 on msvc2010

Is this patch designed to apply on top of my three patches? I'm not entirely clear on its purpose.
Attachment #8382255 - Flags: review?(gerv) → review?(jgilbert)
Comment on attachment 8382254 [details] [diff] [review]
patch 1: use windows sdk for directx headers and libraries

I moved this to bug 980697.
Attachment #8382254 - Attachment is obsolete: true
Attachment #8382254 - Flags: review?(gerv)
See Also: → 980697
Whiteboard: webgl-internal
(In reply to Jeff Gilbert [:jgilbert] from comment #59)
> Comment on attachment 8382255 [details] [diff] [review]
> patch 4: mozilla-build: overwrite WINDOWSSDKDIR to windows sdk 8.0 or 8.1 on
> msvc2010
> 
> Is this patch designed to apply on top of my three patches? I'm not entirely
> clear on its purpose.

No. bug 980697 needs this patch when msvc2010.
Comment on attachment 8382255 [details] [diff] [review]
patch 4: mozilla-build: overwrite WINDOWSSDKDIR to windows sdk 8.0 or 8.1 on msvc2010

I moved this to bug 980697.
Attachment #8382255 - Attachment is obsolete: true
Attachment #8382255 - Flags: review?(jgilbert)

We don't really need this.

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.