[Bug] Build failure on Fedora 44 (GCC 16) #1475

Activity

1. cphlipot

   added

   bugSomething isn't workingSomething isn't working

   on May 6, 2026
2. wbruna commented on May 6, 2026

   

   wbruna

   on May 6, 2026

   Contributor

   More actions

   unless the project overides the behavior which stable-diffusion.cpp does not.

   Actually, we do:

   https://github.com/leejet/stable-diffusion.cpp/blob/master-593-3d6064b/CMakeLists.txt#L256
   https://github.com/leejet/stable-diffusion.cpp/blob/master-593-3d6064b/examples/cli/CMakeLists.txt#L20
   https://github.com/leejet/stable-diffusion.cpp/blob/master-593-3d6064b/examples/server/CMakeLists.txt#L104

   target_compile_features(${TARGET} PUBLIC c_std_11 cxx_std_17)

   And if I change that to C++20, I can reproduce that exact failure:

   ./examples/server/runtime.cpp: In function ‘void refresh_lora_cache(ServerRuntime&)’:
   ./examples/server/runtime.cpp:267:53: 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>’})
     267 |             lora_entry.name     = p.stem().u8string();
         |    
   

   so it's definitely active for our build targets.
3. silvertuanzi commented on May 6, 2026

   

   silvertuanzi

   on May 6, 2026

   Last edited by silvertuanzi

   More actions

   Only set target_compile_features(${TARGET} PUBLIC c_std_11 cxx_std_17) isn't enough.

   In my archlinux I use a mix compiler:
   CC: gcc-16
   CXX: g++-16
   CUDA: nvcc13.2 with host g++-15

   Build will failed with default cmake .. -G Ninja -DSD_CUDA=ON
   But cmake .. -G Ninja -DSD_CUDA=ON -DCMAKE_CXX_STANDARD=17 build successfully.

   I use the same commit 3d6064b
4. cphlipot commented on May 6, 2026

   

   cphlipot

   on May 6, 2026

   Last edited by cphlipot

   ContributorAuthor

   More actions

   target_compile_features(${TARGET} PUBLIC c_std_11 cxx_std_17)

   @wbruna Thanks for pointing to that. Based on my research and testing it looks like requesting a C++ feature set via target_compile_features() only guarantees a MINIMUM C++ version, not an EXACT C++ version. Since CMake assumes the GCC16 default C++20 already includes the features from C++17, it does not actually set -std=C++17 explicitly and just uses the default C++20 which causes issues with this project.

   there is a bit more to read on that topic here: https://discourse.cmake.org/t/determining-the-compiler-default-c-standard/10389/7

   The cxx_std_## features are for minimum standard levels. CXX_STANDARD will be honored even if it is lower than the compiler’s default, and an explicit -std= flag will be used to achieve it if necessary.

   through the use of CMAKE_EXPORT_COMPILE_COMMANDS i can see the exact compiler options used to build runtime.cpp (the one with the errors)

   mkdir build && cd build
   cmake .. -DSD_VULKAN=ON
   CMAKE_EXPORT_COMPILE_COMMANDS=1  cmake --build . --config Release
   grep runtime.cpp compile_commands.json  | grep command
   

   When i do this, i do not see -std=c++17 (or any other C++ version) being specified for this file. which is consistent with the hypothesis it is not being set because default is already c++20

   /usr/lib64/ccache/c++ -DGGML_MAX_NAME=128 -DGGML_USE_CPU -DGGML_USE_VULKAN -DHAVE_INDEX_HTML -DSD_USE_WEBM -DSD_USE_WEBP -I/home/user/git/stable-diffusion.cpp/examples -I/home/user/git/stable-diffusion.cpp/examples/server/frontend/dist -I/home/user/git/stable-diffusion.cpp/. -I/home/user/git/stable-diffusion.cpp/src -I/home/user/git/stable-diffusion.cpp/include -I/home/user/git/stable-diffusion.cpp/thirdparty -I/home/user/git/stable-diffusion.cpp/ggml/src/../include -I/home/user/git/stable-diffusion.cpp/thirdparty/. -I/home/user/git/stable-diffusion.cpp/thirdparty/libwebp/src -I/home/user/git/stable-diffusion.cpp/thirdparty/libwebp -I/home/user/git/stable-diffusion.cpp/thirdparty/libwebm -O3 -DNDEBUG -o CMakeFiles/sd-server.dir/runtime.cpp.o -c /home/user/git/stable-diffusion.cpp/examples/server/runtime.cpp
   

   My recommendation is that we ensure -std=c++17 to be set explicitly both to resolve this compiler issue, and also to ensure that people with newer compilers don't accidentally introduce new C++ features to the codebase that would break those with older toolchains.

   i think we can accomplish that by doing something like:

   set_target_properties(${TARGET} PROPERTIES C_STANDARD 11 C_STANDARD_REQUIRED ON CXX_STANDARD 17 CXX_STANDARD_REQUIRED ON)
   

   👍React with 👍2Reacted by silvertuanzi and Nindaleth
