Skip to content

Conversation

jbsolomon
Copy link

@jbsolomon jbsolomon commented Aug 14, 2025

This allows the user to run Diligent python steps in a venv, such as conda.

Tested on Ubuntu 25, CMake version 3.31.6.

This feature is only well-supported in CMake as of version 3.17, so it is gated by a version test.

It might make sense to put this in the root-level CMake, but this is the minimum change to make the Samples repo build cleanly on my system.

Comment on lines 27 to 29
if (${CMAKE_VERSION} VERSION_EQUAL "3.17.0" OR
${CMAKE_VERSION} VERSION_GREATER "3.17.0")
message(STATUS "Trying to use Python3 from virtual env")
set(Python_FIND_VIRTUALENV FIRST)
endif()
Copy link
Contributor

@TheMostDiligent TheMostDiligent Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments:

  • there is VERSION_GREATER_EQUAL comparison operator
  • I don't think there is a need to print the message as in practice it will always be printed out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a matter of fact, this change should not really be needed as FIRST is the default behavior. Wondering why it makes difference at all:

image

https://cmake.org/cmake/help/latest/module/FindPython.html

@TheMostDiligent
Copy link
Contributor

Can you please also add comments explaining:

  • Why this is needed, probably referencing the issue
  • That while Python_FIND_VIRTUALENV is available since 3.15, Conda is only supported starting with 3.17
  • Despite the fact that according to CMake docs Python_FIND_VIRTUALENV FIRST is the default behavior, setting this here still makes a difference

@jbsolomon jbsolomon force-pushed the find-python-venv-first branch from 302658c to d429b5a Compare August 23, 2025 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants