Autogenerating shaders using fxc does not work on Linux (MinGW Build)

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: tjr, Assigned: tjr)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [tor])

Attachments

(1 attachment)

(Assignee)

Description

a year ago
We don't have the Windows SDK, or are able to run Windows Binaries on Linux as part of the build chain we're working on in Bug 1330608. We've been able to work around it so far without much problem, but I'm not very familiar with Graphics to know what our options are for fxc and Bug 1365859.

Is there an open source alternative to fxc? Is it something that can be created by someone who doesn't know anything about Graphics?  

Failing those, how do you feel about a partial revert where the generated file is still present in the source tree? I'm not sure if the patch was done solely for the purpose of avoiding an ugly diff in hg, or if it was also to help an edit-run-checkin workflow. If it's the latter, we can detect if fxc is present, and if so, regenerate the file for easy checkin - that would solve the workflow problem while making the program optional...
(Assignee)

Updated

a year ago
Flags: needinfo?(dvander)
There's no open source alternative that I know of that can generate Shader Model 4 instructions. The patch was done for both reasons you mentioned.
Flags: needinfo?(dvander)
(Assignee)

Comment 2

a year ago
Can I write a patch that behaves the way I described? (Have the generated file in-tree, detect if fxc is present, and if so, on modification regenerate the file.) Would you accept it?

I'm open to other options, but I'm not sure if there are others.
(Assignee)

Comment 3

a year ago
After talking with David, there is a strong desire to not do a partial revert and put the file back in tree (even with autogenerating), as it slows down the developer workflow.

Shaders can be compiled at run time, so the tentative plan (and potentially only option?) is to add and conditionally compile in the shader compiling code if fxc cannot be found. An alternative would involve storing the latest version of the generated file somewhere and downloading it out of band or something very ugly.
(Assignee)

Comment 4

a year ago
Updating this with latest approach: I'm writing a 'fxc2' program that will call D3DCompileFromFile that exists in d3dcompiler_47.dll. This isn't ideal as it's not open source but it's the only option. https://github.com/wine-mirror/wine/tree/master/dlls/d3dcompiler_43 is not feature-complete enough to use.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8880565 - Flags: review?(jmuizelaar)
(Assignee)

Comment 7

a year ago
I put in a try job to make sure this still runs on Windows, I should submit another to confirm artifact job still works after the first completes successfully. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3268cdfeb9da
Comment on attachment 8880565 [details]
Bug 1370007 Generate Shaders on a MinGW Cross Compile on Linux

dvander's probably more appropriate to review and maybe a build peer...
Attachment #8880565 - Flags: review?(jmuizelaar) → review?(dvander)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8880565 - Flags: review?(jmuizelaar) → review?(dvander)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8880565 - Flags: review?(ted)
(Assignee)

Comment 14

a year ago
Ted, could you review this from a build perspective?

Comment 15

a year ago
mozreview-review
Comment on attachment 8880565 [details]
Bug 1370007 Generate Shaders on a MinGW Cross Compile on Linux

https://reviewboard.mozilla.org/r/151898/#review160034

Kind of a bummer that these fxc tools rely on a non-open-source DLL provided by Microsoft. :-(

::: gfx/layers/d3d11/genshaders.py:85
(Diff revision 7)
>      shader_file,
>      '-E{0}'.format(shader_name),
>      '-Vn{0}'.format(shader_name),
>      '-Vi',
>    ]
> +  if 'fxc2' in buildconfig.substs['FXC']:

I think this would probably be better if you just explicitly checked if you were building on Linux here.

::: moz.configure:450
(Diff revision 7)
>      return ''
>  
>  set_config('MAKENSISU_FLAGS', nsis_flags)
>  
> +fxc = check_prog('FXC', ('fxc.exe', 'fxc2.exe'), when=depends(target)(lambda t: t.kernel == 'WINNT'))
> +wine = check_prog('WINE', ['wine'], when=depends(fxc)(lambda f: f and 'fxc2' in f))

Similarly here I would make this only run when `target.kernel == 'WINNT' and host.kernel == 'Linux'`.
Attachment #8880565 - Flags: review?(ted) → review+
Comment on attachment 8880565 [details]
Bug 1370007 Generate Shaders on a MinGW Cross Compile on Linux

https://reviewboard.mozilla.org/r/151898/#review160048

The whole operating system is closed source. It doesn't bother me that the build tools are too.

This seems fine as long as it doesn't affect normal Windows builds.
Attachment #8880565 - Flags: review?(dvander) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a year ago
nits fixed, thanks all!
Keywords: checkin-needed

Comment 19

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/018e683b25cd
Generate Shaders on a MinGW Cross Compile on Linux r=dvander,ted
Keywords: checkin-needed

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/018e683b25cd
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
have to backout this change for issues like https://treeherder.mozilla.org/logviewer.html#?job_id=112552463&repo=mozilla-central
Status: RESOLVED → REOPENED
status-firefox56: fixed → ---
Flags: needinfo?(tom)
Resolution: FIXED → ---

Comment 22

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/173533a31006
Backed out changeset 018e683b25cd for breaking tier 2 nightly builds on windows
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

a year ago
We'd like to have these checks in in toolchain.config to avoid an extra depends(), but that is broken by Bug 1374727. If that is not resolved when I'm near completing Bug 1330608, we'll land these checks in the top level moz.config.
Depends on: 1374727
Flags: needinfo?(tom)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

a year ago
Okay, Bug 1374727 landed and the updated version gives me green try runs (except the L10N repack, but that error occurs elsewhere and not on the fxc detection like it did the first time.)  I'm going to mark this checkin-needed, and if there's any errors, feel free to back out without acknowledgement.
Keywords: checkin-needed

Comment 29

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aeb9b61cd780
Generate Shaders on a MinGW Cross Compile on Linux r=dvander,ted
Keywords: checkin-needed

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aeb9b61cd780
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.