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

When finding Python3, use python3 executable as a hint #507

Merged
merged 4 commits into from Feb 16, 2024

Conversation

cottsay
Copy link
Contributor

@cottsay cottsay commented Feb 15, 2024

The existing find_package(Python3) call will locate the Python interpreter with the latest version number. However, this may not be the system's default Python interpreter for which our dependencies have been installed.

If Python3_EXECUTABLE is not explicitly specified, use find_program to locate the Python interpreter behind the "python3" command, which is likely the system's default and the one that we want.

If no executable can be found by the name "python3", the find_program call will silently fail and the existing behavior will manifest.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status
  • Windows-debug Build Status <- did I trigger this wrong? Is it even expected to work?

The existing find_package(Python3) call will locate the Python
interpreter with the latest version number. However, this may not be the
system's default Python interpreter for which our dependencies have been
installed.

If Python3_EXECUTABLE is not explicitly specified, use find_program to
locate the Python interpreter behind the "python3" command, which is
likely the system's default and the one that we want.

If no executable can be found by the name "python3", the find_program
call will silently fail and the existing behavior will manifest.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay added the bug Something isn't working label Feb 15, 2024
@cottsay cottsay self-assigned this Feb 15, 2024
@traversaro
Copy link
Contributor

I do not know how to fix this, but note that this may be problematic on Windows as on Windows official Python binaries and in conda python packages there is no python3 executable. That would be ok if there was no python3 executable in the system, but unfortunately that is not the case as there is a python3 executable installed by Windows itself that is just a shortcut that try to open the Windows store and install python, see python/cpython#99185 .

I do not know if there is a smarter solution, but perhaps it could make sense to use this workaround only on platforms different from Windows?

@cottsay
Copy link
Contributor Author

cottsay commented Feb 15, 2024

Thanks, @traversaro. It was pointed out in discussion that Windows used "python" and not "python3", but I hadn't thought about the python installer thing. We could make the find_program logic non-Windows-only, I'd suppose.

Either way, I'll do some poking around tomorrow and see how CMake acts in the presence of the "python3" installer executable.

@traversaro
Copy link
Contributor

Just to understand, did you considered as an alternative to set(Python3_FIND_STRATEGY LOCATION) ? Note that this should be the default once one defines cmake_minimum_required(VERSION 3.15) or cmake_minimum_required(VERSION <min>...3.15) or higher thanks to policy CMP0094, but clearly that is something that should be done in all projects and so is out of scope here.

@clalancette
Copy link
Contributor

Just to understand, did you considered as an alternative to set(Python3_FIND_STRATEGY LOCATION) ? Note that this should be the default once one defines cmake_minimum_required(VERSION 3.15) or cmake_minimum_required(VERSION <min>...3.15) or higher thanks to policy CMP0094, but clearly that is something that should be done in all projects and so is out of scope here.

I'm about to open a set of PRs to rolling that does exactly this.

Unfortunately, there are 2 problems with it. The first is that Python3_FIND_STRATEGY LOCATION doesn't work exactly how we want it to. In particular, it will stop after finding the first version of the Python executable it runs across, but it prefers to find a specific version (/usr/bin/python3.11) over the more generic version (/usr/bin/python3). This means that it won't actually fix the problem we are trying to fix here. Luckily, there is Python3_FIND_UNVERSIONED_NAMES, which you can set to FIRST to have it find /usr/bin/python3 first.

But the second problem comes from that second hint. That hint is only available in CMake 3.20 plus, and humble needs to target earlier versions (including 3.16.3 on Ubuntu 20.04).

So I think that going forward in rolling, we should use CMP0094, but for fixing the problem here in humble, we should go with this solution (obviously taking into account your previous comments about Windows).

@traversaro
Copy link
Contributor

Thanks, crystal clear!

@clalancette
Copy link
Contributor

I do not know how to fix this, but note that this may be problematic on Windows as on Windows official Python binaries and in conda python packages there is no python3 executable. That would be ok if there was no python3 executable in the system, but unfortunately that is not the case as there is a python3 executable installed by Windows itself that is just a shortcut that try to open the Windows store and install python, see python/cpython#99185 .

I do not know if there is a smarter solution, but perhaps it could make sense to use this workaround only on platforms different from Windows?

So I did some testing around this.

First of all, if I open a command prompt and type python3, I do indeed get the Windows pop-up, trying to install Python from the Windows Store.

Second, if I build Humble from current sources, I don't get any pop-up. This isn't surprising, but is the baseline.

Third, if I put this PR and ros2/geometry2#650 , I can also build Humble with no problems, and with no pop-ups.

So it seems like the pop-up doesn't happen when it is invoked via colcon/ament/cmake. If I print out the result of Python3_EXECUTABLE immediately after the find_program, it is Python3_EXECUTABLE-NOTFOUND, so maybe the CMake logic doesn't actually look in the location where the python3 stub is located.

Given that, this leaves us with 3 possibilities:

  1. Take this PR as-is, since find_program(python3) doesn't seem to be a problem on Windows.
  2. Make a modification to this PR where we search for find_program(python) on Windows, and find_program(python3) on Linux. This has the benefit of making sure we try to find python.exe on Windows, and also makes sure that Linux and Windows act similar.
  3. Only attempt to find_program(python3) on Linux. This has the downside in that it kind of causes Windows and Linux to act differently, particularly around venvs.

My suggestion is 2. Thoughts?

@cottsay
Copy link
Contributor Author

cottsay commented Feb 15, 2024

Regarding the Windows-debug CI.

  • Here's a build without this change demonstrating that the failure is not related:
    Build Status
  • Here's a build WITH this change but with tests disabled, demonstrating that this change doesn't regress Windows debug build phase:
    Build Status
  • Here's a build triggered by @clalancette with this change and another change that should address the unrelated build failure:
    Build Status

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Contributor Author

cottsay commented Feb 15, 2024

@traversaro, we did some digging regarding the Windows Store python executables.

I can hardly believe it, but It looks like there is explicit logic in CMake to ignore those Python executables during a call to find_program. Even so, the behavior of find_program(Python3) is different on Windows and specifically versioned Python executables don't seem to take priority like they do on Linux.

We're concluding that the find_program call provides no value on Windows since it appears to give us the behavior we want already, so to avoid any unforeseen regressions, we're just going to skip it and keep the existing logic as-is for Windows specifically.

Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay merged commit a967649 into humble Feb 16, 2024
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the cottsay/find_python3 branch February 16, 2024 15:50
@Ryanf55
Copy link
Contributor

Ryanf55 commented Mar 3, 2024

Should I take the part from #515 and dump it in the non-windows condition here on humble?

@cottsay
Copy link
Contributor Author

cottsay commented Mar 3, 2024

The call to find_package(Python3) has always been part of Humble (introduced in #355) and happens on Windows and non-Windows alike. This PR added a call to find_program.

That said, updating the minimum version is probably still a candidate for backport IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants