-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Improve CMake Python Detection #30753
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
Improve CMake Python Detection #30753
Conversation
module FindPythonInterp was deprecated in 3.12. This patch replaces the deprecated CMake module with one of the suggested modules FindPython2. This new module looks only for version 2 of Python.
Only to comment on the philosophy of implementation here - I'm by no means a CMake expert. Shouldn't we be looking to have the default of Python 3 where possible? Backwards compatibility is still important because macOS ships python 2.7 by default, but it seems like we should be aiming to support a world where that constraint goes away. If the scripts are not both 2/3 compatible yet, we should look into fixing that first. |
Thanks for reviewing this.
I absolutely agree.
Your suggestion definitely seems like a reasonable approach to this. Right now there are a number of piecemeal MRs/commits that are trying to do this. Unfortunately it just seems that those patches for whatever reason are stagnating. Some are reviewed and merged. Some have not even been reviewed. Some that have been fixed have regressed. Since I cannot create CI nodes nor trigger runs here. My goal is to get a local CI node that executes the scripts in Python 3. #30662 and this are the only 2 changes needed for me to actually get my CI node to trigger the Python compatibility errors. Once I can easily see the Python errors I can then systematically submit patches until everything tests and runs with Python 3 on my CI node. Once that is done, then it would be demonstrable that Python 3 can be adopted for all nodes. In the end, I am open to suggestions on a better path forward. But without an advocate (i.e., someone who has the ability to trigger CI runs and merge) this circuitous method is all I can come up with that does not risk anything for the larger community while still achieving my goals. |
23daa99
to
8f96bfc
Compare
Sad that we need this, but, this is still an improvement over the status quo and maintains the existing behaviour. |
@swift-ci please test |
I am working on making GYB compatible with Python 2/3. I have made my own CI runners for 18.04, 19.10 and 20.04 and I have all but like 10 tests passing for both Unfortunately, some of what I'm having to do to make that work requires back-porting a bunch of Lit fixes from upstream. The good news is those back-ports are only really necessary for an environment without Python 2 (like 20.04). |
Also, I looked at the Windows build failure and I am not really sure what to do about it. Can you please advise? |
@swift-ci please test Windows platform |
1 similar comment
@swift-ci please test Windows platform |
Windows issue is fixed by #30905 |
#30695 updated the expected/required CMake version to 3.16.5. The CMake module FindPythonInterp was deprecated in CMake 3.12.
Patch 8f96bfc replaces the deprecated CMake module with one of the suggested modules FindPython2. This module looks only for version 2 of Python.
The second patch, 23daa99dff1005b41a657f7cd87b5ce493df1650, is aspirational and if a reviewer decides it is out of scope can easily be removed from this merge request.
My goal is to have CMake find Python 2 or 3 and to prefer Python 2 over 3. The thinking here is that in the currently supported build environments it will find Python 2 and work as it currently does. On newer, not yet supported, build environments that only have Python 3 it will still find a Python.
As is builds will still fail on those platforms, because of Python 2/3 syntax incompatibility, but as those incompatibilities are patched eventually Python 3 will just work. In my opinion, this is a good compromise between backwards compatiblity while looking forward to Python 3. At some point in the future when Python 3 is preferred all of the changes in 23daa99dff1005b41a657f7cd87b5ce493df1650 get reduced to
find_package(Python COMPONENTS Interpreter REQUIRED)
🎉.Initially my naive patch using the FindPython module broke compatibility with macOS and previous versions of Ubuntu because they can have 2 and 3 installed side by side.