-
Notifications
You must be signed in to change notification settings - Fork 473
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 enhancement #548
Mingw enhancement #548
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
Some modifications to your code are needed since you are requesting manual modifications to Python/Anaconda files which is not our way to proceed. Instead this should be handled automatically by overloading the distutils
compiler code.
Also we should trigger the CI compilation of Python modules for MinGW (which basically consists in reversing the commit 10530b9) to check that the build of the Python modules is successful.
#### Building python module with MinGW toolset | ||
If cython executable is installed in your PATH enviroment or provided to cmake command, the python module will be built as well. | ||
Usually, there is nothing special with MinGW toolset. But if you get `cannot find -lmsvcrt140`, you may need to modify your cygwinccompiler.py, | ||
which is in distutils folder (In anaconda, it should be `Anaconda3\lib\distutils\cygwinccompiler.py`) | ||
|
||
```python | ||
elif int(msc_ver) >= 1900: | ||
# VS2015 / MSVC 14.0 | ||
# return ['msvcr140'] | ||
return ['vcruntime140'] | ||
``` | ||
|
||
And then copy `vcruntime140.dll` to `/path/to/mingw-w64/lib`. This problem is due to msvc runtime library naming convention broken after VS2015. See [here](https://stackoverflow.com/questions/52943590/cython-missing-msvcr140-dll). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting the user to perform manual modifications to their Python installation is not our way to handle platform specific cases.
Actually, we are going to great length to avoid this and instead we are automatically handling such situations by overloading distutils
compilers such as in python/setup.py.in
Lines 78 to 106 in d9df845
# distutils assumes that all the files must be compiled with the same compiler | |
# flags regardless of their file extension .c or .cpp. | |
# JSBSim C++ files must be compiled with -std=c++1x but compilation on MacOSX | |
# fails when trying to compile C files with -std=c++1x. | |
# The class C_CxxCompiler adds C++ flags to compilation flags for C++ files only. | |
class C_CxxCompiler(UnixCCompiler): | |
def _compile(self, obj, src, ext, cc_args, extra_postargs, pp_opts): | |
_cc_args = cc_args | |
# Add the C++ flags to the compile flags if we are dealing with a C++ | |
# source file. | |
if os.path.splitext(src)[-1] in ('.cpp', '.cxx', '.cc'): | |
_cc_args = cpp_compile_flag + cc_args | |
UnixCCompiler._compile(self, obj, src, ext, _cc_args, | |
extra_postargs, pp_opts) | |
# The class BuildC_CxxExtension intercepts the build of the Python module and | |
# replaces the compiler by an instance of C_CxxCompiler that select the compile | |
# flags depending on whether it is a C++ source file or not. | |
class BuildC_CxxExtension(build_ext): | |
def build_extension(self, ext): | |
if self.compiler.compiler_type == 'unix': | |
old_compiler = self.compiler | |
self.compiler = C_CxxCompiler() | |
# Copy the attributes to the new compiler instance | |
for attr, value in old_compiler.__dict__.items(): | |
setattr(self.compiler, attr, value) | |
return build_ext.build_extension(self, ext) |
```python | ||
elif int(msc_ver) >= 1900: | ||
# VS2015 / MSVC 14.0 | ||
# return ['msvcr140'] | ||
return ['vcruntime140'] | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot find this code in Python official distutils
: https://github.com/python/cpython/blob/576e38f9db61ca09ca6dc57ad13b02a7bf9d370a/Lib/distutils/cygwinccompiler.py#L61 so I am a bit skeptical that a user can follow successfully the steps you are describing ?
@@ -56,6 +56,9 @@ compiler = new_compiler(compiler=compiler_name) | |||
if compiler.compiler_type == 'unix': | |||
cpp_compile_flags = ['-std=c++11'] | |||
cpp_link_flags = [] | |||
elif compiler.compiler_type == 'mingw32': | |||
cpp_compile_flags = ['-std=c++11', '-D_hypot=hypot'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask why you are requesting the flag -D_hypot=hypot
since AFAICT ::hypot
is not used in JSBSim ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review.
I use this macro as Python.h
header file declares ::hypot
function. I'm using anaconda with python 3.6, whose distutils may contain some modification code.
I close this PR for the moment. My branch will be working in progress for better MinGW compability support, to pass the CI python module building. If any new findings, I will be back here.:grin:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha OK, I did not think to check Python.h
. That's a good point 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we'll be more than happy to provide you some help so do not hesitate to re-open this PR if you need some support.
@@ -140,6 +140,10 @@ if compiler.compiler_type == 'unix': | |||
cpp_compile_flag = ['-std=c++14'] | |||
cpp_link_flags = [] | |||
link_libraries = [${JSBSIM_LINK_LIBRARIES}] | |||
elif compiler.compiler_type == 'mingw32': | |||
cpp_compile_flag = ['-std=c++14', '-D_hypot=hypot'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: why are you requesting the flag -D_hypot=hypot
?
@@ -140,6 +140,10 @@ if compiler.compiler_type == 'unix': | |||
cpp_compile_flag = ['-std=c++14'] | |||
cpp_link_flags = [] | |||
link_libraries = [${JSBSIM_LINK_LIBRARIES}] | |||
elif compiler.compiler_type == 'mingw32': | |||
cpp_compile_flag = ['-std=c++14', '-D_hypot=hypot'] | |||
cpp_link_flags = ['-Wl,-Bstatic'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this linker flags ?
FYI we previously investigated how to build Python modules with MinGW (see issue #285) but came to the conclusion that this was not feasible since Python officially dropped the support of MinGW. I'd be interested to have your feedback about the discussion we had in issue #285, and I'd be very glad if we could work around these limitations with your PR. |
Support for MinGW python extension build and matlab mex file build.