Skip to content

Update slang-rhi deps for gfx-unit-test#8237

Merged
gtong-nv merged 6 commits intomasterfrom
slang-rhi-for-gfx-tests
Aug 26, 2025
Merged

Update slang-rhi deps for gfx-unit-test#8237
gtong-nv merged 6 commits intomasterfrom
slang-rhi-for-gfx-tests

Conversation

@gtong-nv
Copy link
Copy Markdown
Contributor

@gtong-nv gtong-nv commented Aug 20, 2025

This PR marks the slang-rhi a required dependecy for platform and gfx-unit-test, and only build them when SLANG_ENABLE_SLANG_RHI=ON.
This should allow the slang still to be built without those tests components when SLANG_ENABLE_SLANG_RHI=OFF.

@gtong-nv gtong-nv requested a review from a team as a code owner August 20, 2025 20:36
@gtong-nv gtong-nv added the pr: non-breaking PRs without breaking changes label Aug 20, 2025
@samestep
Copy link
Copy Markdown
Contributor

/format

@slangbot
Copy link
Copy Markdown
Contributor

🌈 Formatted, please merge the changes from this PR

Automated code formatting for
#8237

Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com>
@samestep
Copy link
Copy Markdown
Contributor

samestep commented Aug 20, 2025

Thanks for addressing this! Unfortunately it doesn't seem to have fixed the build issue that @niklaskorz mentioned in #7577 (comment); I still get this error:

In file included from /build/source/tools/test-server/../render-test/slang-support.h:5,
                 from /build/source/tools/test-server/test-server-main.cpp:13:
/build/source/tools/test-server/../render-test/options.h:15:10: fatal error: slang-rhi.h: No such file or directory
   15 | #include <slang-rhi.h>
      |          ^~~~~~~~~~~~~

@gtong-nv
Copy link
Copy Markdown
Contributor Author

Thanks for addressing this! Unfortunately it doesn't seem to have fixed the build issue that @niklaskorz mentioned in #7577 (comment); I still get this error:

In file included from /build/source/tools/test-server/../render-test/slang-support.h:5,
                 from /build/source/tools/test-server/test-server-main.cpp:13:
/build/source/tools/test-server/../render-test/options.h:15:10: fatal error: slang-rhi.h: No such file or directory
   15 | #include <slang-rhi.h>
      |          ^~~~~~~~~~~~~

yea, it's trickier than I thought. test-server also depends on slang-rhi, and slang-test depends on test-server. I'm checking if we can exclude slang-test if slang-rhi is OFF

kaizhangNV
kaizhangNV previously approved these changes Aug 20, 2025
@samestep
Copy link
Copy Markdown
Contributor

@gtong-nv that makes sense. If it'd make it easier, I'd imagine we can also change the Nixpkgs build to use -DSLANG_ENABLE_TESTS=OFF (which I assume is how the Emscripten build in this repo gets away with using -DSLANG_ENABLE_SLANG_RHI=OFF currently); but when I tried that just now, I got a bunch of errors like this:

CMake Error at tools/CMakeLists.txt:224 (add_custom_target):
  Error evaluating generator expression:

    $<TARGET_FILE_DIR:slang-test>

  No target "slang-test"

@samestep
Copy link
Copy Markdown
Contributor

Alternatively: the whole reason the Nixpkgs build disables slang-rhi in the first place is because there doesn't seem to be a way to build it without network access; would it be possible to just provide a way to build slang-rhi with a provided version of Dawn instead? Then we wouldn't need to disable slang-rhi when building Slang.

@gtong-nv
Copy link
Copy Markdown
Contributor Author

I think we should just exclude all tests and examples from the build is SLANG-RHI is OFF, as from what I can tell, almost all of them has #include <slang-rhi.h> directly or indirectly in the source code.

@gtong-nv
Copy link
Copy Markdown
Contributor Author

/format

@slangbot
Copy link
Copy Markdown
Contributor

🌈 Formatted, please merge the changes from this PR

Automated code formatting for
#8237

Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com>
@samestep
Copy link
Copy Markdown
Contributor

@gtong-nv thanks for fixing those earlier errors! It looks like the build still doesn't quite work with -DSLANG_ENABLE_SLANG_RHI=OFF and -DSLANG_ENABLE_TESTS=OFF though:

CMake Error at tools/cmake_install.cmake:272 (file):
  file INSTALL cannot find
  "/build/source/build/RelWithDebInfo/bin/gfx.slang": No such file or
  directory.
Call Stack (most recent call first):
  cmake_install.cmake:77 (include)

