Porting the library towards Windows and CMake compliant

[al-sabr]

Greetings !

I started a Windows port but came to a roadblock because I have no experience with Socket porting from Unix to Windows but somehow I found the bits an pieces from around the web and added new files and modified CMakeLists.txt file to take into consideration WIN32 with vcpkg for Visual Studio 2019 project generation.

I hope that it is going to lead somewhere.

https://github.com/al-sabr/rtmfp-cpp/tree/win32-port

Best Regards

[al-sabr]

Hi @zenomt I'm glad you saw my messages and you are right I am using another Browser which is not working well with GitHub. I will try with Opera. I was able to make your project fully CMake compliant with total compilation. I had to do some modifications in your code but I think that there's something like 5% work left to do for the Windows part in the following files:

You can correct me if I am wrong be my guest :D

https://github.com/al-sabr/rtmfp-cpp/blob/win32-port/src/PerformerWindowsPlatformAdapter.cpp
https://github.com/al-sabr/rtmfp-cpp/blob/win32-port/src/WindowsPlatformAdapter.cpp

If you do the necessary changes I will try to compile it and test the new features.

I also added all CMake Win32 and UNIX conditions in CMakeLists.txt and few of the C++ source files.

Best Regards

[zenomt]

i don't know anything about Windows programming, so i have no input on the Windows platform adapters.

a few notes on the diffs of your repo:

