Skip to content
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

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

Closed
jpgpng opened this issue Aug 13, 2022 · 7 comments
Closed

Comments

@jpgpng
Copy link

jpgpng commented Aug 13, 2022

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
Copy link
Owner

tezc commented Aug 13, 2022

@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
Copy link
Author

jpgpng commented Aug 13, 2022

@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
Copy link
Owner

tezc commented Aug 13, 2022

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
Copy link
Author

jpgpng commented Aug 15, 2022

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
Copy link
Owner

tezc commented Aug 15, 2022

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
Copy link
Author

jpgpng commented Aug 16, 2022

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
Copy link
Author

jpgpng commented Aug 16, 2022

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.

@jpgpng jpgpng closed this as completed Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants