-
Notifications
You must be signed in to change notification settings - Fork 756
Use gcc on Linux, VS on Windows and CLang in other cases for geninterop #510
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
Conversation
Fix crash of python interpreter 3.5 64-bit in garbage collector
@testrunner123, thanks! @vmuriart, @tiran, @brianlloyd, @tonyroberts and @rickardraysearch, please review this. |
All reactions
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #510 +/- ##
==========================================
+ Coverage 76.89% 77.14% +0.24%
==========================================
Files 64 65 +1
Lines 5583 5617 +34
Branches 889 888 -1
==========================================
+ Hits 4293 4333 +40
+ Misses 1000 996 -4
+ Partials 290 288 -2
Continue to review full report at Codecov.
|
All reactions
Sorry, something went wrong.
We have some lookup code for Visual Studio paths in |
All reactions
Sorry, something went wrong.
I agree with @filmor about locating cl.exe from default directories, if the build is not started from VS command-line prompt with pre-configured paths. Also we should probably still allow to fall back to clang, since it is much lighter dependency on Windows than MSVC. Even pycparser recommends Clang for Windows: https://github.com/eliben/pycparser#31interaction-with-the-c-preprocessor For Linux only gcc should be fine. Finally right now this pull request is not testing this functionality with compilers due to |
All reactions
Sorry, something went wrong.
I just tried to search the paths for |
All reactions
Sorry, something went wrong.
Here is proper way to locate cl.exe, or more precisely to setup environment variables to get cl.exe into path: https://github.com/pypa/setuptools/blob/master/setuptools/msvc.py This above module monkey-patches distutils (just import setuptools) to provide location of cl.exe for up-to-date compiler locations (distutils.py updates on much slower cycle): https://github.com/python/cpython/blob/master/Lib/distutils/msvccompiler.py#L267 More details about this spaghetti code for msvc compilers in this article (see also comments): https://blogs.msdn.microsoft.com/pythonengineering/2016/04/11/unable-to-find-vcvarsall-bat/ |
All reactions
Sorry, something went wrong.
@denfromufa
I use CMake for build and it finds compiler paths automatically. I do not think that each package must do it on its own, because it is pretty complex problem.
There is no recommendation for Clang on linked page. Also there are no reports about problems with msvc/gcc on it. And I didn't meet any problems.
But there is no test in existing codebase for clang-based geinterop either. I believe it is a different topic to create automatic test for geninterop script. I have tested it manually with Python 2.7/3.4 on Ubuntu 64-bit, Python 3.5 on Windows 32-/64-bit. |
All reactions
Sorry, something went wrong.
It looks like now clang on Windows is also used from PATH in geninterop.py. Or where is its path set ? |
All reactions
Sorry, something went wrong.
@testrunner123 please address merge conflicts |
All reactions
Sorry, something went wrong.
Since geninterop is an internal script that only needs to be triggered when we want to support a new Python version, I don't thinkt it makes sense to change it right now. |
All reactions
Sorry, something went wrong.
What does this implement/fix? Explain your changes.
Improve generating on geninterop data
Does this close any currently open issues?
#458
Any other comments?
On Windows "cl.exe" must be in PATH, on Linux "gcc".
Checklist
Check all those that are applicable and complete.
AUTHORS
CHANGELOG