-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[many ports] Disambiguate Python usage. #15404
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the PR!
I think we need to add |
As far as I can tell, Python is being found in the default way in all of these ports. I don't understand why CI in #15298 is using the |
|
Thanks! |
All those -DVCPKG_DOWNLOAD_[whatever] did not seem to actually be present in the upstream code. So, they became `vcpkg_add_to_path()`.
81c2319
to
7b6adcf
Compare
Re-reading @JackBoosY's comment, I wonder if this is a problem with the PR CI environment? The last CI run in #15353 didn't seem to have these problems. @NancyLi1013 Did you see this failure with |
I can reproduce the failures with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -31,6 +29,7 @@ vcpkg_configure_cmake( | |||
-DBUILD_EXAMPLES=OFF | |||
-DWITH_TEST=OFF | |||
-DIMGUI_EXTERNAL_PATH=${CURRENT_INSTALLED_DIR}/include/bindings | |||
-DPython3_EXECUTABLE=${PYTHON3} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be Python_EXECUTABLE? and other 4 places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on the upstream CMakeLists. This port uses find_package(Python3)
, so we have to use Python3_EXECUTABLE
. If it used find_package(Python)
, then we would use Python_EXECUTABLE
. In the case of find_package(PythonInterp)
, then we want PYTHON_EXECUTABLE
.
Generally, ports tend to use Python3
the most, followed by PythonInterp
, and Python
least frequently.
It looks like the usage of the python3 port's Python is the result of #14317, specifically this portion - it looks like this may have been bugged before. It seems to me like this part of the toolchain is in error because the tools in that directory are built for the target and therefore may not be usable on the host. That is why x64-windows-static checks are failing. The rest of the failures are, as mentioned in #15457 due to Python 3's python.exe hiding Python 2's python.exe. |
I am closing this in favor of fixing the issue with the python3 port's vcpkg-cmake-wrapper, as suggested by @JackBoosY, in #15221. |
In #15298 there are CI failures that appear to result from ports using the wrong Python interpreter, potentially the one built by #14891. This fixes all cmake based ports to correctly specify the path to the python executable instead of relying on magical fairy dust for finding the acquired python in the path.
What does your PR fix?
See above
Which triplets are supported/not supported? Have you updated the CI baseline?
No change
Does your PR follow the maintainer guide?
Yes