Fix get_python_install_dir issues

[timonegk]

This pull request attempts to close #597 by shelling out to check cmake -P, as suggested in the issue. The executed script, however, does not depend on catkin and should therefore work fine for workspaces without catkin.
While it might have been possible to replicate the behavior of cmake manually, I decided against it because this solution is easier to maintain and less error-prone, especially since the find_program cmake builtin would have been rather annoying to reimplement.

I marked this pull request as a draft because

• there is still a todo in the code that I do not really know how to implement. I think it would be right to abort the build process when conflicting cmake variables have been set (e.g. python version > version of python executable).
• no caching of the result has been implemented yet (I am not even sure if it is needed because the result of get_python_install_dir is only used to write setup.sh once).

In the second commit I fixed the install path for python3 on debian systems to reflect catkin's behavior.

[mikepurvis]

Hi @timonegk— sorry for the long delay, but thanks for taking a stab at this! I've been busy but would like to get back to trying to get this release out and am just taking a look at your work here.

Regarding the business of conflicting variables, I think if we were able to re-implement less logic here this wouldn't matter as much. That is, if we simply vendored-in (and clearly marked, in order to keep it in sync) a copy of python.cmake, and called it from a wrapper that printed out whatever PYTHON_INSTALL_DIR it lands on. Then it wouldn't be up to us to determine conflicts because even if they exist, we'd still be consistent with what was being done by catkin (it uses its version of that logic to determine where to install generated python message headers, for example).

Regarding caching, yeah it can be left off for now. It's not important in the base case of catkin/cmake packages. Internally at Clearpath, we call the function each time we build a plain Python package (see https://github.com/mikepurvis/catkin_tools_python/blob/master/catkin_tools_python/job.py#L215), but if that becomes a concern for us, we can handle adding the caching or changing that logic.

I think the litmus test for getting this in is probably to add a test that does the following:

• Clone ros/catkin (which is a plain cmake package and therefore uses this logic)
• Build the workspace
• Source the workspace and check that PYTHONPATH is pointing to the correct Python.

This should be run 4x, once each for the workspace being Python 2 or Python 3 (catkin_tools itself is now always Python 3), and once each for the develspace and installspace, per this logic here:

catkin_tools/catkin_tools/jobs/cmake.py

Lines 169 to 175 in 74e9d2b

	# Check if the setup file needs to be generated
	if context.install:
	# Create the setup file in the install space
	setup_file_needed = context.isolate_install or not os.path.exists(setup_file_path)
	else:
	# Do not replace existing setup.sh if devel space is merged
	setup_file_needed = not context.link_devel and context.isolate_devel or not os.path.exists(setup_file_path)

[timonegk]

Thank you for the feedback. I added the tests that you proposed. Regarding the PYTHON_INSTALL_DIR, you are probably right and it makes more sense not to mix cmake and python code as I did, I will update the pull request accordingly.

[timonegk]

I do not really understand why the CI is failing. It seems to be related to ros/catkin#863 and therefore seems to be a problem of the CI itself. I also cannot reproduce the problem in a bionic container. If you don't know how to fix the issue, I would suggest to remove the install tests (since the same get_python_install_dir function is used for devel and install anyway).

[mikepurvis]

So the issue with the install tests is that they're using the wrong Python— --install-layout=deb isn't recognized by the upstream Python that Travis uses for its virtualenvs; it's something that Debian patches into the version of Python that it ships in its python packages. This is why we specify using the Debian python in the test configurations:

catkin_tools/.travis.yml

Lines 5 to 24 in 5fc3a5d

	- dist: xenial
	language: python
	python: "3.5"
	os: linux
	env: PYTHON=/usr/bin/python3.5 CATKIN_VERSION=indigo-devel
	- dist: bionic
	language: python
	python: "3.6"
	os: linux
	env: PYTHON=/usr/bin/python3.6 CATKIN_VERSION=indigo-devel
	- dist: bionic
	language: python
	python: "3.7"
	os: linux
	env: PYTHON=/usr/bin/python3.7 CATKIN_VERSION=indigo-devel
	- dist: bionic
	language: python
	python: "3.8"
	os: linux
	env: PYTHON=/usr/bin/python3.8 CATKIN_VERSION=indigo-devel

Whereas your error is showing that it's using /home/travis/virtualenv/python3.5.6/bin/python3 instead.

Also, I'm noticing that there aren't actually any other tests that clone in external content— I thought there were, but they either never merged or have since been revised. I suggested this path though, so this approach is fine to merge for now, but eventually I may change this to just use a toy package contained within the repo, since the overall test harness already happens in a context which contains a build of catkin, per https://github.com/catkin/catkin_tools/blob/master/.travis.yml#L36.

[mikepurvis]

Is this necessary? Pretty sure we should always just call python and let the PATH set by travis steer that to the right place.

[timonegk]

Building catkin with python 2 requires the catkin_pkg package for python 2. This is not related to the python version for catkin_tools which is managed by travis

[timonegk]

Are you sure that setting the PYTHON environment variable actually changes the used python version? The travis log suggests otherwise, e.g. in the current build in line 597, the travis virtualenv is used.

[timonegk]

Well, I just realized that the new get_python_install_dir function is never called in the tests. I adapted the tests to use a CMake package from the resources folder and made sure that the function is actually called this time. For that it was necessary to set the workspace layout to something other than "linked" to avoid using catkin_tools_prebuild which uses catkin's own logic to generate the setup files. (When building the catkin package as we did before, catkin generates the setup files too.)

[mikepurvis]

This looks terrific. Really appreciate this contribution and your patience in iterating on it with me.

[timonegk]

Thank you, I'm happy to contribute!