enhancement - working implementation on Windows10 with Visual Studio (...and Cygwin also) #33
Conversation
| IF UNAME_SYSNAME == "Windows": | ||
| from . cimport _mswin as mswin | ||
| ELSE: | ||
| from posix cimport dlfcn |
There was a problem hiding this comment.
Surprised no one has wrapped this actually.
There was a problem hiding this comment.
Not sure to get your point here (sorry, I'm not a native speaker). Could you clarify please?
There was a problem hiding this comment.
I'm surprised there's not a cross-platform library open for Cython.
There was a problem hiding this comment.
Oh OK. That would be great indeed! I must admit I didn't look for it. If not existing, this is perhaps opportunity for a new project...
pkcs11/_pkcs11.pyx
Outdated
| dlfcn.dlclose(self._handle) | ||
|
|
||
|
|
||
| IF UNAME_SYSNAME == "Windows": |
There was a problem hiding this comment.
Should this go in your mswin.pxd?
There was a problem hiding this comment.
I am assuming you mean the cdef _winerrormsg() code section surrounded with the IF statement.
I have a personal preference to put only headers declaration into .pxd files; Cython however allows code sections into these files. Fair enough, I'll move this as per request.
setup.py
Outdated
| from platform import system | ||
|
|
||
| # if compiling using MSVC, we need to add user32 library | ||
| libraries = ('user32',) if system() == 'Windows' else () |
| #pragma warning "Cygwin 64 bits build will only work with Cygwin64-compiled PKCS#11 modules" | ||
| #endif | ||
|
|
||
| #define CK_PTR * |
There was a problem hiding this comment.
So I understood these defines were different on the different platforms. Is that not the case?
There was a problem hiding this comment.
Short answer: not really...
Long one - take a deep breath:
PKCS#11 was originally designed with Windows platform in mind. ( and, by the time, when 16 bits binaries was still the norm, presumably). You had the ability to define many kinds of pointers ( e.g. far pointers vs short pointers). Hence the assumption these defines could change.
If we consider Today's platforms share, you mainly have Windows and unix ish ones. Incidentally, the same defines apply to all platform, for all the defines.
As an evidence, feel free to checkout this code on Github: https://github.com/Mastercard/pkcs11-tools/blob/master/include/cryptoki/cryptoki.h , from my project (making free advertisement, I recognize 😉 ) that compile nicely on Windows(cross-compiled), Linux, Freebsd, AIX, Solaris, MacOS,... All sharing the same header file.
Actually, PKCS#11 screwed up with one thing: defining CK_ULONG as unsigned int long, which yields a 64 bits wide integer on unix ish patforms. This is not an issue when the PKCS#11 drivers are all compiled through the same compiler that we use; it leads to discrepancies when e.g. you start using Cygwin64 - which implements gcc 64 bits like on a unix platform, and try to interface that code with MSVC-compiled PKCS#11 DLLs, where sizeof(unsigned long int) is always 32 bits, irrespective of the processor bit width.
Hope this answers your question.
There was a problem hiding this comment.
Thanks for that detailed answer.
|
Are the tests failing because it can't find It looks like the the |
|
I am currently abroad, and don't have the ability to progress on this, this week. Thanks for the questions/comments, I'll work this out when I'm back and push an update to the PR. |
|
There you go. For testing, I took the freedom to enhance a few side items:
Here is the result: (using softHSM2) As you can see, there remains an issue with a file descriptor left open. This is caused by subprocess.Popen, which I suspect not to behave properly on this platform. This error happens sometimes, and is likely not related to the library code. |
|
Wait a moment. I'm not sure why Travis has stopped reporting, but there's something to fix up: https://travis-ci.org/danni/python-pkcs11/jobs/493412106 |
|
😅 Here is the reason: Hence Travis legitimately complaining:
Too bad that Travis triggers only upon changes approval and not upon PR submission - I would have spotted it. (Can Github be configured that way?) I have adapted Travis config file to fix the issue, in my last commit. Travis seems happy now. |
|
I'm sure it used to, maybe it stopped because people were executing random denials of service. |
|
Thanks again for this. I'll try and do a release on the weekend. |
|
@danni kindly reminder ;) I was able to test compilation on Windows, the only prerequisite is VS Build Tools |
|
Thanks @avoidik. Just trying to figure out Windows CI. |
Hi danni, it's me again 😉
I my previous PR, I claimed that it was working on the Windows platform. I had to revise this assertion; it indeed compiles (on Cygwin), but it does not work properly.
After digging, it is possible to make it work with Cygwin 32 bits, by adding a
#pragma pack(1)before including the PKCS#11 header files; the situation is different with Cygwin 64 bits, as GCCsizeof(CK_ULONG)on this platform is different than on the 32 bits version, resulting in discrepancy for every structure. Since most of the PKCS#11 DLLs are built from MSVC, the interest is limited.Actually, the
#pragma pack()directive is mandatory on Windows platform, according to the PKCS#11 documentation.I decided therefore to add the code needed to compile with MSVC. It uses the native Windows calls. It took me several changes:
setup.pyand create a new header file that contains the pragmas_mswin.pxdfile that contains the definitions for Windows.IFCython precompiler directives.I have tested on the Windows platform against SoftHSMv2 for Windows, and it passes most of the tests (see result - two tests have failed because of an issue with
subprocess.Popen()on Windows).I hope you'll enjoy it. This PR together with the previous should solve issues #12 and #27, IMO.
Eric