Skip to content

ggml-backend : keep paths in native string type when possible #12144

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

Merged
merged 1 commit into from
Mar 2, 2025

Conversation

slaren
Copy link
Member

@slaren slaren commented Mar 2, 2025

Avoids conversions from std::filesystem::path to non-native std::string to prevent possible issues when converting between different encodings. When unavoidable (e.g. to print a path), checks for exceptions.

Fixes #11198

@github-actions github-actions bot added the ggml changes relating to the ggml tensor library for machine learning label Mar 2, 2025
@gnusupport
Copy link

Thanks for fixing this issue.

@slaren slaren merged commit cc473ca into master Mar 2, 2025
46 of 47 checks passed
@slaren slaren deleted the sl/fix-fs-path branch March 2, 2025 21:11
@MaggotHATE
Copy link
Contributor

MaggotHATE commented Mar 5, 2025

This change causes errors with w64devkit on Windows 10 (c++2a):

ggml-backend-reg.cpp:78:32: error: no match for 'operator=' (operand types are 'std::string' {aka 'std::__cxx11::basic_string<char>'} and 'std::u8string' {aka 'std::__cxx11::basic_string<char8_t>'})
   78 |         u8path = path.u8string();
      |                                ^

Reverting this change fixes the issue (even with latest commits).

UPD: I'm not sure how to fix this correctly since

        std::u8string u8path_u8 = path.u8string();
        u8path = std::string(u8path_u8.cbegin(), u8path_u8.cend());

looks rather ugly, even if it works.

@slaren
Copy link
Member Author

slaren commented Mar 5, 2025

path.u8string returns a std::string in C++17, but a std::u8string in C++20. If some parts of your application need C++20 or higher, you can try building only these with C++20 enabled and continue building ggml and llama.cpp with C++17.

@MaggotHATE
Copy link
Contributor

path.u8string returns a std::string in C++17, but a std::u8string in C++20. If some parts of your application need C++20 or higher, you can try building only these with C++20 enabled and continue building ggml and llama.cpp with C++17.

This PR is the only thing that makes llama.cpp incompatible with C++20, shouldn't it be fixed? It seems a bit weird to leave it this way.

@slaren
Copy link
Member Author

slaren commented Mar 5, 2025

I am not sure what's the point since you can just build ggml with the correct flag, and I am not interested in supporting C++20, but if that's something that you care about feel free to open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ggml changes relating to the ggml tensor library for machine learning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eval bug: Crash with filesystem error when run while in a directory containing files with certain names
4 participants