Page MenuHomePhabricator

[debuginfo-tests] Don't look for Python 3 if we already have it
ClosedPublic

Authored by rnk on Thu, Oct 31, 2:03 PM.

Details

Summary

LLDB already requires Python 3 on Windows, so I already configure it
that way. For some reason CMake fails to find the one that Visual Studio
automatically installs at this standard location:

C:/Program Files (x86)/Microsoft Visual Studio/Shared/Python37_64/python.exe

CMake prefers the python on path, which happens to be python 2.7.

Diff Detail

Event Timeline

rnk created this revision.Thu, Oct 31, 2:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Oct 31, 2:03 PM
Herald added a subscriber: mgorny. · View Herald Transcript

I thought we already had a check in place that refuses to run debuginfo-tests for Python < 3? Can this be removed entirely?

rnk added a comment.Thu, Oct 31, 2:24 PM

I thought we already had a check in place that refuses to run debuginfo-tests for Python < 3? Can this be removed entirely?

Yes, this is that check. I've already configured cmake with Python 3, but it throws all that info away and re-runs find_package, which finds Python 2 instead of 3. The intention of this logic is to... just not do anything if we already have python 3.

rnk added a comment.Thu, Oct 31, 2:26 PM

Nevermind, I've done it poorly in my haste to fix the cmake error. :(

rnk updated this revision to Diff 227349.Thu, Oct 31, 2:28 PM
  • Do a better job.

Ah. So this code allows debuginfo-tests to use python3 even if LLVM was configured with python 2.7 and you have both versions installed.

jmorse accepted this revision.Fri, Nov 1, 5:58 AM

Ah yep, that's broken, in fact when re-configuring I needed this locally too :/.

This revision is now accepted and ready to land.Fri, Nov 1, 5:58 AM
This revision was automatically updated to reflect the committed changes.

I'm running into an issue running clang tests that can be traced back to https://reviews.llvm.org/D68708.

I'm configuring with debuginfo-tests enabled and python2.7 as the python executable, but with a python3 also installed. The end result is that tools/clang/test/lit.site.cfg.py defines config.python_executable = "", which results in the weirdest of errors. That file is expanded from clang/test/lit.site.cfg.py.in where it is config.python_executable = "@PYTHON_EXECUTABLE@". I git-bisected it to D68708, which seems to make sense. I don't claim to fully understand what is going on here, but I believe that unseting PYTHON_EXECUTABLE is dangerous. Could we initialize a PYTHON3_EXECUTABLE variable here, or even better, just error out if PYTHON_EXECUTABLE is not python 3? Which users/bot are we supporting with this code?

(green dragon uses an all-python3 configuration now)

rnk added a comment.Thu, Nov 7, 2:32 PM

Would it be OK if we removed all the logic to redo the python search here, and replace it with a simple version check? So far as I know, most of the rest of LLVM is Python 3 ready, so even if you use python 2 on PATH, there's no reason not to tell cmake to use Python 3.

In D69684#1737796, @rnk wrote:

Would it be OK if we removed all the logic to redo the python search here, and replace it with a simple version check? So far as I know, most of the rest of LLVM is Python 3 ready, so even if you use python 2 on PATH, there's no reason not to tell cmake to use Python 3.

Very strong +1. Getting Python right (at least in CMake < 3.12) is pretty tricky as we've seen many times in LLDB.

(green dragon uses an all-python3 configuration now)

In D69684#1737796, @rnk wrote:

Would it be OK if we removed all the logic to redo the python search here, and replace it with a simple version check? So far as I know, most of the rest of LLVM is Python 3 ready, so even if you use python 2 on PATH, there's no reason not to tell cmake to use Python 3.

I would very much prefer a hard error over anything automagic. I just wanted to make sure no-one else ( @jmorse ) relies on this functionality.

jmorse added a comment.Fri, Nov 8, 3:18 AM

I would very much prefer a hard error over anything automagic. I just wanted to make sure no-one else ( @jmorse ) relies on this functionality.

We don't rely on it either, the testing-for-python-3 rather than a hard dependency was mostly motivated by being un-intrusive on peoples environments. It looks like that's way harder than I expected though.