• i need to study the CMakeLists.txt changes more.
• note that RTMFP currently builds/runs on multiple Unix-like operating systems that i have access to: MacOS, several flavors of Linux (including Debian and in Termux natively on Android), and FreeBSD. i check them all whenever i commit a change.
• because of that, SelectRunLoop should always be built (since i see it's also built in your WIN32 flavor). EPollRunLoop is only for Linux, not any other Unix-like OS.
  • your proposed changes to include/rtmfp/RunLoops.hpp aren’t quite right; the #includes are inside namespaces, and SelectRunLoop.hpp is only included for not-__linux__. no changes to this file should have been necessary anyway.
• it looks like there's some whitespace mangling in some files (like in include/rtmfp/PosixPlatformAdapter.hpp and include/rtmfp/rtmfp.hpp)
• presumably “far” is special in Windows; i'll change far to farCert
• the #include <arpa/inet.h> does not belong in include/rtmfp/Address.hpp. if it doesn't exist on Windows, then it should be excluded in its original location in Address.cpp.
• i really really didn't want to have #if defined(WIN32) ... peppered throughout this project. it seems unavoidable to allow it to build at all on Windows though.
• be mindful of any code that you might check into your own repos to which you might not have a license (like anything copied from stackoverflow), or that has a different or incompatible license (like the GPL for socket_win_socket.c). note that no such contributions can be accepted in this repo.

in the coming days, or perhaps this weekend, i'll try to get to making some of the changes to make building on Windows possible. the changes needed in Performer.cpp make me particularly sad though.

i'm tempted to put a RTMFP_WIN32 #define in the target_compile_definitions in the CMakeLists.txt file so that checks for the 4 different possible Windows #defines aren't needed everywhere every time.

[zenomt]

it looks like #if defined(_WIN32) is sufficient to tell if the target is Windows.

[al-sabr]

in the coming days, or perhaps this weekend, i'll try to get to making some of the changes to make building on Windows possible. the changes needed in Performer.cpp make me particularly sad though.

Yes the code I wrote has everything in it to make anybody sad :D But at least you know where the changes have to be made. Maybe you need to abstract some parts Performer to BasePerformer so that we can granularize like PlatformAdapter.

[al-sabr]

Concerning the situation you are not in the position to develop for Windows I have found this evaluation VMs for development purpose.

https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/

You can set a ready to use environment in 6 minutes : https://www.youtube.com/watch?v=sc6_86jgQls

[al-sabr]

I am with you also concerning Windows socket programming I have no clue but I started to learn and that's the reason I have put socket_win_socket.c file in the source code so that we both have an idea how to adapt the PosixPlatformAdapter for Windows.

I was thinking that it would not to hard by copying it and adapting it for Windows.

Concerning the code parts I copied in the code I've putted there just to be able to compile since I didn't find the equivalent for Windows. But once we have found the right way I will remove all of it.

[zenomt]

how much of a concern is Universal Windows Platform (UWP)? the _pipe() call is not available in UWP. also, select() is not the appropriate way on Windows to construct a run loop (i recall from a very long time ago that you really want to be dealing with Handles or something -- this came up when we were developing MFP 16-18 years ago).

it might be more appropriate to exclude Performer and SelectRunLoop from Windows, with the expectation that similar functionality would be left to a set of Windows adapters that work in the expected Windows Way™.

[zenomt]

i unfortunately mingled the core RTMFP parts with convenience parts in the same library. the expectation though is that even on Posix systems, you don't need to use the provided RunLoops, Performer, Platform Adapters, or Crypto Adapters; you can and should use your own as appropriate (though the provided ones should be usable in many cases).

[hoche]

Hi, some random comments:

• It's not clear to me whether the Windows porting effort is an attempt to just make a demo for Windows, or actually provide a high-performance implementation. If the former, select() and the regular socket API can be used pretty much the same as on unix - and as with unix it's not the most efficient way to do a loop but it should work fine for a relatively small number of connections. If you were writing a real server you'd probably want to use overlapped IO and events on Windows and most likely would compartmentalize the code differently such that you had the RTMFP-specific stuff split out from all the generalized reactor stuff.
• I think there's an error in the windows version of the pipe stuff where _write() and _read() are called - you're passing in the address of the pointer to the buffer instead of the pointer itself. It should be pretty much identical to the POSIX write() and read(). Also the _pipe() function should have a much bigger buffer size - probably 64K if you want to match Linux.
• The stuff in arpa/inet.h is just the prototypes for the network/host byte ordering functions, and is in winsock.h. That should only be included in files where those functions are used in the code - in this case Address.cpp not Address.hpp.
• getopt() - no, no, no, no. You can't just grab GNU code and stick it in any project. It attaches constraints to the licensing that the MIT license doesn't have. There's an older BSD version with a license that might be OK from that standpoint if you want to embed that in this library, or since its use here is pretty lightweight you could roll your own. Either way, that shouldn't be in the RTMFP library at all - at best it should be in a separate directory, built as a separate library, and just linked in as necessary when the binaries are created.
• You can just ignore UWP. It's for apps, not servers. It's somewhat like Java in that stuff written for it gets compiled to a "universal" byte code for a runtime engine. Apps can then be written in a variety of languages, distributed on the Microsoft App Store, and then run on any Microsoft device that happens to have that runtime engine. It also walls you off from a bunch of native APIs and enforces a number of other restrictions.

[zenomt]

since this library can be used in any program (including random apps, and not just "servers"), i don't think we can ignore UWP.

[al-sabr]

@hoche Your random comments are well appreciated by us. We both have no XP with Windows Socket programming, any insight is more than welcomed.

[zenomt]

hi @al-sabr . a few more questions about your changes:

• why is <limits> #included in include/rtmfp/Address.hpp? i don’t see anything in src/Address.cpp that would need things defined in that system include file.
• why are there any changes to include/rtmfp/RunLoops.hpp? i don’t see anything in that file that should be a problem on Windows (that file doesn’t cause a problem on any other non-Linux system i build on)

[zenomt]

re RunLoops.hpp: since i'm planning to exclude SelectRunLoop and EPollRunLoop on WIN32, nobody on Windows would be including that file anyway, so it shouldn't matter if there's anything problematic in it for Windows.

[al-sabr]

I've reverted the changes and removed the unnecessary includes. It compiled well...

[zenomt]

i've pushed a checkpoint that contains all the changes i'm planning to make to the source. the scope is to "un-break" Windows, not to support it. i probably won't get to the CMakeLists.txt file until this weekend. @al-sabr please confirm the changed files work for you on Windows.

note that i will be removing SelectRunLoop, EPollRunLoop, Performer, and the PosixPlatformAdapters if WIN32 in the CMakeLists.txt.

[zenomt]

i did some of the CMakeLists.txt changes, except for the package config stuff. e4d5422

[zenomt]

also @al-sabr is the #include <ws2tcpip.h> in Session.cpp needed? that's also included in Address.hpp which gets loaded during compilation. EDIT: i removed it from Session.cpp since there should be no need for it.

[al-sabr]

also @al-sabr is the #include <ws2tcpip.h> in Session.cpp needed? that's also included in Address.hpp which gets loaded during compilation. EDIT: i removed it from Session.cpp since there should be no need for it.

We can remove it if it is declared in one root place.

[zenomt]

here's the diff showing all the changes so far: 9b17dbf...main

[zenomt]

the latest commit should finish up the CMakeLists.txt changes. seems to work on Linux, hopefully on Windows. @al-sabr please let me know.

[zenomt]

hm. the warning for sscanf should have been suppressed. the docs for _CRT_SECURE_NO_WARNINGS are imprecise, but the online consensus seems to be it needs defined before anything else is included. i moved it to the top of Address.cpp. hopefully that takes care of it.

[al-sabr]

hm. the warning for sscanf should have been suppressed. the docs for _CRT_SECURE_NO_WARNINGS are imprecise, but the online consensus seems to be it needs defined before anything else is included. i moved it to the top of Address.cpp. hopefully that takes care of it.

Good code !!!!!

[main] Building folder: rtmfp-cpp rtmfp
[build] Starting build
[proc] Executing command: "C:\Program Files\CMake\bin\cmake.EXE" --build d:/programming/cpp/rtmfp-cpp/build --config Debug --target rtmfp -j 10 --
[build] Microsoft (R) Build Engine version 16.11.2+f32259642 for .NET Framework
[build] Copyright (C) Microsoft Corporation. All rights reserved.
[build] 
[build]   Address.cpp
[build]   rtmfp.vcxproj -> D:\programming\cpp\rtmfp-cpp\build\Debug\rtmfp.lib
[build] Build finished with exit code 0

[hoche]

Generally _CRT_SECURE_NO_WARNINGS isn't set in the code itself; it's set in the Visual Studio project file. It ends up getting passed in as -D_CRT_SECURE_NO_WARNINGS every time the compiler is invoked.

[zenomt]

i only wanted it defined for the one file where i know i'm using an "unsafe" function in a safe way, not project-wide. this seemed like the least-gross way, given that all of the "if Windows one way, otherwise the Standard way" is inherently gross.

EDIT: plus having it in the source means i can have a comment explaining what it's for near where its influence is felt.

[hoche]

If you want to keep it that localized then the general practice is to leave _CRT_SECURE_NO_WARNINGS undefined and just disable the specific warning with a pragma in the code.