tezc / sc

Common libraries and data structures for C.
BSD 3-Clause "New" or "Revised" License
2.23k stars 239 forks source link

#if defined(_WIN32) || defined(_WIN64) is not enough to check for MSVC #94

Closed jpgpng closed 1 year ago

jpgpng commented 2 years ago

For compatibility reason I think, MinGW-W64 also defined these macros. To be sure it's actually MSVC, use this:

if defined(_WIN32) || defined(_WIN64)

if defined(_MSC_VER)

pragma warning(disable : 4996)

endif

endif

MinGW-W64 doesn't understand that pragma. BTW, even if we applied this additional check I can't get sc_log.c to compile. It seems you only have MSVC in mind when you wrote this library, don't you?

tezc commented 2 years ago

@jpgpng Yes, you're right. I didn't try with mingw and I don't have any intention to support it to be honest. If it requires minor tweaks, it is okay, we can merge it. If it requires bigger changes, I can't promise anything. I don't have time to setup a mingw environment and maintain the compatibility. Contributions are welcome though, so, feel free to send fixes :)

jpgpng commented 2 years ago

@jpgpng Yes, you're right. I didn't try with mingw and I don't have any intention to support it to be honest. If it requires minor tweaks, it is okay, we can merge it. If it requires bigger changes, I can't promise anything. I don't have time to setup a mingw environment and maintain the compatibility. Contributions are welcome though, so, feel free to send fixes :)

I understand. MinGW-W64 supports two thread model: one is Win32 like MSVC and one is POSIX thread. The one I'm using is POSIX thread and most MinGW-W64 distributions out there are also using POSIX thread. To cover this, one only has to tweak the checking a bit by also cover #if defined(__MINGW32__) as __MINGW32__ is defined on both MinGW-W64 32 bit and 64 bit.

tezc commented 2 years ago

Okay, I see, thank you. I guess it must be done in other files as well. I think proper way to do it is adding a mingw build to the CI and fix all the compile issues. If you feel like doing it, please go ahead and create a PR :) Otherwise, I'm not sure when I'll have some time to do it. Let's leave the issue open until we fix it.

jpgpng commented 2 years ago

Okay, I see, thank you. I guess it must be done in other files as well. I think proper way to do it is adding a mingw build to the CI and fix all the compile issues. If you feel like doing it, please go ahead and create a PR :) Otherwise, I'm not sure when I'll have some time to do it. Let's leave the issue open until we fix it.

Well, I think your library is too Linuxism. I tried to compile on Cygwin which is a full POSIX environment and it failed at sc_socket (sc_sock.c) with error regarding sc_sock_notify_systemd (do we have anything to do with systemd at all?). This means it will very likely to also fail on other POSIX systems like BSDs and Solaris. In summary, it will fail on any platforms that is not Linux.

tezc commented 2 years ago

I mentioned tested environments here: https://github.com/tezc/sc#test

So, CI runs on Linux, Windows, MacOS (fork of a BSD) and on FreeBSD. Primary focus is Linux ofcourse. We've never used these libraries on Cygwin, Mingw or on Solaris. So, these are not among our target environments.

sc_sock is not an easy-use library itself. Main focus is Linux but it also works on above OSes. Support for other OSes are just to make development easier and it helps with testing.

If you are going to use these OS specific libraries with another environment like Cygwin or Mingw, you need to make some adjustments in the code unfortunately.

jpgpng commented 2 years ago

I just find out another problem at line 53 of sc_sock.h. MinGW-W64 doesn't support this pragma (it seems it will just ignore this pragma altogether but I'm not sure): #pragma comment(lib, "ws2_32.lib")

If you want to cover MinGW-W64 you need to move this logic to CMakeLists.txt.

jpgpng commented 2 years ago

If you are going to use these OS specific libraries with another environment like Cygwin or Mingw, you need to make some adjustments in the code unfortunately.

I understand. Thank you. BTW, MinGW-W64 is not a POSIX environment like Cygwin, the binary compiled by it is a native Win32 application that doesn't link with cygwin1.dll nor msys-2.0.dll. I think you confused MinGW-W64 with MSYS2.

Update: almost no one uses the original mingw and msys anymore, it's all MinGW-W64 and MSYS2 nowadays. Perhaps you should mention that it only supports MSVC on Windows in README.md.