Add -municode for MINGW

[vrabaud]

Fixes #1755

[wantehchang]

LGTM. I suggest a change.

[wantehchang]

We should test this. I remember MINGW is GCC, so I am worried that it will match the GCC check at line 299 instead.

[vrabaud]

Definitely, let me add a MINGW workflow from https://github.com/msys2/setup-msys2

[wantehchang]

If MINGW is identified as GCC, we will need to handle it as follows:

elseif(CMAKE_C_COMPILER_ID MATCHES "GNU")
    message(STATUS "libavif: Enabling warnings for GCC")
    add_compile_options(-Wall -Wextra)
    if(MINGW)
       add_compile_options(-municode)
    endif()
elseif(MSVC)

[wantehchang]

On the Web I found at least two pages that say -municode can be used as a linker option. Here is a patch file to do that:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 12e0a10..6b05fb7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -802,11 +802,17 @@ if(AVIF_BUILD_APPS)
     if(AVIF_USE_CXX)
         set_target_properties(avifenc PROPERTIES LINKER_LANGUAGE "CXX")
     endif()
+    if(MINGW)
+        target_link_options(avifenc PRIVATE -municode)
+    endif()
     target_link_libraries(avifenc avif_apps)
     add_executable(avifdec apps/avifdec.c)
     if(AVIF_USE_CXX)
         set_target_properties(avifdec PROPERTIES LINKER_LANGUAGE "CXX")
     endif()
+    if(MINGW)
+        target_link_options(avifdec PRIVATE -municode)
+    endif()
     target_link_libraries(avifdec avif_apps)

     if(NOT SKIP_INSTALL_APPS AND NOT SKIP_INSTALL_ALL)
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index d174e05..b8d4b10 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -223,6 +223,9 @@ if(AVIF_BUILD_APPS)
     # When building apps, test the avifenc/avifdec.
     # 'are_images_equal' is used to make sure inputs/outputs are unchanged.
     add_executable(are_images_equal gtest/are_images_equal.cc)
+    if(MINGW)
+        target_link_options(are_images_equal PRIVATE -municode)
+    endif()
     target_link_libraries(are_images_equal aviftest_helpers)
     add_test(NAME test_cmd COMMAND bash ${CMAKE_CURRENT_SOURCE_DIR}/test_cmd.sh ${CMAKE_BINARY_DIR}
                                    ${CMAKE_CURRENT_SOURCE_DIR}/data

[wantehchang]

Some webpages also say if wmain() is defined in a C++ file, it needs to be declared as extern "C" for the g++ in MinGW. In my testing I don't need to make that change to are_images_equal.cc.

[kmilos]

FYI, MINGW is defined for both GCC and Clang. See

https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Platform/Windows-GNU.cmake
https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Platform/Windows-Clang.cmake

In the CI, you can/should test both UCRT64 and CLANG64 environments.

Btw, don't you want all those (or some) CMAKE_C_COMPILER_ID MATCHES "GNU" checks to apply to Clang as well?

[wantehchang]

If MINGW is identified as GCC, we will need to handle it as follows:

elseif(CMAKE_C_COMPILER_ID MATCHES "GNU")
    message(STATUS "libavif: Enabling warnings for GCC")
    add_compile_options(-Wall -Wextra)
    if(MINGW)
       add_compile_options(-municode)
    endif()
elseif(MSVC)

Posted a patch that I tested

[kmilos]

Just in case you want to make the CI faster, MSYS2 ships googletest already...

[vrabaud]

Right, but I thought (just like on Ubuntu), that it was better to compile gtest from source to make sure the same compile flags are passed?

[vrabaud]

Ok, I added a working CI.
@kmilos , it is now failing for pure code reasons: -municode is set, wmain is used, but setlocale(LC_ALL, ".UTF8") does not work. Any way you can help me with that? Thx!

[kmilos]

setlocale(LC_ALL, ".UTF8") does not work

@vrabaud It seems to not fail in UCRT64 and CLANG64 environments (as they use the modern UCRT), but obviously fail in the legacy MINGW32/64 ones as they use MSVCRT where .utf8 locales are non-existent. In MINGW32/64 (i.e. MSVCRT), there's no way around not using wstring/wchar_t and W APIs for i18n.

In order to just avoid the error, you can wrap the setlocale() call w/ #ifdef _UCRT I guess.

I guess what you need to decide/say is - libavif is dropping support for MSVCRT (MINGW32/64 and anything before Windows 10 1903 and VS 2015), and will have only UCRT builds (and CI) w/ UTF-8, so default on VS 2015 and later, and UCRT64+CLANG64 (and possibly CLANG32 if you really want to cover x86).

[vrabaud]

Thx @kmilos for the explanation. How about:

• I remove -municode for MINGW32/64
• I also check for __MINGW32__ and __MINGW64__ not being defined when defining INIT_ARGV with setlocale in apps/shared/avifutil.h?
• I disable the UTF8 test for MINGW32/64

This way, MINGW32/64 is supported, not just UTF8

[kmilos]

• I also check for __MINGW32__ and __MINGW64__ not being defined when defining INIT_ARGV with setlocale in apps/shared/avifutil.h?

These are defined for all MinGW environments (and it's redundant to check for __MINGW64__ like it is redundant to check for _WIN64 if you already check for _WIN32). They really just define the platform, not the actual toolchain details. I guess you really want to check for _UCRT as the runtime C library being used (by either MSVC or MinGW toolchains).

Good idea for the other two points!

Although, not sure how to tell the difference between e.g. MINGW64 vs UCRT64 from within CMake!? Some small test code?

[vrabaud]

Indeed, the only thing I found so far (from https://discourse.cmake.org/t/how-to-check-that-the-current-compiler-is-gcc-msys-when-using-ninja-generator/6698/5) is to run gcc -dumpmachine.

[kmilos]

Indeed, the only thing I found so far (from https://discourse.cmake.org/t/how-to-check-that-the-current-compiler-is-gcc-msys-when-using-ninja-generator/6698/5) is to run gcc -dumpmachine.

Again, not the same thing. There's no problem telling if you're building for MINGW vs MSYS/CYGWIN (those all exist in CMake as variables already, so there's no need for such acrobatics). You can also tell GCC vs Clang w/ CMAKE_{C,CXX}_COMPILER_ID. But AFAIK you can't tell between MSVCRT and UCRT? You can only e.g. check the MSYSTEM environment variable perhaps...

Btw, gcc -dumpmachine in the UCRT64 environment returns the same as in MINGW64: x86_64-w64-mingw32, it doesn't tell you anything about the the C runtime.

So something like if(MINGW AND NOT "$ENV{MSYSTEM}" MATCHES "(MINGW32|MINGW64)")?

[vrabaud]

Thx, @kmilos, that works ! Still I had to disable mingw32: some errors appeared that seem unrelated: https://github.com/vrabaud/libavif/actions/runs/6985029492/job/19008737494#step:3:79
mingw64 is fine though.

[wantehchang]

Vincent: I assume this pull request has stabilized. I will review it tomorrow (Thursday).

[wantehchang]

LGTM. Thank you very much for all your work on this.

[wantehchang]

Optional: This seems like an unrelated bug fix. Would be good to move it to a separate pull request. Or at least describe this bug fix in the commit message.

[vrabaud]

Done in commit message.