Page MenuHomePhabricator

Use the correct Python lib for each build configuration generated by the Visual Studio CMake generator
ClosedPublic

Authored by enlight on Sep 29 2015, 1:51 AM.

Details

Summary

Previously CMAKE_BUILD_TYPE was used to determine whether to link in python27.lib or python27_d.lib, unfortunately this only works reliably when using a CMake generator that generates a single build configuration (e.g. Ninja). The Visual Studio CMake generator generates four build configurations at once (Debug, Release, RelWithDebInfo, MinSizeRel), so if CMAKE_BUILD_TYPE is set to Debug all four build configurations end up linking in python27_d.lib, this is clearly undesirable.

To ensure that the correct Python lib is used for each build configuration the value of PYTHON_LIBRARY is now determined using generator expressions that evaluate to either the debug or release Python lib. The values of PYTHON_EXECUTABLE and PYTHON_DLL are now likewise determined using generator expressions.

Note that these changes only apply to the Windows build.

Diff Detail

Repository
rL LLVM

Event Timeline

enlight updated this revision to Diff 35942.Sep 29 2015, 1:51 AM
enlight retitled this revision from to Use the correct Python lib for each build configuration generated by the Visual Studio CMake generator.
enlight updated this object.
enlight added reviewers: brucem, zturner.
enlight set the repository for this revision to rL LLVM.
enlight added a subscriber: lldb-commits.
brucem edited edge metadata.Sep 29 2015, 2:02 AM

This looks good to me and like the correct fix. Leaving it for zturner to give the official nod of approval though.

zturner added inline comments.Sep 29 2015, 9:06 AM
cmake/modules/LLDBConfig.cmake
61 ↗(On Diff #35942)

This line is hard to parse mentally, and I'm not sure I've seen this kind of nested generator expression. I trust you when you say it's right, but can you explain what this does?

enlight added inline comments.Sep 29 2015, 10:47 AM
cmake/modules/LLDBConfig.cmake
61 ↗(On Diff #35942)

Sure thing.

  1. $<CONFIG:Debug> evaluates to 1 when the Debug configuration is being generated, or 0 in all other cases.
  2. $<$<CONFIG:Debug>:${PYTHON_DEBUG_EXE}> expands to ${PYTHON_DEBUG_EXE} when the Debug configuration is being generated, or nothing (literally) in all other cases.
  3. $<$<NOT:$<CONFIG:Debug>>:${PYTHON_RELEASE_EXE}> expands to ${PYTHON_RELEASE_EXE} when any configuration other than Debug is being generated, or nothing in all other cases.

Since the conditionals in 2 & 3 are mutually exclusive, and a conditional expression that evaluates to 0 yields no value at all, it's possible to concatenate them to obtain a single value. This value will be ${PYTHON_DEBUG_EXE} when generating the Debug configuration, or ${PYTHON_RELEASE_EXE} when generating any other configuration.

zturner accepted this revision.Sep 29 2015, 11:15 AM
zturner edited edge metadata.

If the above if / else is equivalent, I prefer if you change it to that before comitting. If not, feel free to commit as is.

cmake/modules/LLDBConfig.cmake
61 ↗(On Diff #35942)

Ahh, I understand. Is it equivalent to write this:

if ($<CONFIG:Debug> == 1)
  set(PYTHON_EXECUTABLE ${PYTHON_DEBUG_EXE})
else()
  set(PYTHON_EXECUTABLE ${PYTHON_RELEASE_EXE})
endif()

? If so, I kind of prefer this way, if nothing else so that other people will be able to understand it in the future if they go to edit this code.

This revision is now accepted and ready to land.Sep 29 2015, 11:15 AM
enlight planned changes to this revision.Sep 29 2015, 11:04 PM

I'll submit a revised patch that contains an explanation of the generator expressions to aid future maintainers/contributors.

cmake/modules/LLDBConfig.cmake
61 ↗(On Diff #35942)

Unfortunately that's not currently possible because the if command doesn't support generator expressions (not that the docs mention it or anything). So while CMake won't complain if you did this:

if ($<CONFIG:Debug>)
  set (PYTHON_EXECUTABLE ${PYTHON_DEBUG_EXE})
else ()
  set (PYTHON_EXECUTABLE ${PYTHON_RELEASE_EXE})
endif ()

It wouldn't actually work as intended because the if/else is only evaluated once during the configuration step rather than per-build-configuration (during the generation step).

enlight updated this revision to Diff 36072.Sep 30 2015, 12:27 AM
enlight edited edge metadata.

Added an explanation of the generator expression used in this patch.

This revision is now accepted and ready to land.Sep 30 2015, 12:27 AM

Ok, looks good

This revision was automatically updated to reflect the committed changes.

Hi Vadim, when I run CMake with this change I get the following output:

[1/1] Re-running CMake...

  • Warning: Did not find file Compiler/MSVC-ASM
  • Target triple: i686-pc-win32
  • Native target architecture is X86
  • Threads enabled.
  • Doxygen disabled.
  • Sphinx disabled.
  • Go bindings disabled.
  • Could NOT find OCaml (missing: OCAMLFIND OCAML_VERSION

OCAML_STDLIB_PATH)

  • OCaml bindings disabled.
  • Using Debug VC++ CRT: MDd
  • Found PythonInterp: C:/Python27/python.exe (found version "2.7.8")
  • Constructing LLVMBuild project information
  • LLVMHello ignored -- Loadable modules not supported on this platform.
  • Targeting AArch64
  • Targeting AMDGPU
  • Targeting ARM
  • Targeting BPF
  • Targeting CppBackend
  • Targeting Hexagon
  • Targeting Mips
  • Targeting MSP430
  • Targeting NVPTX
  • Targeting PowerPC
  • Targeting Sparc
  • Targeting SystemZ
  • Targeting X86
  • Targeting XCore
  • Clang version: 3.8.0
  • SampleAnalyzerPlugin ignored -- Loadable modules not supported on this

platform.

  • PrintFunctionNames ignored -- Loadable modules not supported on this

platform.

  • LLD version: 3.8.0
  • *Found PythonLibs:

$<$<CONFIG:Debug>:C:/Python27_LLDB/x86/libs/python27_d.lib>$<$<NOT:$<CONFIG:Debug>>:C:/Python27_LLDB/x86/libs/python27.lib>*

  • LLDB version: 3.8.0
  • Could NOT find LibXml2 (missing: LIBXML2_LIBRARIES LIBXML2_INCLUDE_DIR)
  • Could NOT find Doxygen (missing: DOXYGEN_EXECUTABLE)
  • *Found PythonInterp:

$<$<CONFIG:Debug>:C:/Python27_LLDB/x86/python_d.exe>$<$<NOT:$<CONFIG:Debug>>:C:/Python27_LLDB/x86/python.exe>*
(found version "1.4")

This doesn't seem right to me. Can you take a look and fix this?

That looks fine to me. CMake prints those messages during the configuration step, at that point the generator expressions haven't been expanded yet. I could prettify those messages a bit to read: Found PythonLibs: C:/Python27_LLDB/x86/libs/python27_d.lib and C:/Python27_LLDB/x86/libs/python27.lib.

Yea that's what I meant. Everything works, but the configuration step is
supposed to print out stuff that people can read at a glance. So
prettifying it would be desirable.

My change to support Python 3 is in. Now that that's in, would you mind
trying to prettify the output messages when you get a chance?