5. jeeb commented on May 8, 2026

   

   jeeb

   on May 8, 2026

   Last edited by jeeb

   Contributor

   More actions

   Thanks for properly reporting this, I found this out a while ago since I have been running Fedora 44 since it was in beta, but as I did not see a clear way forward and was busy, I did not report it.

   When looking into this, the problem for me was that:

   • std::string and std::u8string were to my understanding always separate types
   • f0f641a introduced the mis-match from the get-go in the LoraEntry structure, without explaining why this mis-match was needed. LoraEntry always had std::string there, but the code setting values to it was:

   LoraEntry e;
   e.name          = p.stem().u8string();
   std::string rel = fs::relative(p, lora_dir).u8string();
   std::replace(rel.begin(), rel.end(), '\\', '/');
   e.path = rel;

   So in other words, it was not clear what the intent was. Neither the commit message nor the PR say anything regarding it ( #1256 ) . If the intent was to have u8string s, then why was the structure defined with strings ? If there was a reason for the conversion by means of implicit casting (which C++ in general dislikes), then why was there no cast or mention as to why the types were mismatching between the structure and the input?

   Of course, it doesn't help that GCC only started blocking this sort of code by default with version 16 (not sure if it even warned before about it), and clang 22 is still silent looking at a clang build I did. So catching this after writing it would have not been simple.

   In other words, while forcing C++17 specifically would let it build, I think looking at what the intent of this code is would probably be a good idea as well?
6. wbruna commented on May 8, 2026

   

   wbruna

   on May 8, 2026

   Contributor

   More actions

   LoraEntry e;
   e.name = p.stem().u8string();
   std::string rel = fs::relative(p, lora_dir).u8string();
   std::replace(rel.begin(), rel.end(), '\', '/');
   e.path = rel;

   So in other words, it was not clear what the intent was. Neither the commit message nor the PR say anything regarding it ( #1256 ) . If the intent was to have u8string s, then why was the structure defined with strings ? If there was a reason for the conversion by means of implicit casting (which C++ in general dislikes), then why was there no cast or mention as to why the types were mismatching between the structure and the input?

   Since those strings come from filesystem names, and given the \ replacement, I believe the intent is avoiding encoding mismatches on Windows (Koboldcpp does something similar when loading models). If that's really the case, explicit conversions at this point should be fine. But we ideally need a way to at least get a warning on C++17, to avoid regressions.
7. jeeb commented on May 8, 2026

   

   jeeb

   on May 8, 2026

   Contributor

   More actions

   I do agree that compilers should warn about this stuff.

   Also looking at previous IRC logs where I tried to wrap my head around whether the compilation error was valid or not, the following spec proposals were linked regarding char8_t and its integration into u8string usage:

   • https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0482r6.html (OG proposal for char8_t, adopted for C++20)
   • https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1423r3.html (char8_t backward compatibility remediation)
8. cphlipot commented on May 8, 2026

   

   cphlipot

   on May 8, 2026

   ContributorAuthor

   More actions

   I think we should still explictly set the C++ version which not only unblocks the build but also ensures going forward all devs are testing the build locally using the same C++17 version instead of whatever their local compiler defaults to. So even if we resolve the u8string issue, i think this would be good to keep.

   I'm a a bit indifferent on the idea of warnings. personally i've found on other projects that warnings are often ignored unless also coupled with -werror to also turn them into compile errors, although I can't say for certain if that would apply to this project's devs too or not.

   if you wanted to explore enabling warnings for this, -Wc++20-compat exists as a potential option for that, although i will admit i have not tested it.
9. cphlipot commented on May 8, 2026

   

   cphlipot

   on May 8, 2026

   ContributorAuthor

   More actions

   https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1423r3.html (char8_t backward compatibility remediation)
   

   I found and skimmed this doc when doing original troubleshooting. I'm pretty sure the compiler errors we're seeing on Fedora44 fall under the "ill-formed" classification even with that remediation in place based on the Examples table there.
10. jeeb commented on May 8, 2026

    

    jeeb

    on May 8, 2026

    Contributor

    More actions

    Since those strings come from filesystem names, and given the \ replacement, I believe the intent is avoiding encoding mismatches on Windows (Koboldcpp does something similar when loading models). If that's really the case, explicit conversions at this point should be fine. But we ideally need a way to at least get a warning on C++17, to avoid regressions.

    Alright, so specifically UTF-8 containing strings are wanted (I presume that is the whole wstring/string difference between Windows and non-Windows), then if the APIs these strings get pushed to only take in std::string the struct should contain that and std::string(s.begin(), s.end()) utilized when #if defined(__cpp_lib_char8_t) as per P1423R3 - or just always if the compiler is OK with it in C++17 mode. This will incur a cost due to the copying. Although I'm definitely not sure about where the C++ stdlib does character set conversions, if at all.

    I think we should still explictly set the C++ version which not only unblocks the build but also ensures going forward all devs are testing the build locally using the same C++17 version instead of whatever their local compiler defaults to. So even if we resolve the u8string issue, i think this would be good to keep.

    I'm a a bit indifferent on the idea of warnings. personally i've found on other projects that warnings are often ignored unless also coupled with -werror to also turn them into compile errors, although I can't say for certain if that would apply to this project's devs too or not.

    if you wanted to explore enabling warnings for this, -Wc++20-compat exists as a potential option for that, although i will admit i have not tested it.

    -Wc++20-compat sounds like a good catch. Thanks, will test.

    And yes, specific warnings should be made errors in the build system - many projects do that (when you limit to specific ones you are much less likely to get hit by a random change causing build failure with new toolchains).
11. jeeb commented on May 8, 2026

    

    jeeb

    on May 8, 2026

    Last edited by jeeb

    Contributor

    More actions

    if you wanted to explore enabling warnings for this, -Wc++20-compat exists as a potential option for that, although i will admit i have not tested it.

    -Wc++20-compat sounds like a good catch. Thanks, will test.

    Unfortunately at least clang 22.1.4 gives no additional warnings with this for examples/server/runtime.cpp (and looking at the specific warnings it adds the char8_t related stuff seems to be mostly about string literals as opposed to "live" strings: warning: type of UTF-8 string literal will change from array of const char to array of const char8_t in C++20 )
12. wbruna commented on May 8, 2026

    

    wbruna

    on May 8, 2026

    Contributor

    More actions

    I do agree that compilers should warn about this stuff.

    Heh, they can't: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1423r3.html#introduction mentions:

    2. Change of return type for std::filesystem::path member functions u8string and generic_u8string.

    So there's nothing to warn about on C++17. It was designed to be an annoying transition 🤦 Guess they got envious of Python 3 encoding mess...
13. cphlipot commented on May 11, 2026

    

    cphlipot

    on May 11, 2026

    ContributorAuthor

    More actions

    @jeeb If the goal behind enabling a warning for this is to simply turn around and then expose it as an error. You may want to consider skipping the warning, and just enable the the compile feature for this directly so it will always error. This may also be the only option, as it sounds like there is no warning for this.

    For example if we wanted to keep the c++ 17 base feature set and enable only this feature so u8 strings get checked at compile time without giving devs full access to C++20 features, we could do something like this:

    if(MSVC)
        target_compile_options(${TARGET} PRIVATE /Zc:char8_t)
    else()
        target_compile_options(${TARGET} PRIVATE -fchar8_t)
    endif()