-
Notifications
You must be signed in to change notification settings - Fork 406
nix: dev-shell fixes #9437
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
nix: dev-shell fixes #9437
Conversation
0229435 to
7f915a2
Compare
samestep
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these fixes! I haven't tested them yet, but I did have one request in the meantime.
Add the packages required for prebuilt LLVM to LD_LIBRARY_PATH. Also, switch from `stdenvNoCC` to a proper clang/llvm stdenv.
7f915a2 to
035a709
Compare
Also rewrites the comments around our slicing of llvmPackages to explain exactly what each slice is for.
|
The clang/clangd interaction is upsetting and subtle. I broke it before, but it should be fixed with the latest commit. |
samestep
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this does indeed look subtle... thank you for figuring this all out!
|
@rkoser-nv since you mentioned that this PR is meant to add support for prebuilt LLVM, do you have a particular (set of) build command(s) I can run in the Nix dev shell before and after this PR to see what specifically didn't work before but does now? For instance, just running this command doesn't seem to work: cmake --preset default -DSLANG_SLANG_LLVM_FLAVOR=USE_SYSTEM_LLVMThat is, on both commit 671b391 and commit 5674b1b, I get the same error: |
|
This fixes the downloaded LLVM binaries used by slang/tools/slang-test/slang-test-main.cpp Lines 798 to 805 in 671b391
|
|
@rkoser-nv right yeah, I understand that: that was why I opened #8028 and #8031 in August, at which point in time I was having to follow the Building with a Local Build of slang-llvm instructions from external/build-llvm.sh --source-dir build/slang-llvm_src --install-prefix build/slang-llvm_install
cmake --preset default --fresh -DSLANG_SLANG_LLVM_FLAVOR=USE_SYSTEM_LLVM -DLLVM_DIR=build/slang-llvm_install/lib/cmake/llvm -DClang_DIR=build/slang-llvm_install/lib/cmake/clang
cmake --build --preset releaseWhat I'm asking is: does this PR now enable me to successfully run the FileCheck tests with a different set of commands? If so, which ones? (Since the one I tried in my message above didn't work.) |
|
Yes, the following should work: the resulting |
samestep
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks! In the future it would be cool if one could just use -DSLANG_SLANG_LLVM_FLAVOR=USE_SYSTEM_LLVM with LLVM from Nix instead of having to download a prebuilt slang-llvm binary, but obviously that's beyond the scope of this PR.
Just now I ran these commands to see the difference produced by this PR:
cmake --preset default
cmake --build --preset release
build/Release/bin/slang-testOn commit dd4ba6e (the base of this PR branch), I got this output at the end:
===
99% of tests passed (1828/1829), 4396 tests ignored
===
1 failing tests:
---
tests/hlsl-intrinsic/scalar-int64.slang (cpu)
---
And on commit 5674b1b (the tip of this PR branch), I got this output at the end instead:
===
100% of tests passed (4264/4264), 2641 tests ignored
===
I'm guessing those 2436 extra passing tests / 1755 fewer ignored tests mean that slang-llvm is working properly using this PR.
this is what I've been doing for a while now! I should really integrate my version... |
|
@expipiplus1 that would be amazing! It would definitely have saved me and @rkoser-nv from working on these inferior solutions 😅 |
Add the packages required for prebuilt LLVM to `LD_LIBRARY_PATH`. Also, switch from `stdenvNoCC` to a proper clang/llvm stdenv. --------- Co-authored-by: Ellie Hermaszewska <[email protected]>
186351f
Add the packages required for prebuilt LLVM to
LD_LIBRARY_PATH.Also, switch from
stdenvNoCCto a proper clang/llvm stdenv.