This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Loosen Python version check and ignore patch version
ClosedPublic

Authored by JDevlieghere on Jul 24 2019, 10:41 AM.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jul 24 2019, 10:41 AM

Interesting way to get from version to list! :)

Cosmetics: It looks like PYTHON_VERSION_STRING and PYTHONLIBS_VERSION_STRING are global and all others are local variables? Using lowercase names for locals makes it easier to see. As names are case-sensitive, it would also avoid the collision with the actual PYTHON_MAJOR/MINOR.

lldb/cmake/modules/LLDBConfig.cmake
222

I think CMake would not shadow, but overwrite them in directory scope (which is root here, so everywhere except the cache).

Use lowercase variables for locals.

labath added a subscriber: labath.Jul 24 2019, 11:05 PM
labath added inline comments.
lldb/cmake/modules/LLDBConfig.cmake
220–221

If this is the only reason, then maybe we could fix find_python_libs_windows to set the relevant variables instead (?)

JDevlieghere marked an inline comment as done.Jul 25 2019, 9:22 AM
JDevlieghere added inline comments.
lldb/cmake/modules/LLDBConfig.cmake
220–221

FindPythonLibs only sets PYTHONLIBS_VERSION_STRING, so we'd still have that to parse the major and minor version of that manually. It was a coin flip between keeping the parsing together and updating find_python_libs_windows. I'm happy to do either.

labath added inline comments.Jul 25 2019, 12:35 PM
lldb/cmake/modules/LLDBConfig.cmake
220–221

Yea, I see what you mean. I think I'd go with changing find_python_libs_windows just to minimize the difference between the windows and non-windows branches of the python-finding code (and possible side-effects of those differences), but it's not a big deal either way...

JDevlieghere marked an inline comment as done.

Export the same variables in find_python_libs_windows as FindPythonInterp and FindPythonLibs.

@amccarth I think this might explain the issues you were having last week. Without this patch, find_python_libs_windows is not setting the expected version variables. As a result, my check was complaining about a mismatch between the Python interpreter that LLVM found, and the libs that LLDB found, while LLDB was actually using a different Python interpreter, found by find_python_libs_windows.

lldb/cmake/modules/LLDBConfig.cmake
220–221

Sure, sounds good!

This revision is now accepted and ready to land.Jul 26 2019, 1:48 AM

Thanks.

BTW, I tried this on windows, and it didn't blow up in my face, I think it should be relatively safe. I couldn't reproduce the problems that @amccarth was experiencing, so I don't know if it fixes that problem. It would be good to know whether it does, as we could cherry-pick this to the release branch in that case.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2019, 7:27 AM

Thanks.

BTW, I tried this on windows, and it didn't blow up in my face, I think it should be relatively safe. I couldn't reproduce the problems that @amccarth was experiencing, so I don't know if it fixes that problem. It would be good to know whether it does, as we could cherry-pick this to the release branch in that case.

Sorry for the slow reply. I've been in a conference most of the week.

Since my problem tends to be a "just me" problem that doesn't affect the bots or apparently anyone else, I don't think a cherrypick is necessary.

By having only one version of Python on my machine, I was able to work around that problem. Unfortunately, I've been stuck on another problem since making that change (specifically, all dotest.py tests report "Python memory allocator called without holding the GIL" and crash). This, too, seems to be a "just me" thing, and I'm continuing to work on that this morning. Until I get that figured out, I don't think I want to perturb my environment further by re-installing a second version of Python. I suspect the other patch to eliminate multiprocessing from dotest.py might solve the GIL problem as well, but I want to know more first.

Just closing the loop. Regarding the "Python memory allocator called without holding the GIL" bugs: this is not a "just me" problem. It's for anyone who enables debug mode. This appears to be a test infrastructure problem, but it might be harmless. If I run in "release" mode, I get the same behavior as the Windows build bot.