@gtong-nv
Copy link
Copy Markdown
Contributor Author

gtong-nv commented Aug 22, 2025

Hi @samestep, I tried a clean build and it should work.
Tried a few presets and even just with.
mkdir build; cd build; cmake .. -DSLANG_ENABLE_SLANG_RHI=OFF -DSLANG_ENABLE_TESTS=OFF

Can you clear camke cache and try a clean build? gfx.slang should still be generated and copied to bin/ with the -DSLANG_ENABLE_GFX=ON, which should be ON by default

@samestep
Copy link
Copy Markdown
Contributor

Unfortunately I have already been using a clean build :( Specifically, I've been testing this with the build setup in Nixpkgs, since that was the original motivation for this change: https://github.com/NixOS/nixpkgs/blob/596312aae91421d6923f18cecce934a7d3bfd6b8/pkgs/by-name/sh/shader-slang/package.nix

That is, one can reproduce the exact same build error by cloning Nixpkgs at that same commit:

gh repo clone NixOS/nixpkgs
git checkout 596312aae91421d6923f18cecce934a7d3bfd6b8

Then applying this patch to build with the latest commit from this branch:

diff --git a/pkgs/by-name/sh/shader-slang/package.nix b/pkgs/by-name/sh/shader-slang/package.nix
index 78edfe7f43ac..3d40aee57050 100644
--- a/pkgs/by-name/sh/shader-slang/package.nix
+++ b/pkgs/by-name/sh/shader-slang/package.nix
@@ -27,13 +27,13 @@
 
 stdenv.mkDerivation (finalAttrs: {
   pname = "shader-slang";
-  version = "2025.12.1";
+  version = "2025.14.3";
 
   src = fetchFromGitHub {
     owner = "shader-slang";
     repo = "slang";
-    tag = "v${finalAttrs.version}";
-    hash = "sha256-5M/sKoCFVGW4VcOPzL8dVhTuo+esjINPXw76fnO7OEw=";
+    rev = "c46125d22e6ecf66a8bb9b0c4b5bcaed8ee6990a";
+    hash = "sha256-8hC39nO7HbVPzSOIqHFSaq22mmwMwvd8W+zJ/ZEAh9w=";
     fetchSubmodules = true;
   };
 
@@ -118,6 +118,7 @@ stdenv.mkDerivation (finalAttrs: {
     # https://github.com/shader-slang/slang-rhi is "under active refactoring
     # and development, and is not yet ready for general use."
     "-DSLANG_ENABLE_SLANG_RHI=OFF"
+    "-DSLANG_ENABLE_TESTS=OFF"
     "-DSLANG_USE_SYSTEM_MINIZ=ON"
     "-DSLANG_USE_SYSTEM_LZ4=ON"
     "-DSLANG_SLANG_LLVM_FLAVOR=${if withLLVM then "USE_SYSTEM_LLVM" else "DISABLE"}"

Then running this command:

nix-build -A shader-slang

I also tried explicitly adding -DSLANG_ENABLE_GFX=ON and still got the same error.

@gtong-nv looking at e.g. the cmakeFlags in that pkgs/by-name/sh/shader-slang/package.nix file I linked above, do you see anything wrong that I should be changing to make this build work?

Or @niklaskorz do you happen to know how/if this build error should be fixed on the Nixpkgs side?

@niklaskorz
Copy link
Copy Markdown
Contributor

Or @niklaskorz do you happen to know how/if this build error should be fixed on the Nixpkgs side?

As you already noted in #8102, this is caused by an unintended dependency of the gfx module on the tests. Adding "-DSLANG_ENABLE_GFX=OFF" to the cmakeFlags (together with "-DSLANG_ENABLE_TESTS=OFF") makes this PR pass in nix.

Copy link
Copy Markdown
Contributor

@samestep samestep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you already noted in #8102, this is caused by an unintended dependency of the gfx module on the tests. Adding "-DSLANG_ENABLE_GFX=OFF" to the cmakeFlags (together with "-DSLANG_ENABLE_TESTS=OFF") makes this PR pass in nix.

Ah you're so right, thanks; just tried that and it does indeed work.

@gtong-nv
Copy link
Copy Markdown
Contributor Author

Techincally, SLANG_ENABLE_GFX should be OK to turn on as nothing in the slang repo should be relying on it. The gfx.dll is still being built only because some customers are still using it.
I will investigate this and make a follow up change if needed. I think this PR can be merged first.

@gtong-nv gtong-nv added this pull request to the merge queue Aug 26, 2025
Merged via the queue into master with commit 0b87355 Aug 26, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: non-breaking PRs without breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants