[xmlsec] MinGW build pull request
Peter Budai
peterbudai at hotmail.com
Thu May 18 10:34:02 PDT 2017
Hi Alexey and Roumen,
thanks for the feedback, let me Reply on the questions/issues.
In order to be able to see exactly what is happening during the build, please see two separate CI process log:
The first build log is the git master, without the patch:
https://ci.appveyor.com/project/peterbud/mingw-packages/build/1.0.64
The second one is actually the build process using the proposed patch:
https://ci.appveyor.com/project/peterbud/mingw-packages/build/1.0.62
You can see not just the 32 and 64 bit build process, but also the results of the test themselves, which shows what works properly (all test are OK) and what is not working properly, starting approx. here:
https://ci.appveyor.com/project/peterbud/mingw-packages/build/1.0.62#L624
>a) src/gcrypt/app.c
>Not mingw* specific.
>What is wrong with long cast for printf %ld flag?
Indeed this has nothing to do with mingw. However gcc 6.3.0 gives the following warning with the original code:
warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
"min_version=%ld", (long)GCRYPT_MIN_VERSION);
See here:
https://ci.appveyor.com/project/peterbud/mingw-packages/build/1.0.64#L532
>c)
>- # To avoid problem with loading of a shared library (dlopen or
>- # equivalent) at run time on some platforms we need to link
>- # everything statically (it works without hack on 9x and under
>- # emulation; on nt 5.x (w2k,xp) the error is 998: "Invalid
>- # access to memory location").
>
>I guess that above comment describes issue clearly.
>If on OS version >= Win-7 issue does not exist then fine. May be issue
>was specific to 32-bit MS OS and does not exist for 64 bit .
>Unfortunately today I cannot test xmlsec in native environment.
>
>If I remember well Microsoft loader fail to load some shared
>libraries(dll) dynamically due to limited functionality (failure in
>initialization of some global variables(?)).
The shared library loading issue does not exists on Win10 (Windows 7 I have not tested), shared libraries are just working OK both on 32 and 64 bit.
See test results:
https://ci.appveyor.com/project/peterbud/mingw-packages/build/1.0.62#L624
The only problem I have found is that dynamic loading of libraries are done with LoadLibrary and without the full path. That means that tests which are relying on LD_LIBRARY_PATH alone are not working unfortunately without exporting also the PATH (similar to LD_LIBRARY_PATH): after that LoadLibrary also find the dynamic libraries in the tests scripts. After make install shared libraries are also working properly, and the system finds them even without full path.
So I agree that we need a more granular OS check here before forcing static libraries
>d) shared vs static build and definition of XMLSEC_STATIC
>Define of -DXMLSEC_STATIC=1 in configure is just plain wrong.
>
>First I would like to point that xmlsec uses libtool for builds in unix
>like environment
>The build is controlled by libtool flags exported to configure
> --enable-shared[=PKGS] build shared libraries [default=yes]
> --enable-static[=PKGS] build static libraries [default=yes
I understand that so far on Windows static libraries were forced, but I believe it is possible to build both static and shared libraries as well. What I have found is that if you attempt to build xmlsec on windows with static libraries using MSYS2/MINGW, build fails as xmlsec1.exe as XMLSEC_STATIC has _not_ been defined anywhere. The only place where XMLSEC_STATIC is defined is the msvc makefile, which is obviously not used by gcc:
https://github.com/lsh123/xmlsec/blob/master/win32/Makefile.msvc#L615
I’d suggest to define XMLSEC_STATIC in configure / configure.ac if static build is chosen, that would universally work, not just in case MSVC is used to build library. That means there is no need to define separately XMLSEC_STATIC in msvc makefile
Some final thoughts:
* In the current build I had to disable the gcrypt library, as the MINGW version of libgcrypt seems to have problems (which obviously results a few xmlsec test to fail). See issue reported here: https://github.com/Alexpux/MINGW-packages/issues/2460 . As soon as that will be fixed, I’ll enable libgrcrypt and look into all gcrypt related test cases as well.
* The modifications in export.h (although have not been reviewed) I believe are also needed, would be also great to see if you are OK with that?
* For b) tests/testrun.sh related comments I’ll come back soon, just I need more time.
Thank you once again for the thorough review, best regards,
Peter
----------------------------------------------------------------------
Date: Tue, 16 May 2017 23:55:52 +0300
From: Roumen Petrov <xmlsec at roumenpetrov.info>
To: "xmlsec at aleksey.com" <xmlsec at aleksey.com>
Subject: Re: [xmlsec] MinGW build pull request
Message-ID: <591B6758.8010902 at roumenpetrov.info>
Content-Type: text/plain; charset="utf-8"; Format="flowed"
Hi Aleksey,
Aleksey Sanin wrote:
> Roumen (and anyone else who has experience with MinGW environment),
>
> There is a pull request to make changes to MinGW build. Unfortunately,
> I have no experience on this platform and would love to get your
> feedback on whether this is the correct change or not:
>
> https://github.com/lsh123/xmlsec/pull/115/files
My recommendation is to reject the request for the reasons described below:
a) src/gcrypt/app.c
Not mingw* specific.
What is wrong with long cast for printf %ld flag?
b) tests/testrun.sh
Wrong . Not mingw* specific.
mingw* produce native binaries that use OS functionality .
priv_key_suffix is related to OS (mis)functionality. Definitely is
not related to mingw GNU compiler. It is not compile time flag. It is
runtime flag!
If I remember well it is hack for MS crypto store for certain version of
operation system.
Perhaps logic could be improved to be forward compatible. I mean
something like this "if OS version >= Win-XP then use methodA else methodB".
For example runtime in above case mean build is on win2k and using the
same build tree run tests on Win-XP, Win7 ... .
c)
- # To avoid problem with loading of a shared library (dlopen or
- # equivalent) at run time on some platforms we need to link
- # everything statically (it works without hack on 9x and under
- # emulation; on nt 5.x (w2k,xp) the error is 998: "Invalid
- # access to memory location").
I guess that above comment describes issue clearly.
If on OS version >= Win-7 issue does not exist then fine. May be issue
was specific to 32-bit MS OS and does not exist for 64 bit .
Unfortunately today I cannot test xmlsec in native environment.
If I remember well Microsoft loader fail to load some shared
libraries(dll) dynamically due to limited functionality (failure in
initialization of some global variables(?)).
So removing enable_static_linking="yes" and enable_crypto_dl="no" needs
addition clarification (testing on native platform) and today I cannot help.
=> Must be separate request.
d) shared vs static build and definition of XMLSEC_STATIC
Define of -DXMLSEC_STATIC=1 in configure is just plain wrong.
First I would like to point that xmlsec uses libtool for builds in unix
like environment
The build is controlled by libtool flags exported to configure
--enable-shared[=PKGS] build shared libraries [default=yes]
--enable-static[=PKGS] build static libraries [default=yes]
During make phase libtool shows one command but actually performs two
compilation - one for static and one for shared.
Logic should be in common header something like following
#ifdef PIE
# defines for shared build
#else
# defines for static build
#endif
Remark : libtool pass -DPIE if compilation is for shared library
Note I have no issue with current build system and I'm not sure why is
necessary to change code.
May be mingw* complier is old and issue is for mingw* gcc >= 6*.
e) extra
Out of scope but you may want to update libtool macros in configure script
Please find attached file "0004-remove-obsolete-AC_PROG_LIBTOOL.patch"
AC_PROG_LIBTOOL is defined for backward compatibility in libtool 2+ and
LT_INIT([win32-dll]) is enough.
Regards
Roumen Petrov
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-remove-obsolete-AC_PROG_LIBTOOL.patch
Type: text/x-diff
Size: 552 bytes
Desc: not available
URL: <http://www.aleksey.com/pipermail/xmlsec/attachments/20170516/8cb4b405/attachment-0001.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.aleksey.com/pipermail/xmlsec/attachments/20170518/e05a303b/attachment-0001.html>
More information about the xmlsec
mailing list