Compatibility for Windows

[ghost]

Hello,

I made few adjustments to code so it compiles on Vs 2013.

[dbahrdt]

Hi, thank you for your work. There are some files in your pull request that are auto-generated on linux. These are the .pb. files. Can these be auto-generated on windows as well?

[ghost]

I have tried using both Protobuf 3.0 and Protobuf 2.6.1. When creating solution file with Cmake and specifying the directory with sources, in first case you get error that generated proto files were newer and in the second that they are older. Only working solution is to compile protoc on Windows and generate them manually.

UPDATE: I have removed the compiled .pb files, updated module dependencies for Windows and provided working instructions in Readme.md. I have tested it and it works. I think you may pull this onto master. Just before that please test if i didn´t broken it up for Linux. Have a nice day

[dbahrdt]

Before we can merge your changes, we may consider changing the license to a more permissive one. Adding #4 as dependency. We also need the license of mman-win32 to be compatible. So far I did not find any information about the license of mman-win32.

[ghost]

Mman-win32 has MIT license, see here https://code.google.com/archive/p/mman-win32/. I am not a lawyer, but i thought it almost doesn´t matter which license you use as long as its open source, it should be free to use,right, so what´s the problem with licenses anyway? Just use MIT.

[dbahrdt]

MIT is fine. The license is important since some licenses are not compatible to each other. For example the GPL-2 and GPL-3 licenses are impossible to combine. Currently osmpbf is licensed under LGPLv3 only. We're currently only 2 people working on the library. As soon as we merge your code and accept code from more people it becomes impossible to change the license if there's a need to do so. That's why we have to decide now if we really want to stick to the LGPLv3 only licensing.

[ghost]

I don´t know if its only Windows problem, but i have even tried to downgrade to 2.6.1 Protobuf,but i still have memory leaks coming from Protobuf. Can you also confirm this on Linux?

[dbahrdt]

I'm using 2.6.1 without any problems. But these are the system libraries from Debian. inphos has more experience in cross-platform programming. Unfortunately he currently does not have a lot of spare time. But he tries to review your pull-request on Sunday.

[dbahrdt]

As a first thing, can you re-factor the ifdefs into an extra files? For each OS one file that implements the wrapper functions and one file that selects the os-specific wrapper implementations. git-subprojects should only be used for code that is common to all systems. It would be better to have a script that pulls in the windows dependencies if the users wants to do so. The user may have them installed already. Furthermore the windows subprojects are not needed by most users. Please wait for inphos' comments before re-factoring your code.

Thanks again for your work.

[ghost]

I don´t know what´s the problem, when i am trying to build it as static library it build without problem,but when i link it empty main function creates 31 memory leaks.

UPDATE: Actually you have to call google::protobuf::ShutdownProtobufLibrary() to free up the resources. Then it works without problem, it wasn´t a problem with the library. Ouch spent 7 hours trying to figure out the problem.

[ghost]

.git-subprojects should only be used for code that is common to all systems. It would be better to have a script that pulls in the windows dependencies if the users wants to do so. The user may have them installed already. Furthermore the windows subprojects are not needed by most users.

I have added script which asks user if he wants to download the dependency from github repo.

As a first thing, can you re-factor the ifdefs into an extra files? For each OS one file that implements the wrapper functions and one file that selects the os-specific wrapper implementations.

I don´t understand why ifdef is wrong. It is not going to break compatibility with current linux version or do you have problem compiling my modifications? I am sorry but i don´t know what you have in mind, i have never done something like this, i can´t imagine how to do it.

[dbahrdt]

I've merged your stuff into the multi-os-support branch. Can you try that version? I don't have windows to test the changes.

[ghost]

I was able to compile with very small changes, it compiles OK,but when i try to open file,it can´t mmap properly. These two lines doesn´t succeed on Windows: m_FileData = (char *) mmap(0, m_FileSize, MM_PROT_READ, MM_MAP_SHARED, m_FileDescriptor, 0); if (osmpbf::validMmapAddress(m_FileData))

UPDATE: In blobfile.cpp you did check if osmpbf::validMmapAddress(m_FileData) then it should throw error,but this function checks if its valid,so it should only throw error if !osmpbf::validMmapAddress(m_FileData) is satisfied,because this function does check addr != MAP_FAILED. Great job anyways, ifdefs are not necessary now, i have updated the fork so it compiles on Windows properly after your changes :)

[dbahrdt]

I've merged your changes but I reverted your changes of fileio.h. You should keep the number of includes to a minimum. All the system file-io headers are not needed in fileio.h but only in fileio.cpp. If you put them into the header, then every file that includes fileio.h includes them as well for no apparent reason. There already are way too many includes in the header files right now.

[dbahrdt]

I've missed the "winsock2.h"/"netinet/in.h" discrepancy. Have to think about that for a moment.

[ghost]

I've merged your changes but I reverted your changes of fileio.h. You should keep the number of includes to a minimum. All the system file-io headers are not needed in fileio.h but only in fileio.cpp. If you put them into the header, then every file that includes fileio.h includes them as well for no apparent reason. There already are way too many includes in the header files right now.

That´s correct, because blobfile.cpp needs them too, i couldn´t compile without it so i put it into header file. If you didn´t put those includes into fileio.h,you broke the support for Windows.

[dbahrdt]

blobfile.cpp needed Winsock2.h for ntohl and htonl. These functions are now available through net.h which includes the appropriate headers in its definition.

[ghost]

Nicely done, it compiles and works without a problem now :) I think it will be a good idea to add to master´s readme some info that there exists another branch for multi-os support,so they can find the building instructions for Windows i have written there.

[dbahrdt]

If everything is working out of the box for you then I will merge the changes into master.

[ghost]

You can merge it.

[ghost]

I have tested the master branch now,it works correctly. Again you have done a great job. You can close this.