Change include_guard to check target only

[afestini]

include_guard seems to break all contained logic to handle use case of building for multiple Python versions.

Description

Use case: trying to create multiple targets for different Python versions

Issue: some variables are not updated on subsequent targets, most noticeably the SUFFIX, resulting in (as example) all .pyd files being named .cp39-win_amd64.pyd (despite linking against 3.9, 3.10, etc.)

There seems to be a lot of logic to handle this case on subsequently entering this file, however a fix (likely related to changed in Conan) added an include guard to prevent errors about creating the same target twice. This also sabotages all of that above logic, since it will not execute another time and nothing gets updated after switching Python versions.

Removing the include_guard and instead only create the target if it doesn't exist already seems to fix that. However, someone with deeper understanding of the dependencies and flows should verify that this won't have unexpected and fatal side effects.

[rwgk]

Hi again @eacousineau, this is a one-line change that LGTM, but I don't know much about cmake. Could you please help?

Or anyone else familiar with cmake seeing this: could you please approve or comment?

[EricCousineau-TRI]

Hm... I'm a bit rusty at CMake, esp. more complex usages like processing against multiple versions. (FWIW, I'm using Bazel / Blaze, but typically the same host-target configuration, so even my usage there is a bit rudimentary.)

@afestini Is there an easily reproducible example of your workflow (single CMake configuration run against multiple interpreter versions)? If possible, having a reproduction case would help ground my understanding towards a better review, esp. given a mechanism I'm not too familiar with.

If too much effort, I'm OK with rubber-stamping.
I've reviewed include_guard docs, and it smells weird, but par the course for stateful / oddly encapsulated CMake logic.

[afestini]

I tried to strip it down to the core of what is happening. Basically, given a list of Python versions, each function call should produce a matching project. It seems that in terms of linking, each resulting binary is linked against the proper Python version (though interestingly, each project is adding the include and lib entries for ALL versions, luckily the one we want is always first, but that is an issue I still have to debug).

The obvious effect of the include guard is that a lot of the variables don't get updated, so for example, each binary would have the same suffix as the first one (like -cp39-win-amd64.pyd).

function(BuildBinder py_version)
  set (PYTHON_VERSION ${py_version})
  set (CURRENT_TARGET binder-${Python_VERSION})

  project(${CURRENT_TARGET} CXX)

  find_package(Python ${PYTHON_VERSION} EXACT COMPONENTS Interpreter Development)

  if(${Python_FOUND})
    find_package(pybind11)
    pybind11_add_module(${CURRENT_TARGET} ${SOURCES})

    target_compile_definitions(${CURRENT_TARGET} PUBLIC PYBIND11_PYTHON_VERSION=${Python_VERSION})
    target_include_directories(${CURRENT_TARGET} ${pybind11_INCLUDE_DIRS})
    target_link_libraries(${CURRENT_TARGET} PUBLIC ${Python_LIBRARIES} PUBLIC pybind11::headers pybind11::lto)
  endif()
endfunction()

foreach(python_version ${CMAKE_PYTHON_VERSIONS})
  BuildBinder(${python_version})
endforeach()

[henryiii]

Was this fixed by #4401 ?

[afestini]

Not quite, since in this scenario, a loop in a single CMakefile is iterating over multiple Python versions. It would still require some hacky workaround like duplicating the file into multiple directories (and likely lose the ability to configure which versions to build by passing it as argument).