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

get_python_install_dir issues #597

Closed
mikepurvis opened this issue Feb 6, 2020 · 0 comments · Fixed by #601
Closed

get_python_install_dir issues #597

mikepurvis opened this issue Feb 6, 2020 · 0 comments · Fixed by #601

Comments

@mikepurvis
Copy link
Member

This was noticed by @tspicer01 as part of Clearpath's Python 3 migration effort. The crux of the issue is that catkin's version of this functionality (implemented in CMake) is based on checking ${PYTHON_VERSION_MAJOR} as populated by the PythonInterp CMake module, which I believe actually calls PYTHON_EXECUTABLE to check what it is.

In contrast, the version in catkin_tools is based on checking sys.version_info, that is, it assumes that the Python that's invoking catkin_tools will be the same one as PYTHON_EXECUTABLE. This is definitely not a valid assumption, particularly after catkin_tools 0.5.0 is released (#594 ) and we may have users running Python 3 catkin_tools to build Python 2 ROS Melodic workspaces.

The ideal would be to replace this logic with just shelling out to a cmake -P invocation that just includes and calls the function from catkin. The difficulty with that is that we don't know for certain that catkin is in the workspace or even in an underlay— catkin_tools needs this functionality even in the scenario where it's just building a bunch of CMake packages with no catkin in sight. So it seems like the right answer then is to extract PYTHON_EXECUTABLE (if present) from the context.cmake_args, and also invoke it ourselves to determine what version it is (falling back to env python), and otherwise continuing to duplicate the logic from catkin.

And because shelling out to a new python process is rather more expensive than just looking up sys.version_info, the result after the change should be returned from a cache after the first call.

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 a pull request may close this issue.

1 participant