radarsat1 / liblo

liblo is an implementation of the Open Sound Control protocol for POSIX systems
GNU Lesser General Public License v2.1
192 stars 60 forks source link

WIN32: detect target correctly. #119

Closed carlo-bramini closed 2 years ago

carlo-bramini commented 2 years ago

When compiling the tests of RTOSC submodule of ZynAddSubFx, I got the same error signaled into issue #114. This happens because WIN32 is undefined. The right symbol to use is _WIN32 since it is intrinsic to the compiler. See:

https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?redirectedfrom=MSDN&view=msvc-170

Actually, testing for _MSC_VER has also no much sense here , because if _WIN32 does not exist then you surely cannot use or include WinSock.

Krasjet commented 2 years ago

There are quite a few instances of WIN32 left in the code base. Could you also replace them as well?

Rossmaxx commented 1 year ago

@carlo-bramini @JohannesLorenz is working on porting rtosc to msvc and I'm assisting him. When building, we face a linker error, see #145 . Can you help?

carlo-bramini commented 1 year ago

@Rossmaxx I have compiled liblo successfully on MSVC. However, I have some doubts that it will work on 64bit Windows. I did a number of changes and I think I fixed all the compatibility issues when compiling for Windows, so including MSVC. My fixes can be resumed into these points:

1) I adjusted the inclusion of the system header files. Macro _WIN32 is intrinsic and must be used for testing if, for example, winsock.h and others must be included. These system includes will then define WIN32 macro, letting the user to know that Windows APIs can be used. This means that, after including PSDK headers, you can just test WIN32 to know if a particular function is supported or not. This is a small piece of code as example:

#ifdef _WIN32
#include <windows.h>
#endif
//...
// After including Windows.h, macro WIN32 will be defined and you can test it...
#ifdef WIN32
Sleep(1000);
#else // !WIN32
usleep(1000*1000);
#endif

In my opinion, this is the correct way for handling _WIN32 and WIN32 macros. While somebody may think that only intrinsic _WIN32 can be used, but this won't be a good idea. Let's think to build for CYGWIN: this environment come with a full POSIX layer, but it has also a customized version of W32API header files. These header files can be used together with POSIX includes and they allow to use also Windows APIs. But CYGWIN does not provide _WIN32 macro, so if this is the initial scenario before including Windows.h:

MINGW/MSVC:
_WIN32 defined
WIN32 not defined

CYGWIN:
_WIN32 not defined
WIN32 not defined

then you can do something like this:

#if defined(_WIN32) || defined(__CYGWIN__)
#include <windows.h>
#endif

and you will get:

MINGW/MSVC:
_WIN32 defined
WIN32 defined

CYGWIN:
_WIN32 not defined
WIN32 defined

After that, also CYGWIN could use some native Windows APIs, if you want, because WIN32 now exists after Windows.h. However, talking about sockets, mixing POSIX and WinSock is usually not a good idea and it must be handled carefully. Basically, the fixes into PR #119 covered few points that blocked building on MSVC, but a cleanup for removing useless tests on _MSC_VER could be also done, if someone wants.

2) At the moment, liblo assumes that a socket handle is an int type, while it is an opaque object called SOCKET on WinSock. The definition of SOCKET into WinSock is a typedef to UINT_PTR, which has a different size of an int type. So, it is technically possible that some information is lost here. For fixing this behaviour, I declared this:

typedef SOCKET lo_socket;

when WIN32 is defined and this:

typedef int lo_socket;
#define INVALID_SOCKET -1

when it is not and I adjusted all their uses into liblo. There are just two points that I could not fix. Here, because I did not understand the code: https://github.com/radarsat1/liblo/blob/02989c6c69f54157b8d9651fb8d9ad6de8128d59/src/server.c#L1787 and here because this is an exported function of the DLL: https://github.com/radarsat1/liblo/blob/02989c6c69f54157b8d9651fb8d9ad6de8128d59/src/server.c#L2311 it returns an int type, but the socket handle is not, so I did a cast to int for not breaking the ABI of liblo.

3) Few fixes here and there, thanks to the warnings signaled by MSVC.

Since I have these two missing fixes not implemented into point (2), I have not created a PR with these changes. However, it seems that it is missing information on how liblo has been used. I tried to configure zynaddsubfx, which seems to be the project you are trying to compile, but I could not do it successfully even by using vcpkg. Since liblo compiles and link itself successfully standalone, perhaps this issue #145 has not been been addressed to the right project and without the steps for reproducing it.

Rossmaxx commented 1 year ago

I tried to configure zynaddsubfx, which seems to be the project you are trying to compile, but I could not do it successfully even by using vcpkg.

Not exactly. I'm trying to compile rtosc, which is a submodule of zyn. Once we get rtosc done, I will attempt a port of zyn.

Also, found more info than I posted earlier. It seems to be a linker error which uses -L instead of /LIBPATH: on MSVC too along with gcc. I can't seem to find definition for where the flags are set.

Also, if you wanna try it yourself, use the ci branch of rtosc instead of master, where the porting is going on.