Skip to content

CMake: Prevent pollution of user system - #252

Closed
jbsolomon wants to merge 2 commits into
DiligentGraphics:masterfrom
phoenix-engine:find-python-venv-first
Closed

CMake: Prevent pollution of user system#252
jbsolomon wants to merge 2 commits into
DiligentGraphics:masterfrom
phoenix-engine:find-python-venv-first

Conversation

@jbsolomon

@jbsolomon jbsolomon commented Aug 14, 2025

Copy link
Copy Markdown

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 thread RenderStateNotation/CMakeLists.txt Outdated
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()

@TheMostDiligent TheMostDiligent Aug 15, 2025

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
@jbsolomon

Copy link
Copy Markdown
Author

I actually didn't realize this was the default behavior when I opened this PR. I think this is not needed

@jbsolomon jbsolomon closed this Sep 22, 2025
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