Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace glsl-to-spirv with shaderc #2731

Closed
wants to merge 3 commits into from
Closed

Conversation

djcsdy
Copy link

@djcsdy djcsdy commented Apr 6, 2019

Replaces glsl-to-spirv with shaderc.

glsl-to-spirv is deprecated and the author states: "use shaderc-rs instead".

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds
  • tested examples with the following backends: dx12, vulkan
  • rustfmt run on changed code

Note: make reftests is failing for dx12 on master, so it’s failing on this branch too. It passes for vulkan.

glsl_to_spirv is deprecated and the author states: "Use shaderc-rs
instead".
glsl_to_spirv is deprecated and the author states: "Use shaderc-rs
instead".
@djcsdy djcsdy changed the title Shaderc Replace glsl-to-spirv with shaderc Apr 6, 2019
@djcsdy
Copy link
Author

djcsdy commented Apr 6, 2019

Urgh. The CI build is failing on Windows due to the 260 character path limit 🤢.

ninja is an optional build dependency of shaderc. Without it, shaderc
fails to build on Travis Windows due to the Visual Studio project
generator creating files with pathnames that exceed the notorious
260-character limit on Windows.
@djcsdy
Copy link
Author

djcsdy commented Apr 7, 2019

This PR now also installs ninja on Travis Windows before the build.

ninja is an optional build-time dependency of shaderc. It is recommended on Windows because without it the shaderc build process generates files with very long pathnames, which can cause build failures.

@msiglreith
Copy link
Contributor

Hi and thanks for the PR!
How is the experience for a user who wants to start using gfx-rs by trying out an example or building code on top of existing ones? Are there further build systems required to get started, which might especially a blocker on windows systems?

@djcsdy
Copy link
Author

djcsdy commented Apr 8, 2019

This adds a dependency on Python on all platforms, and on Windows sometimes adds a dependency on ninja.

I am using Windows myself and I didn’t encounter significant problems. The standard Windows distribution of Python 3 from https://www.python.org/downloads/windows/ works fine. Of course Python has to be on the PATH. The Python installer can do this for you – it is optional but IIRC turned on by default.

Ninja is required on Windows if the path to the gfx source tree is longer than about 30 characters. This is because, without Ninja, the shaderc build process uses a different project generator that generates excessively long path names, which can cause a build failure on Windows. I didn’t experience this problem on my machine because I had the project checked out at a shorter path, but as you can see it blew up on Travis and it was not immediately obvious what the solution was. I think it would be a good idea for the shaderc project to just add a hard dependency on ninja at build time on Windows, since at least then it could produce a helpful error message.

I’m not aware of any other additional dependencies compared to what this project already depended on – shaderc depends on cmake and a C compiler, but those were already required.

@djcsdy
Copy link
Author

djcsdy commented Apr 8, 2019

I proposed a PR to shaderc which would make Ninja mandatory when building for Windows with MSVC: google/shaderc-rs#47

@fu5ha
Copy link
Contributor

fu5ha commented Apr 9, 2019

IMO we should not adopt shaderc for the examples but we should in warden. We can make a note that shaderc is a better choice for a real project, but since glsl-to-spirv works for the quite simple use case we need it for, and honestly doesn't really break until you get a fair bit deeper, it "just works", and it doesn't double or more the build times.... I think it is worth it to leave in.

@ghost
Copy link

ghost commented Apr 9, 2019

There's some earlier discussion about this subject in #2546.

I agree that shaderc build times are not great. I'm also not a fan of the additional dependencies to Python and ninja. Was CMake already a dependency for gfx-hal? Where is this dependency?

We might be able to avoid these dependencies by wrapping glslang ourselves and invoking it directly instead of going through shaderc.

@djcsdy
Copy link
Author

djcsdy commented Apr 10, 2019

I thought CMake was a dependency of gfx already, but I guess I was mistaken. I installed CMake for some Rust project, but I guess it wasn’t this one.

@ghost
Copy link

ghost commented Apr 11, 2019

Looking into it, glsl-to-spirv does have CMake as a dependency, but since it includes pre-built binaries on Windows, it's only used on the other platforms.

@knappador
Copy link

Static lib options and crate behavior have been made more normal and default behaviors are expanded in google/shaderc-rs#46

The expanded static options just make it easier to pick a path that builds really fast with few dependencies. The old --no-defaults option was funky (but still supported, just deprecated).

I don't think this makes life easier on gfx since it's pretty heavily focused on expanding options for Linux CI. @djcsdy if you want support on getting the build script working on Windows or have anything you want but didn't get, just say so.

Appveyor is currently advertising a build failure, but I don't have access to the failure info to go fix it. Time for other-platform people to ✨ ✨

@djcsdy
Copy link
Author

djcsdy commented Apr 16, 2019

@knappador The build does work on Windows. Check my last commit.

@djcsdy
Copy link
Author

djcsdy commented Apr 16, 2019

@knappador Scratch that sorry, I misunderstood. I see you are referring to google/shaderc-rs#46, which is currently failing on Windows.

@knappador
Copy link

@djcsdy I'm fixing a line that got regressed as soon as I can shove it into history realtime github chat

@knappador
Copy link

@djcsdy okay tests passed on the CI servers. No idea what platforms or coverage they have. I don't think you need ninja to do the static lib build paths. I pretty much just cargo-culted forward what was there for the --no-defaults legacy path in build.rs for non-Linux builds

@knappador
Copy link

Shaderc's crate split has landed with some additional options to get pre-built binaries linked. I'm available to guide integration attempts and land additional PR's against shaderc-rs if more build variations are desired.

@imkow
Copy link

imkow commented Jun 19, 2019

functionality side, shaderc looks fine, but I think it's not ready for cross platform support. And it depends on too many 3rd party stuff.

@djcsdy
Copy link
Author

djcsdy commented Jun 20, 2019

I don’t see any path forward for me here.

If someone else wants to pick this up, please do. But consider me thoroughly discouraged.

@kvark
Copy link
Member

kvark commented Jun 27, 2019

@djcsdy sorry about the way this PR progressed (and essentially ended up unmerged)!
Please don't hesitate to visit our chat to talk or just provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants