This is an archive of the discontinued LLVM Phabricator instance.

Differential D67641

Cache PYTHON_EXECUTABLE for windows
ClosedPublic

Authored by hhb on Sep 16 2019, 4:39 PM.

Details

Reviewers
JDevlieghere
Commits
rG5f8a3270ae74: Cache PYTHON_EXECUTABLE for windows
rLLDB372194: Cache PYTHON_EXECUTABLE for windows
rL372194: Cache PYTHON_EXECUTABLE for windows
Summary

This way it can be overwritten when cross compiling.

Diff Detail

Repository
rL LLVM

Event Timeline

hhb created this revision.Sep 16 2019, 4:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 4:39 PM
Herald added subscribers: lldb-commits, mgorny. · View Herald Transcript
hhb added a reviewer: JDevlieghere.Sep 16 2019, 4:41 PM
Harbormaster completed remote builds in B38185: Diff 220405.Sep 16 2019, 4:42 PM
JDevlieghere added a comment.Sep 16 2019, 4:52 PM

Can we match what FindPythonInterp and FindPythonLibs does?

mark_as_advanced(
  PYTHON_EXECUTABLE
  PYTHON_DEBUG_LIBRARY
  PYTHON_LIBRARY
  PYTHON_INCLUDE_DIR
)
hhb updated this revision to Diff 220410.Sep 16 2019, 5:16 PM

Add mark_as_advanced.

hhb added a comment.Sep 16 2019, 5:18 PM
In D67641#1671881, @JDevlieghere wrote:

Can we match what FindPythonInterp and FindPythonLibs does?

mark_as_advanced(
  PYTHON_EXECUTABLE
  PYTHON_DEBUG_LIBRARY
  PYTHON_LIBRARY
  PYTHON_INCLUDE_DIR
)

Added for PYTHON_EXECUTABLE.

PYTHON_HOME is required, and other variables are derived from PYTHON_HOME. I don't think anyone will be interested in setting them separately...

Harbormaster completed remote builds in B38186: Diff 220410.Sep 16 2019, 5:22 PM
JDevlieghere added a comment.Sep 16 2019, 5:41 PM
In D67641#1671917, @hhb wrote:
In D67641#1671881, @JDevlieghere wrote:

Can we match what FindPythonInterp and FindPythonLibs does?

mark_as_advanced(
  PYTHON_EXECUTABLE
  PYTHON_DEBUG_LIBRARY
  PYTHON_LIBRARY
  PYTHON_INCLUDE_DIR
)

Added for PYTHON_EXECUTABLE.

PYTHON_HOME is required, and other variables are derived from PYTHON_HOME. I don't think anyone will be interested in setting them separately...

With mark_as_advanced you don't have to modify the code above. Even if they're not needed, I'd still prefer to include the other variables as well, just to maintain consistency with was CMake does.

hhb updated this revision to Diff 220424.Sep 16 2019, 7:28 PM

Fix comment.

Harbormaster completed remote builds in B38189: Diff 220424.Sep 16 2019, 7:30 PM
JDevlieghere accepted this revision.Sep 17 2019, 9:23 AM

LGTM!

This revision is now accepted and ready to land.Sep 17 2019, 9:23 AM
hhb updated this revision to Diff 220584.Sep 17 2019, 4:35 PM

Turns out I still need to change previous lines to remove PARENT_SCOPE. Otherwise local value will still be used even if a different value is set in the cache.

Harbormaster completed remote builds in B38226: Diff 220584.Sep 17 2019, 4:42 PM
hhb updated this revision to Diff 220589.Sep 17 2019, 5:00 PM

Add CACHE PATH again. I think this is the only way to go.

With PARENT_SCOPE: Tthe local detected value is always used, even if a different value is set in cache.
With no parameter: Always the value in cache is used. If a value is not set in cache, empty is used.

Harbormaster completed remote builds in B38227: Diff 220589.Sep 17 2019, 5:01 PM
JDevlieghere accepted this revision.Sep 17 2019, 5:40 PM

