Fix #8778 - FindPython in modern cmake and handle issue when LINK_WITH_PYTHON on windows with both Release/debug libraries #8832

jmarrec · 2021-06-18T09:19:45Z

Pull request overview

(This is the third version of this branch).

Fix LINK_WITH_PYTHON issue on windows #8778 -handle issue when LINK_WITH_PYTHON on windows with both Release/debug libraries
Use the modern cmake FindPython instead of the deprecated FindPythonInterp+FindPythonLibs
- Previously the deprecated FindPythonInterp and FindPythonLibs where called at different points (toplevel CMakeLists.txt and src/EnergyPlus/CMakeLists.txt respectively), which in my case often resulted in different versions being picked up (I have a 3.6 system python on Ubuntu, and 4 different pyenv versions and several virtualenvs on top of these).
- FindPythonInterp/FindPythonLibs were deprecated in favor of FindPython in CMake 3.12: https://cmake.org/cmake/help/latest/module/FindPythonInterp.html

This requires bumping the cmake_minimum_version to at least 3.12. Because of we're bumping, we're actually bumping to 3.16 here which adds PCH (precompiled headers) which is an option enabled by default now

EnergyPlus/CMakeLists.txt

Line 30 in 73fc0b1

option(ENABLE_PCH "Enable Precompiled Headers" ON)

target_precompile_headers was added in version 3.16: https://cmake.org/cmake/help/git-stage/command/target_precompile_headers.html

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

optimized is always first, pick that one up https://gitlab.kitware.com/cmake/cmake/-/blob/d98a7cdb25611ed6f1e856fd4c4ff980225b89cd/Modules/FindPython/Support.cmake#L63-91

@Myoldmopar

A conversation with @Myoldmopar lead to this. I'm already bumping to 3.12, but 3.16 added Precompiled Headers (PCH) which is a significant speedup. 3.13 added target_link_options which is also handy.

jmarrec · 2021-06-18T09:20:29Z

CMakeLists.txt

@@ -9,7 +9,7 @@ cmake_policy(SET CMP0048 NEW) # handling project_version_* variables

 project(EnergyPlus)

-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.16)


New Minimum version here to 3.16 for precompiled header support.

jmarrec · 2021-06-18T09:20:54Z

CMakeLists.txt

+# If LINK_WITH_PYTHON, also request the Development (libs) at the same time, to ensure consistent version between interpreter and Development
+if(LINK_WITH_PYTHON)
+  find_package(Python 3.6 COMPONENTS Interpreter Development REQUIRED)
+else()
+  find_package(Python 3.6 COMPONENTS Interpreter REQUIRED)
+endif()


New way, find both components at the same time to ensure consistent versions

jmarrec · 2021-06-18T09:21:36Z

CMakeLists.txt

