Find the python version specified in ROS_PYTHON_VERSION

[clalancette]

If ROS_PYTHON_VERSION is defined, make catkin respect that and go looking for at least that version. If it is not specified, just fall back to whatever the platform version of Python is.

[dirk-thomas]

Could the PYTHON_VERSION cache variable be initialized with $ENV{ROS_PYTHON_VERSION} to avoid the extra logic?

Please describe how you have tested these changes with Python 2/3 in a workspace.

[clalancette]

Could the PYTHON_VERSION cache variable be initialized with $ENV{ROS_PYTHON_VERSION} to avoid the extra logic?

Potentially. I'll take a look at doing that.

Please describe how you have tested these changes with Python 2/3 in a workspace.

I have a workspace setup on Bionic that has a bunch of core packages (ros_comm, cmake_modules, gencpp, message_generation, roscpp_core, etc). I also have a checked-out version of catkin with the changes in this PR. I call catkin by running src/catkin/bin/catkin_make_isolated --install, and tested the following:

• No ROS_PYTHON_VERSION set at all (i.e. with an unmodified ros_environment package). When doing that, it ends up calling find_package(PythonInterp REQUIRED), which finds Python 2 by default on Bionic. This installs all python code to install_isolated/lib/python2.7/dist-packages.
• With ROS_PYTHON_VERSION explicitly set to 2 (what we expect the default on Melodic to be once we merge and release Add in ros_python_version. ros_environment#4). That calls find_package(PythonInterp 2 REQUIRED), which ends up the same as above.
• With ROS_PYTHON_VERSION explicitly set to 3 (what we expect users to use when they want to try out Python 3). In that case, it ends up calling find_package(PythonInterp 3 REQUIRED), which finds Python 3 properly. This also propagates to all downstream packages, so all other packages that use find_package(catkin ...) end up picking up Python 3 by default in CMake. This installs all python code to install_isolated/lib/python3/dist-packages.

Now that I have most of this going, I'm going to try to start building higher in the stack; let me know if there are other tests in particular that you think are valuable.

[dirk-thomas]

Out of curiosity: do the tests pass with Python 3 for your workspace?

[clalancette]

Out of curiosity: do the tests pass with Python 3 for your workspace?

Unfortunately not:

build_isolated/genpy/test_results/genpy/nosetests-test.xml: 44 tests, 3 errors, 2 failures, 0 skipped
build_isolated/message_filters/test_results/message_filters/nosetests-test.test_approxsync.py.xml: 1 tests, 1 errors, 0 failures, 0 skipped
build_isolated/rosgraph/test_results/rosgraph/nosetests-test.xml: 53 tests, 2 errors, 0 failures, 0 skipped
build_isolated/roslaunch/test_results/roslaunch/nosetests-test.unit.xml: 107 tests, 4 errors, 0 failures, 0 skipped
build_isolated/rosmsg/test_results/rosmsg/nosetests-test.xml: 31 tests, 7 errors, 7 failures, 0 skipped
build_isolated/rostest/test_results/rostest/rostest-test_hztest.xml: 1 tests, 0 errors, 1 failures, 0 skipped
build_isolated/rostest/test_results/rostest/rostest-test_publishtest.xml: 1 tests, 0 errors, 1 failures, 0 skipped
build_isolated/rostopic/test_results/rostopic/nosetests-test.test_rostopic_command_line_offline.py.xml: 2 tests, 2 errors, 0 failures, 0 skipped
build_isolated/rosunit/test_results/rosunit/nosetests-test.xml: 18 tests, 8 errors, 3 failures, 0 skipped
build_isolated/roswtf/test_results/roswtf/nosetests-test.xml: 3 tests, 0 errors, 1 failures, 0 skipped
build_isolated/roswtf/test_results/roswtf/rosunit-roswtf_command_line_online.xml: 2 tests, 0 errors, 1 failures, 0 skipped
Summary: 919 tests, 27 errors, 16 failures, 0 skipped

I haven't yet looked into why.

[clalancette]

Could the PYTHON_VERSION cache variable be initialized with $ENV{ROS_PYTHON_VERSION} to avoid the extra logic?

Potentially. I'll take a look at doing that.

Commit ca20ea7 ends up revamping this to use PYTHON_VERSION instead, through an intermediate variable. With this in place the logic is greatly simplified; I tested out all of the scenarios above, plus manually specifying -DPYTHON_VERSION on the command-line, and all work as expected.

[dirk-thomas]

Commit ca20ea7 ends up revamping this to use PYTHON_VERSION instead, through an intermediate variable.

Can you describe why this intermediate variable is necessary. If the environment variable is not defined it should evaluate to an empty string.

[clalancette]

Can you describe why this intermediate variable is necessary. If the environment variable is not defined it should evaluate to an empty string.

Ah, I didn't realize that about CMake. OK, then we don't need the intermediate and it is even simpler. Will fix that.