updated PYTHON_INSTALL_DIR generation #831
Conversation
There was a problem hiding this comment.
I could imagine a different way of handling this:
- have a cmake file that finds the
PYTHON_EXECUTABLEand calculates thePYTHON_INSTALL_DIR - execute this cmake file in a temporary directory with the build tool, extract the
PYTHON_EXECUTABLEandPYTHON_INSTALL_DIRas a tuple - pass this "precomputed" tuple to each catkin package over the command line
- in the
cmake/python.cmakefile:- find the
PYTHON_EXECUTABLE - check for a
PYTHON_INSTALL_DIRfor that exec in the "precomputed" tuples passed on the command line - otherwise have the package calculate
PYTHON_INSTALL_DIRusing the file described above in the first bullet
- find the
This way you can avoid the costly call to python in each package by precomputing with the build tool, but without the risk of giving an incorrect PYTHON_INSTALL_DIR due to tool and cmake using different python executables. Also the packages retain the ability to function without the tool. This also avoids having the logic to calculate this code being duplicated in both cmake and python (regardless of how simple it is).
cmake/python.cmake
Outdated
| OUTPUT_VARIABLE _var | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE) | ||
| if(NOT _res EQUAL 0) | ||
| message(FATAL_ERROR "Determine PYTHON_INSTALL_DIR failed") |
There was a problem hiding this comment.
"Failed to determine the PYTHON_INSTALL_DIR" or "Determining the PYTHON_INSTALL_DIR failed"?
cmake/python.cmake
Outdated
| # setuptools is fussy about windows paths, make sure the install prefix is in native format | ||
| file(TO_NATIVE_PATH "${CMAKE_INSTALL_PREFIX}" SETUPTOOLS_INSTALL_PREFIX) | ||
| set(PYTHON_INSTALL_DIR "${_var}" | ||
| CACHE INTERNAL "This needs to be in PYTHONPATH when 'setup.py install' is called. And it needs to match. But setuptools won't tell us where it will install things.") |
There was a problem hiding this comment.
It needs to match what? And what purpose does the stuff about setuptools serve. I don't understand these parts of the variable description. I think it either needs to be more specific or maybe just leave this stuff out.
There was a problem hiding this comment.
The message was just moved around in the patch but I shortened it to avoid any confusion.
| set(_PYTHON_PATH_VERSION_SUFFIX "${PYTHON_VERSION_MAJOR}") | ||
| # this block determines the same value as the Python function get_python_install_dir() from python/catkin/builder.py | ||
| if(NOT DEFINED PYTHON_INSTALL_DIR) | ||
| execute_process(COMMAND "${PYTHON_EXECUTABLE}" |
There was a problem hiding this comment.
I have a feeling (no evidence) that this will have a pretty large impact on build times in workspaces with lots of packages. Can we change catkin_make and catkin_make_isolated to calculate this and pass it to cmake to avoid this extra call to Python, or is that already happening?
There was a problem hiding this comment.
catkin_make will only process this once since all packages share the same context.
catkin_make_isolated (as well as catkin build) will have the cost of an extra Python invocation. The tool can decide to pass in that value (which will put it in the cache) to avoid the computation. Anyway I don't plan to add that to cmi. The tool would then also need to make sure that it sets a patching path if its using a different Python version then detected in CMake (related to your other comment https://github.com/ros/catkin/pull/831/files#r84345010).
There was a problem hiding this comment.
I think it would be easier for a tool to support this if the checking to make sure the python executables match is done in this cmake code, i.e. passing PYTHON_EXECUTABLE:PYTHON_INSTALL_DIR rather than just PYTHON_INSTALL_DIR from the tool. It is very difficult for the tool to determine if it has the same PYTHON_EXECUTABLE as this cmake code and since this CMake logic cannot be run separately (at least in its current configuration) the tool cannot do anything to resolve the mismatch if/when it was detected.
My suggestion would be to:
- separate the logic that calculates the
PYTHON_EXECUTABLEand thePYTHON_INSTALL_DIRinto a separate CMake file so it can be reused- this would allow the tool to make sure it is using the same
PYTHON_EXECUTABLEand thereforePYTHON_INSTALL_DIRas the package will be using in most cases- this allows us to avoid the extra python invocation, without this I think it leaves a clear con for this pr (increased build time)
- prevent the issue where the
PYTHONPATHand installed subdirectory disagree- though I agree that this not the responsibility of this pr to solve, it is a nice benefit
- this would allow the tool to make sure it is using the same
- add some code here to handle the
PYTHON_EXECUTABLE:PYTHON_INSTALL_DIRtuple input- protects the individual package from any unforeseen mismatches when the tool is attempting to precompute the
PYTHON_INSTALL_DIRfor the package- again this wouldn't be necessary if this pr didn't introduce an additional call to python, so I see it as in scope to provide a mechanism to mitigate that drawback
- protects the individual package from any unforeseen mismatches when the tool is attempting to precompute the
I can help implement the changes, but I think they're pretty important to consider and possibly get in before merging the other changes (which introduce the extra Python call).
There was a problem hiding this comment.
If you could write a PR for this enhancement that would be great.
There was a problem hiding this comment.
I think that this pr covers the changes: #832
I didn't find a need to refactor the python.cmake file after all. I can just call it with cmake in script mode and extract the information from the existing I/O. It is a bit fragile in that a change in the structure of that output could break the Python code, but it is covered in tests, so the tests would start failing in that case.
There was a problem hiding this comment.
In order to ensure that the tool and CMake "agree" on the same location the patch still needs to pass the command line argument for -DPYTHON_EXECUTABLE to the Python functions, right?
There was a problem hiding this comment.
Yes. The above pr fixes one case, i.e. where the tool is using one version of Python but cmake finds another and the users is not using any cmake arguments to affect the process. It does not address any case where the user affects the version that cmake finds using variables like PYTHON_VERSION or PYTHON_EXECUTABLE. To cover those, the build tool needs to pass any extra cmake arguments to the new Python functions (which is the purpose of the cmake_args parameters that were added to those functions).
There was a problem hiding this comment.
The reason I didn't take the last step was that I didn't know if naively passing all the user provided cmake arguments to this function would work or if they needed to be intelligently pruned for just options that affect Python version selection. Also it seemed like a good stopping point for the afternoon (so I can work on some other stuff). I can try to pass along the user defined cmake args to these functions and see if that works fine, but it will probably be next week.
| @@ -287,17 +288,8 @@ def get_multiarch(): | |||
|
|
|||
| def get_python_install_dir(): | |||
| # this function returns the same value as the CMake variable PYTHON_INSTALL_DIR from catkin/cmake/python.cmake | |||
There was a problem hiding this comment.
What if the tool is invoked with a different python executable than the one found in PYTHON_EXECUTABLE in cmake?
I can imagine, that if the tool is using Python3 and then the CMake code find_package(PythonInterp) finds Python2, the code will be installed to something like <prefix>/lib/python2.7/site-packages but the path <prefix>/lib/python3.5/site-packages would be added to the PYTHONPATH using the setup file which is generated elsewhere in this file.
Or maybe this function is really only about getting site-packages versus dist-packages?
There was a problem hiding this comment.
The Python version of the tool calling cmake is not relevant for this. CMake will make a choice and use it throughout the process. So whatever is chosen in CMake (which can be influenced by passing PYTHON_VERSION) decides the install path for Python code.
There was a problem hiding this comment.
So the python code here is actually called by CMake and not the build tool?
There was a problem hiding this comment.
This function is used here:
https://github.com/ros/catkin/blob/kinetic-devel/python/catkin/builder.py#L602
And that is used to generate a setup file here:
https://github.com/ros/catkin/blob/kinetic-devel/python/catkin/builder.py#L641
So maybe it only affects catkin_make_isolated in non-merged spaces, but I still think it could result in the PYTHONPATH pointing to one place and the code getting installed to another.
There was a problem hiding this comment.
The build tool is also using get_python_install_dir in order to do additional stuff, e.g. generating additional environment variables. So the constraint that the Python versions needs to match is still the case. But I don't think that this is affected in any way by this patch. It was the case before as well as after this change.
There was a problem hiding this comment.
That's true, but it was related to potentially avoiding every package calling python itself by having the tool compute it once and pass it on the command line.
dirk-thomas
force-pushed
the
replace_636
branch
from
ad090f1
to
14376cb
Compare
|
Hi sorry for the very, late reaction. I tried this on Slackware 14.2 but it still does not seem to be working - on 64 bit OS packages are installed in both lib and lib64 - as mentioned in ros-infrastructure/catkin_pkg#153. Just wanted to make sure I am doing it correctly - I substituted my |
|
I will set the milestone to "untargeted" for now until @wjwwood will have a chance to continue implementing the approach he described above. |
|
@nikonikolov I think that you could instead test the contents of #832 to see if it would work. I'm not sure what I'm doing there would be necessary for your fix or not. |
|
Hey, what's the status of this issue/is it going to be resolved? |
|
I have not had time to implement the fix I described. I don't think I'll have time to do it in the near future. |
|
See: #831 (comment) and the following comment for what needs to be done still. |
|
Closing due to inactivity. |
Replaces #636.
@ros/ros_team Since this significantly affects the core I would like to have some feedback. Not only but especially on the following thought:
SETUPTOOLS_ARG_EXTRA.