@@ -270,7 +275,7 @@ endif()
 add_subdirectory(idd)

 execute_process(
-  COMMAND ${PYTHON_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/dev/generate_epJSON_schema/generate_epJSON_schema.py" "${PROJECT_SOURCE_DIR}"
+  COMMAND ${Python_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/dev/generate_epJSON_schema/generate_epJSON_schema.py" "${PROJECT_SOURCE_DIR}"


PYTHON_XXXX variables were retained for backward compatibility, but the official casing is Programname_XXX so Python_XXX to be consitent with the rest of cmake.

OK, makes sense. I'm pretty sure if anyone ever adds another call from Python they'll end up just copying whatever we have in place anyway.

jmarrec · 2021-06-18T09:22:19Z

doc/CMakeLists.txt

@@ -1,12 +1,12 @@
 project(EnergyPlusDocs LANGUAGES NONE)

-cmake_minimum_required(VERSION 3.5.1)
+cmake_minimum_required(VERSION 3.12)


In docs, we only need to bump to 3.12 for FindPython because PCH isn't in the mix... So I did 3.12 instead of 3.16, up for debate.

Hmmm...I'm not sure. I guess it's best to keep the minimum required to the minimum required for that specific project scope. So 3.12 makes sense for the new FindPython.

jmarrec · 2021-06-18T09:23:09Z

src/EnergyPlus/CMakeLists.txt

+    # In case you have both release and debug Python libraries on your system, Python_LIBRARIES might be "optimized;C:/.../python38.lib;debug;C:/.../python38_d.lib"
+    # so it can't be used directly, we could use a generator expression to find the $<TARGET_FILE:Python::Python> library used and that'd point directly
+    # to the DLL. But it won't work nicely for the install(code ...) command below... so we do a hack, get the optimized one always...
+    list(LENGTH Python_LIBRARIES _LEN)
+    if (_LEN GREATER 1)
+      set (_Python_DOING_RELEASE FALSE)
+      foreach(_currentArg ${Python_LIBRARIES})
+        message("_currentArg=${_currentArg}")
+        if ("x${_currentArg}" STREQUAL "xoptimized")
+          set(_Python_DOING_RELEASE TRUE)
+        elseif(_Python_DOING_RELEASE)
+          get_filename_component(RESOLVED_PYTHON_LIBRARY "${_currentArg}" REALPATH)
+          break()
+        endif()
+      endforeach()
+    # else()
+      # No-op, already done above
+    endif()


Actual fix for #8778 when you have both Release and Debug python libs on your system.

Myoldmopar · 2021-06-22T18:20:15Z

Well would you look at that!? Happy on all 3 platforms! This is good stuff.

Myoldmopar

I feel confident with all of this but I do want some clarification on the loop for finding the right version of Python. If we can get this resolved I believe this is good to go.

Myoldmopar · 2021-06-23T15:20:14Z

CMakeLists.txt

@@ -9,7 +9,7 @@ cmake_policy(SET CMP0048 NEW) # handling project_version_* variables

 project(EnergyPlus)

-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.17)


Great! This makes the whole PCH thing feel much better too.

Myoldmopar · 2021-06-23T15:20:26Z

CMakeLists.txt

+# If LINK_WITH_PYTHON, also request the Development (libs) at the same time, to ensure consistent version between interpreter and Development
+if(LINK_WITH_PYTHON)
+  find_package(Python 3.6 COMPONENTS Interpreter Development REQUIRED)
+else()
+  find_package(Python 3.6 COMPONENTS Interpreter REQUIRED)
+endif()


Myoldmopar · 2021-06-23T15:21:18Z

CMakeLists.txt

@@ -270,7 +275,7 @@ endif()
 add_subdirectory(idd)

 execute_process(
-  COMMAND ${PYTHON_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/dev/generate_epJSON_schema/generate_epJSON_schema.py" "${PROJECT_SOURCE_DIR}"
+  COMMAND ${Python_EXECUTABLE} "${PROJECT_SOURCE_DIR}/scripts/dev/generate_epJSON_schema/generate_epJSON_schema.py" "${PROJECT_SOURCE_DIR}"


OK, makes sense. I'm pretty sure if anyone ever adds another call from Python they'll end up just copying whatever we have in place anyway.

Myoldmopar · 2021-06-23T15:23:52Z

doc/CMakeLists.txt

@@ -1,12 +1,12 @@
 project(EnergyPlusDocs LANGUAGES NONE)

-cmake_minimum_required(VERSION 3.5.1)
+cmake_minimum_required(VERSION 3.12)


Hmmm...I'm not sure. I guess it's best to keep the minimum required to the minimum required for that specific project scope. So 3.12 makes sense for the new FindPython.

Myoldmopar · 2021-06-23T15:26:16Z

src/EnergyPlus/CMakeLists.txt

+        elseif(_Python_DOING_RELEASE)
+          get_filename_component(RESOLVED_PYTHON_LIBRARY "${_currentArg}" REALPATH)
+          break()
+        endif()


Is there a code path here missing in case you don't find the optimized keyword? I know it would be random but just trying to make it as robust as possible. Maybe if the finder finds multiple debug build versions on one system?? Would it be debug;C:/Something.lib;debug;C:/SomethingElse.lib?

@Myoldmopar AFAIK you cannot install only debug libraries, and optimized are always installed. Either you have both and it's guaranteed you'll have optimized, or you have only one item. See source code here: https://gitlab.kitware.com/cmake/cmake/blob/fc762735196dbbc983dfac2145cd7afd7892bf54/Modules/FindPython/Support.cmake#L63-91

Myoldmopar · 2021-07-01T02:57:48Z

No point waiting on this to go in. This is a great set of changes. Much more reliable finding! Thanks @jmarrec !

jmarrec added 4 commits June 18, 2021 11:13

FindPython in modern cmake, v3

7250ae2

Hack in case you have both release and debug libs on windows

d57036c

optimized is always first, pick that one up https://gitlab.kitware.com/cmake/cmake/-/blob/d98a7cdb25611ed6f1e856fd4c4ff980225b89cd/Modules/FindPython/Support.cmake#L63-91

Be a bit more reliable when locating the optimized lib

9b5a6b6

Bump Cmake Minimum version to 3.16

73fc0b1

A conversation with @Myoldmopar lead to this. I'm already bumping to 3.12, but 3.16 added Precompiled Headers (PCH) which is a significant speedup. 3.13 added target_link_options which is also handy.

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Jun 18, 2021

jmarrec requested a review from Myoldmopar June 18, 2021 09:19

jmarrec self-assigned this Jun 18, 2021

Remove debug message

72b36cf

jmarrec commented Jun 18, 2021

View reviewed changes

jmarrec added 3 commits June 22, 2021 17:32

Merge remote-tracking branch 'origin/develop' into FindPython

cbed5b5

Bump to 3.17 per @Myoldmopar

29ec703

Deleted the wrong line in 72b36cf

1656f7b

Myoldmopar reviewed Jun 23, 2021

View reviewed changes

Myoldmopar merged commit 4708fe7 into develop Jul 1, 2021

Myoldmopar deleted the FindPython branch July 1, 2021 02:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #8778 - FindPython in modern cmake and handle issue when LINK_WITH_PYTHON on windows with both Release/debug libraries #8832

Fix #8778 - FindPython in modern cmake and handle issue when LINK_WITH_PYTHON on windows with both Release/debug libraries #8832

jmarrec commented Jun 18, 2021

jmarrec Jun 18, 2021

jmarrec Jun 18, 2021

Myoldmopar Jun 23, 2021

jmarrec Jun 18, 2021

Myoldmopar Jun 23, 2021

jmarrec Jun 18, 2021

Myoldmopar Jun 23, 2021

jmarrec Jun 18, 2021

Myoldmopar commented Jun 22, 2021

Myoldmopar left a comment

Myoldmopar Jun 23, 2021

Myoldmopar Jun 23, 2021

Myoldmopar Jun 23, 2021

Myoldmopar Jun 23, 2021

Myoldmopar Jun 23, 2021

jmarrec Jun 29, 2021

Myoldmopar Jul 1, 2021

Myoldmopar commented Jul 1, 2021

Fix #8778 - FindPython in modern cmake and handle issue when LINK_WITH_PYTHON on windows with both Release/debug libraries #8832

Fix #8778 - FindPython in modern cmake and handle issue when LINK_WITH_PYTHON on windows with both Release/debug libraries #8832

Conversation

jmarrec commented Jun 18, 2021

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jun 22, 2021

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jul 1, 2021