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

MinGW support #2360

Merged
merged 18 commits into from
Jun 19, 2018
Merged

MinGW support #2360

merged 18 commits into from
Jun 19, 2018

Conversation

ark0f
Copy link
Member

@ark0f ark0f commented Jun 5, 2018

All components work excluding:

  • ApacheConnector (MSYS2 required)
  • MySQL (MariaDB connector doesn't support MinGW)

Message compiler automatically disable in MinGW if it is not exists.
Add wrapper for wmain() that will be used in Application and ServerApplication. It can be disabled by POCO_NO_WMAIN_WRAPPER definition.
Change logic of NetworkInitialzer in Util.h.

Fix several tests.

Fix missing argument name in functions "setEscapeUnicode" in JSON package.

ark0f added 5 commits June 3, 2018 01:17
TODO list:
* Fix tests compilation (undefined reference to CppUnit::...)
* Fix SQLs compilation (No rule to make target '${LIBNAME}.dll.a', needed by '${LIBNAME}.dll'.  Stop.)
* Fix crypto executables compilation
* Test static compilation
* Test MSVC compilation
* Add unicode support

See pocoproject#2356
Also fix PDF test runner.

TODO list:
* Fix tests compilation (undefined reference to CppUnit::...)
* Add unicode support
* Resolve what to do with message compiler

See pocoproject#2356
…ral implementations. Add "POCO_NO_MINGW_UNICODE" for "wmain".

TODO list:
* Check PostgreSQL and MySQL
* Resolve what to do with message compiler

See pocoproject#2356
@@ -1,2 +1,4 @@
add_executable(genrsakey src/genrsakey.cpp)
target_link_libraries(genrsakey PUBLIC Poco::Crypto Poco::Util Poco::XML)

POCO_ENABLE_EXE_WMAIN(genrsakey)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bjoe can you please confirm this is the right thing to do

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my review comment...

UnicodeConverter::toUTF8(lpMsgBuf, errMsg);
#endif
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style

@@ -20,6 +20,9 @@
#include <set>


#define MAX_PATH 260
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please shield this with #ifndef MAX_PATH

else()
target_link_libraries(Net PUBLIC "ws2_32.lib")
target_link_libraries(Net PUBLIC "ws2_32")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I don't know yet if this will work with microsoft compile. But ws2_32 is better then ws2_32.lib .... I would like to investigate if cmake support .lib suffix for microsoft compiler...

NULL,
#else
0,
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please just use 0 here, but why is NULL not good here and is in the last argument?

Copy link
Member Author

@ark0f ark0f Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because function required number in MinGW


NULL,
#else
0,
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@aleks-f aleks-f requested a review from Bjoe June 5, 2018 15:44
@ark0f
Copy link
Member Author

ark0f commented Jun 5, 2018

Please, cancel build develop-3561 at AppVeyor as I pushed new commit.

Copy link
Contributor

@Bjoe Bjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMakeLists.txt Outdated
@@ -81,6 +85,10 @@ else()
option(BUILD_SHARED_LIBS "Build shared libraries" ON)
endif()

if(BUILD_SHARED_LIBS AND MINGW)
add_definitions(-D_DLL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use "add_definitons". Use target_compile_definitons ... See also CMake-contribution-rules

Copy link
Member Author

@ark0f ark0f Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define _DDL use almost all packages and dependencies. Do you offer to define it for every target? Or you offer define as PUBLIC?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should ask your self, if it is mandatory to set also _DLL into your project they use the compiled poco library ? Mostly I checked in the code if this preprocessor definiton is in the header file .... if not, then it can be PRIVATE (it is only need to compile the poco librarie) ... if it is in the header file, then you should investigate is this header file published with the compiled library ... if yes then it is mostly mandatory that you added this as PUBLIC because it is needed to use the compiled poco library into your project. You should not added these to every target. Added with target_compile_definition with PUBLIC there where it is used and this definiton will be provided to the other targets they use this target. For example for me we should add this on the Poco::Foundation target. All of the other targets depends on Poco::Foundation and get this definiton ...See also target_compile_definitions I found that _DLL is used in Crypto.h, CppUnit.h, Foundation,h, NetSSL.h, pngconf.h .... I see also we have POCO_DLL ... hmmm is this the same?

@@ -134,6 +134,10 @@ target_include_directories(PDF
PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src
)

if(MINGW)
add_definitions(-DHPDF_DLL_MAKE_CDECL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use add_definitons ... use target_compile_definitons. See also CMake-contribution-rules


macro(POCO_ENABLE_EXE_WMAIN target_name)
if(MINGW)
set_target_properties("${target_name}" PROPERTIES LINK_FLAGS "-municode")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-municode is needed to compile unicode applications. Should be activated when Uniicode support is enabled in poco or?
I found also this

Help! I get an error: unrecognized command line option "-municode"!

in MinGW wikiUnicode applications

Normaly CMake takes care that it works on all compilers and platforms. See also CMake rulse:

  1. Don't make any assumption about the platform or compiler!

I would like to investigate if cmake has any support for such compile flag.

Copy link
Member Author

@ark0f ark0f Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to compile application with wmain using MinGW. You'll get undefined reference to WinMain, if you don't add this option. Also see https://stackoverflow.com/a/11706847.
And quote from MinGW wiki:

While it is not necessary to define _UNICODE or UNICODE to compile the above code, -municode is needed for linking because it uses wmain() instead of the traditional main().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But wmain is needed when you will have unicode support or? For me it looks like this depends on POCO_ENBALE_WSTRING .... but I'm still investigating...

CMakeLists.txt Outdated
@@ -2,6 +2,10 @@ cmake_minimum_required(VERSION 3.2.0)

project(Poco)

if(WINCE AND MINGW)
message(FATAL_ERROR "MinGW does not support WinCE")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I should delete this. CMake will cope with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think also we should remove this.... mingw will not work at all on wince ...

CMakeLists.txt Outdated
@@ -2,6 +2,10 @@ cmake_minimum_required(VERSION 3.2.0)

project(Poco)

if(WINCE AND MINGW)
message(FATAL_ERROR "MinGW does not support WinCE")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think also we should remove this.... mingw will not work at all on wince ...

CMakeLists.txt Outdated
@@ -81,6 +85,10 @@ else()
option(BUILD_SHARED_LIBS "Build shared libraries" ON)
endif()

if(BUILD_SHARED_LIBS AND MINGW)
add_definitions(-D_DLL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should ask your self, if it is mandatory to set also _DLL into your project they use the compiled poco library ? Mostly I checked in the code if this preprocessor definiton is in the header file .... if not, then it can be PRIVATE (it is only need to compile the poco librarie) ... if it is in the header file, then you should investigate is this header file published with the compiled library ... if yes then it is mostly mandatory that you added this as PUBLIC because it is needed to use the compiled poco library into your project. You should not added these to every target. Added with target_compile_definition with PUBLIC there where it is used and this definiton will be provided to the other targets they use this target. For example for me we should add this on the Poco::Foundation target. All of the other targets depends on Poco::Foundation and get this definiton ...See also target_compile_definitions I found that _DLL is used in Crypto.h, CppUnit.h, Foundation,h, NetSSL.h, pngconf.h .... I see also we have POCO_DLL ... hmmm is this the same?

@@ -44,7 +44,6 @@
namespace Poco {
namespace Net {


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read contribution documentation about code formatting

@@ -31,8 +31,6 @@
// MongoDB_API functions as being imported from a DLL, whereas this DLL sees symbols
// defined with this macro as being exported.
//


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read contribution documentation about code formatting

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -31,7 +31,6 @@
// Poco_SQL_API functions as being imported from a DLL, whereas this DLL sees symbols
// defined with this macro as being exported.
//

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read contribution documentation about code formatting :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


macro(POCO_ENABLE_EXE_WMAIN target_name)
if(MINGW)
set_target_properties("${target_name}" PROPERTIES LINK_FLAGS "-municode")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But wmain is needed when you will have unicode support or? For me it looks like this depends on POCO_ENBALE_WSTRING .... but I'm still investigating...

@@ -232,7 +232,7 @@ class Util_API ServerApplication: public Application
//
// Macro to implement main()
//
#if defined(_WIN32) && !defined(__MINGW32__)
#if defined(_WIN32) && !defined(POCO_NO_MINGW_UNICODE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No unicode ? I was expected you should have wmain for unicode support?

Copy link
Member Author

@ark0f ark0f Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option -municode affect to WinAPI too. For example InetPton. It is define to InetPtonA or to InetPtonW. If option -municode set, InetPton will be InetPtonW. If not, InetPton will be InetPtonA.
What does mean A or W:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd374089(v=vs.85).aspx

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated in the URL

A Windows code page version with the letter "A" used to indicate "ANSI"
A Unicode version with the letter "W" used to indicate "wide"

@aleks-f
Copy link
Member

aleks-f commented Jun 16, 2018

I'm ok with changes, @zosrothko there's appveyor NetSSL test failure but I don't think it is related to this pull, can you check?

@aleks-f aleks-f requested a review from zosrothko June 16, 2018 00:02
Copy link
Contributor

@Bjoe Bjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix some finding in cmake.

@@ -10,7 +10,7 @@

#include "Poco/Platform.h"
// see https://github.com/openssl/openssl/blob/master/doc/man3/OPENSSL_Applink.pod
#if defined(POCO_OS_FAMILY_WINDOWS)
#if defined(_MSC_VER)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes I see, we use POCO_OS_FAMILY_WINDOWS and sometimes we use _MSC_VER .... May I ask what is the difference?

Copy link
Member Author

@ark0f ark0f Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenSSL for MinGW has no applink.c.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

endif()

if(MINGW) # Needed by WinAPI in MinGW
string(REGEX MATCH "^[0-9]+\\.[0-9]+" WIN_VERSION ${CMAKE_SYSTEM_VERSION})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cmake it is possible to verify version numbers with VERSION_LESS etc. See https://cmake.org/cmake/help/latest/command/if.html?highlight=version_less

Copy link
Member Author

@ark0f ark0f Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't verify version, it calculate it from version like 6.0.141241 to 0x0600.

  • Take first two numbers 6 and 0, so we get 6.0
  • It take major version, so we get 6
  • It take minor version, so we get 0
  • Transform these two numbers to version like WinAPI. 256 * 6 + 0 = 1536 or 0x0600
    If windows version differ, WinAPI version will be different too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor

@Bjoe Bjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

endif()

if(MINGW) # Needed by WinAPI in MinGW
string(REGEX MATCH "^[0-9]+\\.[0-9]+" WIN_VERSION ${CMAKE_SYSTEM_VERSION})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -31,8 +31,6 @@
// MongoDB_API functions as being imported from a DLL, whereas this DLL sees symbols
// defined with this macro as being exported.
//


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -10,7 +10,7 @@

#include "Poco/Platform.h"
// see https://github.com/openssl/openssl/blob/master/doc/man3/OPENSSL_Applink.pod
#if defined(POCO_OS_FAMILY_WINDOWS)
#if defined(_MSC_VER)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -31,7 +31,6 @@
// Poco_SQL_API functions as being imported from a DLL, whereas this DLL sees symbols
// defined with this macro as being exported.
//

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@aleks-f aleks-f merged commit 0379a1e into pocoproject:develop Jun 19, 2018
@filimonov
Copy link

MySQL (MariaDB connector doesn't support MinGW)

Did you try that?

pacman -S --needed mingw-w64-x86_64-openssl mingw-w64-x86_64-libmariadbclient

And after that apply patch from https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-poco/012-find-mysql.patch

Also you can review other patches from https://github.com/Alexpux/MINGW-packages/tree/master/mingw-w64-poco

@ark0f
Copy link
Member Author

ark0f commented Jul 31, 2018

But how use libmariadbclient outside of MSYS2?

@filimonov
Copy link

filimonov commented Aug 1, 2018

Static linking? And as you see from prefix mingw-w64-x86_64 it is mingw64 native, not msys2 dependant.

BTW: i've tryed to build latest develop of poco on mingw (on windows 7). It build ok, but lot of tests fail. Do tests pass on your build?

@filimonov
Copy link

Here are the package file with patches needed to support mariadbcliect on mingw platform https://github.com/Alexpux/MINGW-packages/tree/bdc31c969f802816604254c938c524d3a8283530/mingw-w64-libmariadbclient

@ark0f
Copy link
Member Author

ark0f commented Aug 1, 2018

About tests. As I understand, it are require run some services, eg. Redis, MySql, etc. Also some packages are not in release yet.

@ark0f
Copy link
Member Author

ark0f commented Aug 1, 2018

And about patches. I'll try to use them myself on mariadb-connector-c.

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

Successfully merging this pull request may close these issues.

5 participants