Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

Hoikas
Copy link
Contributor

@Hoikas Hoikas commented Dec 31, 2020

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

@Hoikas Hoikas marked this pull request as ready for review January 1, 2021 01:08
@PhoebeHui PhoebeHui added the category:port-bug The issue is with a library, which is something the port should already support label Jan 4, 2021
@JackBoosY JackBoosY self-assigned this Jan 4, 2021
Copy link
Contributor

@PhoebeHui PhoebeHui left a 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!

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Jan 4, 2021
@JackBoosY
Copy link
Contributor

I think we need to add vcpkg-cmake-wrapper to python to let all python-dependent ports use the default way to find python in vcpkg, instead of having to manually add python paths to these ports.

@Hoikas
Copy link
Contributor Author

Hoikas commented Jan 4, 2021

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 python.exe installed by the python3 port. CMake by default looks in the registry, then the path, so I'm not sure how it's finding our python executable. It shouldn't even be on the path at all in any port. The error does exhibit behavior that might occur with the python3 vcpkg-cmake-wrapper that will (hopefully) be introduced by #15221. While this solution is somewhat more verbose in terms of the cmake invocation, it eliminates a potential source for errors.

@NancyLi1013
Copy link
Contributor

@Hoikas

graphqlparser has the same problem with these ports on python.

-- Found PythonInterp: D:/installed/x64-windows/tools/python3/python.exe (found suitable version "3.9", minimum required is "2") 
CMake Error at CMakeLists.txt:29 (MESSAGE):
  Python 2 is required.

@Hoikas
Copy link
Contributor Author

Hoikas commented Jan 4, 2021

Thanks! It looks like upstream used to have special handling for building the vcpkg port, but it was removed. Edit: no, it's a goofy patch.

All those -DVCPKG_DOWNLOAD_[whatever] did not seem to actually be
present in the upstream code. So, they became `vcpkg_add_to_path()`.
@Hoikas
Copy link
Contributor Author

Hoikas commented Jan 4, 2021

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 graphqlparser on CI or your local machine?

@NancyLi1013
Copy link
Contributor

@Hoikas

I can reproduce the failures with graphqlparser on my local if I installed python3 port.

Copy link
Member

@vicroms vicroms left a 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}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@Hoikas
Copy link
Contributor Author

Hoikas commented Jan 5, 2021

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.

@Hoikas
Copy link
Contributor Author

Hoikas commented Jan 5, 2021

I am closing this in favor of fixing the issue with the python3 port's vcpkg-cmake-wrapper, as suggested by @JackBoosY, in #15221.

@Hoikas Hoikas closed this Jan 5, 2021
@luncliff luncliff mentioned this pull request Feb 21, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants