Page MenuHomePhabricator

build: improve python check for Windows
ClosedPublic

Authored by compnerd on Oct 28 2019, 2:46 PM.

Details

Summary

If we have a new enough CMake, use find_package(Python3). This version is able to check both 32-bit and 64-bit versions and will setup everything properly without the user needing to specify PYTHON_HOME. This enables building lldb's python bindings on Windows under Azure's CI again.

Diff Detail

Event Timeline

compnerd created this revision.Oct 28 2019, 2:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 2:46 PM

I proposed this in https://reviews.llvm.org/D64881 and the consensus was that we didn't want to do this.

Also I don't think this should be a Windows-only thing.

The reason for bringing this back up as a Windows specific thing is that currently, there is no good way to build LLDB with python support without having to specify additional details on *just* windows because the windows path is doing something special. This is trying to bring the windows path to parity with the Linux path.

Yeah, I'd still like to avoid having multiple paths for configuring python, as one of them is bound to end up broken sooner or later. However, given that there's already talk about bumping the minimum cmake version, maybe we could do an lldb-specific bump first?

Though I am generally against aggressive version bumps, this python+windows situation is sufficiently messy that I think the bump may be worth it. Given that this code is python3 specific and other platforms still support python2, maybe we can make this bump windows-specific at first, and then extend it to other platforms once we officially stop supporting python2 ?

[+ other windows stakeholders: @amccarth, @ted, @aleksandr.urakov ]

lldb/cmake/modules/LLDBConfig.cmake
225

What if I use a single-config generator and build a release configuration. Is the "development" thingy still required?

compnerd marked an inline comment as done.Oct 29 2019, 6:51 PM

Yeah, doing an incremental rollout makes sense.

lldb/cmake/modules/LLDBConfig.cmake
225

Yes, it is - the component is the headers for building.

.. Given that this code is python3 specific and other platforms still support python2, maybe we can make this bump windows-specific at first, and then extend it to other platforms once we officially stop supporting python2 ?

Yes, please. It's a mess on Windows, so I'll support a Windows-specific version bump.

compnerd updated this revision to Diff 230658.Nov 22 2019, 8:36 AM

Use @labath's suggestion of bumping minimum required version for Windows

This looks fine to me, and it's really great to get rid of the that code, but it would be good for some some "windows person" to ack this too. I'm particularly interested in Stella's opinion, as according to http://lab.llvm.org:8011/buildslaves/win-py3-buildbot, her bot still runs cmake 3.11. @stella.stamenova, would it be possible to upgrade that bot to cmake>=3.13 ?

stella.stamenova accepted this revision.Dec 9 2019, 1:34 PM

Sorry, I was out on vacation until today :).

The bot is actually running 3.14 already, I updated the host file to reflect that.

This revision is now accepted and ready to land.Dec 9 2019, 1:34 PM