Hmm, that's unfortunately but I guess it makes sense. Let's remove the mark_as_advanced as it doesn't really serve a purpose anymore. Apologies about the churn!

hhb updated this revision to Diff 220594.Sep 17 2019, 5:58 PM

Fix comment

Harbormaster completed remote builds in B38230: Diff 220594.Sep 17 2019, 6:00 PM
Closed by commit rL372194: Cache PYTHON_EXECUTABLE for windows (authored by hhb). · Explain WhySep 17 2019, 6:00 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2019, 6:01 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript

Revision Contents

PathSize
lldb/
trunk/
cmake/
modules/
8 lines

Diff 220596

lldb/trunk/cmake/modules/LLDBConfig.cmake

Show First 20 Lines • Show All 244 Lines • ▼ Show 20 Lines if(PYTHON_VERSION_STRING)
list(GET PYTHON_VERSION_PARTS 2 PYTHON_VERSION_PATCH) list(GET PYTHON_VERSION_PARTS 2 PYTHON_VERSION_PATCH)
else() else()
unset(PYTHON_VERSION_MAJOR) unset(PYTHON_VERSION_MAJOR)
unset(PYTHON_VERSION_MINOR) unset(PYTHON_VERSION_MINOR)
unset(PYTHON_VERSION_PATCH) unset(PYTHON_VERSION_PATCH)
endif() endif()
# Set the same variables as FindPythonInterp and FindPythonLibs. # Set the same variables as FindPythonInterp and FindPythonLibs.
set(PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE}" PARENT_SCOPE) set(PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE}" CACHE PATH "")
set(PYTHON_LIBRARY "${PYTHON_LIBRARY}" PARENT_SCOPE) set(PYTHON_LIBRARY "${PYTHON_LIBRARY}" CACHE PATH "")
set(PYTHON_DLL "${PYTHON_DLL}" PARENT_SCOPE) set(PYTHON_DLL "${PYTHON_DLL}" CACHE PATH "")
set(PYTHON_INCLUDE_DIR "${PYTHON_INCLUDE_DIR}" PARENT_SCOPE) set(PYTHON_INCLUDE_DIR "${PYTHON_INCLUDE_DIR}" CACHE PATH "")
set(PYTHONLIBS_VERSION_STRING "${PYTHONLIBS_VERSION_STRING}" PARENT_SCOPE) set(PYTHONLIBS_VERSION_STRING "${PYTHONLIBS_VERSION_STRING}" PARENT_SCOPE)
set(PYTHON_VERSION_STRING "${PYTHON_VERSION_STRING}" PARENT_SCOPE) set(PYTHON_VERSION_STRING "${PYTHON_VERSION_STRING}" PARENT_SCOPE)
set(PYTHON_VERSION_MAJOR "${PYTHON_VERSION_MAJOR}" PARENT_SCOPE) set(PYTHON_VERSION_MAJOR "${PYTHON_VERSION_MAJOR}" PARENT_SCOPE)
set(PYTHON_VERSION_MINOR "${PYTHON_VERSION_MINOR}" PARENT_SCOPE) set(PYTHON_VERSION_MINOR "${PYTHON_VERSION_MINOR}" PARENT_SCOPE)
set(PYTHON_VERSION_PATCH "${PYTHON_VERSION_PATCH}" PARENT_SCOPE) set(PYTHON_VERSION_PATCH "${PYTHON_VERSION_PATCH}" PARENT_SCOPE)
message(STATUS "LLDB Found PythonExecutable: ${PYTHON_EXECUTABLE} (${PYTHON_VERSION_STRING})") message(STATUS "LLDB Found PythonExecutable: ${PYTHON_EXECUTABLE} (${PYTHON_VERSION_STRING})")
message(STATUS "LLDB Found PythonLibs: ${PYTHON_LIBRARY} (${PYTHONLIBS_VERSION_STRING})") message(STATUS "LLDB Found PythonLibs: ${PYTHON_LIBRARY} (${PYTHONLIBS_VERSION_STRING})")
▲ Show 20 Lines • Show All 234 Lines • Show Last 20 Lines