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

cmake: fix paths for vulkan shaders compilation on Windows #8573

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

stduhpf
Copy link
Contributor

@stduhpf stduhpf commented Jul 18, 2024

Fixes #8562

I hope it's not breaking it for others.

@mofosyne mofosyne added the Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix label Jul 19, 2024
@mstephenson6
Copy link
Contributor

Just got successful Vulkan builds with MSBuild/cl and ninja/clang on Windows 11 x64 with this PR, thank you!

@0cc4m 0cc4m self-assigned this Aug 4, 2024
Copy link
Collaborator

@0cc4m 0cc4m left a comment

Choose a reason for hiding this comment

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

Looks good to me. No issues on Linux. I can't test Windows, but other commenters confirmed that it works there. Thank you.

@0cc4m 0cc4m merged commit e31a4f6 into ggerganov:master Aug 5, 2024
7 checks passed
@MaggotHATE
Copy link
Contributor

Compilation fails on w64Devkit with this update:

error: 'replace' is not a member of 'std'
  487 |             std::replace(path.begin(), path.end(), '/', '\\' );

@Donwulff
Copy link

Donwulff commented Aug 6, 2024

Seconding this, not fixed, from my limited perspective into the code it's bit perplexing because the build seems to be fixed at -std=c++11 while std::replace is c++20 onwards, how is this supposed to work anywhere?

@0cc4m
Copy link
Collaborator

0cc4m commented Aug 6, 2024

Seconding this, not fixed, from my limited perspective into the code it's bit perplexing because the build seems to be fixed at -std=c++11 while std::replace is c++20 onwards, how is this supposed to work anywhere?

I agree the way it is shown by default in the documentation makes it look like std::replace is available since C++17 or C++20, but if you set the C++ revision at the top of the page you can see that std::replace is available since C++98. That's not the issue.

Maybe the problem for w64Devkit is just that #include <algorithm> is missing in the #ifdef _WIN32 include part at the top? Can you try that?

@MaggotHATE
Copy link
Contributor

#include

Yes, it seems to be the case #8880
Although I've missed your comment and didn't put it under #ifdef _WIN32 - is it critical?

@0cc4m
Copy link
Collaborator

0cc4m commented Aug 6, 2024

#include

Yes, it seems to be the case #8880 Although I've missed your comment and didn't put it under #ifdef _WIN32 - is it critical?

Not critical, but the algorithm part is in a Windows-only section.

arthw pushed a commit to arthw/llama.cpp that referenced this pull request Aug 7, 2024
…#8573)

* Vulkan-shaders: attempt fix compilation on windows

* fix miss-matched parenthesis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Vulkan build no longer working with MSVC cmake on windows
8 participants