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

Compile shaders at runtime #423

Closed

Conversation

Hentropy
Copy link
Contributor

@Hentropy Hentropy commented Feb 13, 2021

This PR closes #231 though it does things a little differently.

Currently, this won't work for Android or wasm targets. The internal logic is all in place, but it won't build.

The issue is entirely in the cargo.toml, the relevant parts:

[features]
use-installed-tools = ["spirv-builder/use-installed-tools"]
use-compiled-tools = ["spirv-builder/use-compiled-tools"]

[target.'cfg(not(any(target_os = "android", target_arch = "wasm32")))'.dependencies]
spirv-builder = { path = "../../../crates/spirv-builder", default-features = false }

[build-dependencies]
spirv-builder = { path = "../../../crates/spirv-builder", default-features = false }

This should work but fails for the aarch64-linux-android target:

error[E0463]: can't find crate for `rustc_ast`
  --> crates\rustc_codegen_spirv\src\lib.rs:57:1
   |
57 | extern crate rustc_ast;
   | ^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

Commenting out the normal spirv-builder dependency - the one that should be ignored for Android - fixes the issue, but then PC targets don't work.

This works for both PC and Android:

[features]
default = ["spirv-builder"]
runtime-spirv-builder = ["spirv-builder"]
#use-installed-tools = ["spirv-builder/use-installed-tools"]
#use-compiled-tools = ["spirv-builder/use-compiled-tools"]

[target.'cfg(not(any(target_os = "android", target_arch = "wasm32")))'.dependencies]
spirv-builder = { path = "../../../crates/spirv-builder", default-features = false, features = ["use-installed-tools"], optional = true }

[build-dependencies]
spirv-builder = { path = "../../../crates/spirv-builder", default-features = false, features = ["use-installed-tools"] }

However, this isn't an acceptable solution because it forces everyone to have spirv-tools to run the examples (or devs/CI to compile them every time, which takes a long time). Uncommenting the features causes the same issue as before, even though the optional feature isn't enabled.

Renaming one of them also doesn't work, cargo does not allow the same dependency to have different names, even if one is a build dependency and one isn't. I don't know why, but c'est la vie.

The only workable solution I see here is reimplementing the SpirvBuilder logic in the compile_shader function, however this was flagged as an issue when I did that in the ash runner.

Moves shader building from a build.rs to runtime. Recompile shaders with F5 while the app is running.  Fixes validation errors on minimize.
Android and wasm can't run rustc so they work the same as before.
@Jasper-Bekkers
Copy link
Contributor

I think this is a good direction to head in, but we should discuss before accepting this change if removing a significant test for our build.rs path is acceptable for the project. Would be good to discuss in the next team meeting perhaps?

@khyperia khyperia mentioned this pull request Feb 18, 2021
Duplicate the compilation logic from spirv-builder to circumvent cargo issues.
@khyperia
Copy link
Contributor

With rust-lang/cargo#9216 fixed, the cargo issues you're hitting may have been resolved! (still need to wait for a nightly with the fix though)

@Hentropy
Copy link
Contributor Author

Thanks for following up on that 😃 looks like it should be on nightly soon

@VZout VZout removed their request for review March 13, 2021 17:10
@Hentropy
Copy link
Contributor Author

The issue has been resolved.

@khyperia
Copy link
Contributor

Hmm, having spirv-builder be referenced twice on non-android/wasm32 targets (once as normal dep, once as build dep) causes a warning when using resolver = "2"

warning: output filename collision.
The lib target `rustc_codegen_spirv` in package `rustc_codegen_spirv v0.4.0-alpha.0 (/home/khyperia/me/rust-gpu/crates/rustc_codegen_spirv)` has the same output filename as the lib target `rustc_codegen_spirv` in package `rustc_codegen_spirv v0.4.0-alpha.0 (/home/khyperia/me/rust-gpu/crates/rustc_codegen_spirv)`.
Colliding filename is: /home/khyperia/me/rust-gpu/target/debug/deps/librustc_codegen_spirv.so
The targets should have unique names.
Consider changing their names to be unique or compiling them separately.
This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>.

It'd be really nice to not have to build spirv-builder as a build-dep on runtime-compiled platforms, if that's possible

@Hentropy
Copy link
Contributor Author

Setting resolver = 2 on Windows breaks completely, causing two link.exe errors.
First, an error with exit code 1104 which shows it attempting to link a couple hundred rustc_codegen_spirv codegen units:

/rust-gpu/target/debug/deps/rustc_codegen_spirv.<some hash>.rcgu.o

LNK1104 is most likely being caused by multiple compilation instances trying to access the same file.
Second, an exit code 1120 with thousands of unresolved external symbols for core traits and other deps.

There's a related cargo issue about this issue already.

The same warning also occurs without the resolver flag, but for example-runner-wgpu, without causing any further issues (that's been a thing the whole time I've been working on rust-gpu).

Regarding changing it to not import it twice, I don't think it's possible in the current form of SpirvBuilder. build.rs is built for the host, so the usual #[cfg(target_os ="..", target_arch = "..")] macros pick up the host OS/arch, which means that build dependencies effectively can't be optional based off the target OS/arch. Overriding the build.rs itself using the [package] build = ".." option doesn't work because [target.'..'.package] isn't a thing.
One possible solution is adding a spirv_builder_macros crate with a build_spirv proc macro, removing the build.rs entirely, and invoking the macro in the already conditionally compiled shader_module function.
Another is adding features for dynamically compiled vs statically compiled shaders and updating CI to use the appropriate flags. This is simpler, but the macro is more generally useful since it removes the need for a build.rs for all users.

@khyperia
Copy link
Contributor

Looks like a way around being unable to cfg dependencies in build.rs is the approach this PR used - platform-specific specifying of a feature in a helper crate's build.rs cart/glsl-to-spirv#7

@Hentropy Hentropy closed this Jun 3, 2021
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.

3 participants