Skip to content

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

Merged

Conversation

RLovelett
Copy link
Contributor

#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.

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.
@CodaFi
Copy link
Contributor

CodaFi commented Apr 2, 2020

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.

@RLovelett
Copy link
Contributor Author

Thanks for reviewing this.

Shouldn't we be looking to have the default of Python 3 where possible?

I absolutely agree.

If the scripts are not both 2/3 compatible yet, we should look into fixing that first.

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.

@RLovelett RLovelett force-pushed the replace-deprecated-cmake-python-modules branch from 23daa99 to 8f96bfc Compare April 3, 2020 12:39
@RLovelett RLovelett requested a review from compnerd April 3, 2020 12:40
@compnerd
Copy link
Member

compnerd commented Apr 8, 2020

Sad that we need this, but, this is still an improvement over the status quo and maintains the existing behaviour.

@compnerd
Copy link
Member

compnerd commented Apr 8, 2020

@swift-ci please test

@RLovelett
Copy link
Contributor Author

Sad that we need this, but, this is still an improvement over the status quo and maintains the existing behaviour.

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 --preset=buildbot_incremental_linux and --preset="buildbot_incremental_linux,long_test". I feel like I am so close.

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).

@RLovelett
Copy link
Contributor Author

Also, I looked at the Windows build failure and I am not really sure what to do about it. Can you please advise?

@compnerd
Copy link
Member

compnerd commented Apr 8, 2020

@swift-ci please test Windows platform

1 similar comment
@compnerd
Copy link
Member

compnerd commented Apr 9, 2020

@swift-ci please test Windows platform

@compnerd
Copy link
Member

compnerd commented Apr 9, 2020

Windows issue is fixed by #30905

@compnerd compnerd merged commit 2a977f0 into swiftlang:master Apr 9, 2020
@RLovelett RLovelett deleted the replace-deprecated-cmake-python-modules branch April 15, 2020 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants