Fix for LLAMA_WIN_VER default value, fixes #5158

[crimsonmagick]

Fix for #5158

Updated CMakeLists.txt to set the default value for LLAMA_WIN_VER with set() instead of option() (which is specifically for booleans).

I encountered the issue when I updated my Java JNI project to tag b2050, and my MinGW build broke.

Confirmed the bug with this diagnostic in CMakeLists.txt:

if (WIN32)
    option(LLAMA_WIN_VER                     "llama: Windows Version"                           0x602)
    message(WARNING "LLAMA_WIN_VER set to: ${LLAMA_WIN_VER}")
endif()

Which resulted in:

So, unless LLAMA_WIN_VER is supplied by a user def, this value defaults to OFF instead of the intended 0x602. Since I wasn't specifying the def, the MinGW build failed.

The option() command is for boolean values, and it looks like cmake defaults to OFF if it can't parse the parameter.

Switching from option() to set() fixed the issue:

I also confirmed that specifying LLAMA_WIN_VER still works after my update by passing -DLLAMA_WIN_VER=0x400:

[slaren]

Looks correct, but do you know why it wasn't caught when LLAMA_WIN_VER was first introduced in #3419?

[crimsonmagick]

Looks correct, but do you know why it wasn't caught when LLAMA_WIN_VER was first introduced in #3419?

My guess is that it's because of a difference in how the MinGW and MSVC toolchains handle an unsupported _WIN32_WINNT value.

MinGW likely either defaults to an older set of headers, or excludes certain headers altogether, resulting in the error include/fileapi.h:168:65: error: unknown type name 'FINDEX_INFO_LEVELS'; did you mean 'GET_FILEEX_INFO_LEVELS'? Here's a relevant forum post from back in 1999:

 FindFirstFileEx() is only available on Windows NT 4.0+ (not on Win95/98). It's declared in winbase.h as 
 
 #if(_WIN32_WINNT >= 0x0400)
   typedef enum _FINDEX_INFO_LEVELS {
   ...

If MinGW defaults to a version before 0x0400, we'd expect to get the error in issue #5158. MSVC likely defaults to a newer version of _WIN32_WINNT, probably the the latest version of Windows that the SDK supports. Since the Windows builds in build.yml use MSVC, this issue would not be readily apparent.

Do you think it would be worthwhile to add a check for MinGW?

[slaren]

As far as I understand the CMakeLists.txt, LLAMA_WIN_VER is only used when building with MinGW:

llama.cpp/CMakeLists.txt

Lines 956 to 959 in ed6b91d

	if (MINGW)
	# Target Windows 8 for PrefetchVirtualMemory
	add_compile_definitions(_WIN32_WINNT=${LLAMA_WIN_VER})
	endif()

So I don't think this is related to MSVC, but maybe something changed in MinGW since that was merged.

[crimsonmagick]

Ah, you're right. So not a difference between toolchains, but the issue still wouldn't be caught by the pipeline, unless there's a MinGW action I've missed in build.yml

[guilt]

Looks correct, but do you know why it wasn't caught when LLAMA_WIN_VER was first introduced in #3419?

That would be me. My bad. I was explictly testing it with -DLLAMA_WIN_VER=0x501 and it was working fine. Apparently it's a Boolean option and yet CMake overrides it with a working int value? That is some unintuitive bug.

The idea was to ensure we explictly define the version for older OSes and catch unintended compile time and runtime issues through explicit compiler guards, as quoted above. Did not catch this one. Sorry, and thank you for this patch.

[crimsonmagick]

Looks correct, but do you know why it wasn't caught when LLAMA_WIN_VER was first introduced in #3419?

That would be me. My bad. I was explictly testing it with -DLLAMA_WIN_VER=0x501 and it was working fine. Apparently it's a Boolean option and yet CMake overrides it with a working int value? That is some unintuitive bug.

The idea was to ensure we explictly define the version for older OSes and catch unintended compile time and runtime issues through explicit compiler guards, as quoted above. Did not catch this one. Sorry, and thank you for this patch.

Yeah agreed, the CMake behavior is really unintuitive - I would hope that OPTION() would throw an error when set with a non boolean value. Moreover, when left at the default OFF, the MinGW toolchain didn't complain about an invalid _WIN32_WINNT value, at least not at default logging levels.