add win_ros script wrappers to make python scripts executable

[kejxu]

Python scripts are executable (can be directly called on command line with the help of the shebang line) on Ubuntu, so built packages can be directly used in command line. However, without extra setup on Windows, python scripts cannot be executed directly as on Ubuntu. To work around this issue, we utilize 2 kinds of script wrappers to execute the python scripts.

One way of doing it is creating a batch script that simply adds that python prefix before the python script; the other way of doing it is to wrap the python script inside a win32 wrapper, and execute it using the CreateProcess call (and return error code).

Similar to the change to CMakeLists.txt in this pull request, adding a if(WIN32) + add_windows_helper would be the easiest way to create executable for packages.

[dirk-thomas]

One way of doing it is creating a batch script that simply adds that python prefix before the python script; the other way of doing it is to wrap the python script inside a win32 wrapper, and execute it using the CreateProcess call (and return error code).

Can you please describe the differences (in terms of pros and cons) of these two approaches and why both should be provided.

[kejxu]

Can you please describe the differences (in terms of pros and cons) of these two approaches and why both should be provided.

batch script wrapper
pros:

• very straightforward in terms of how it works

con:

• does not return error code from the underlying python process
• pressing CTRL+C will lead to an additional interactive interrupt (the underlying python process also receives the keyboard interrupt)

Terminate batch job (Y/N)?

win32 executable wrapper
pro:

• can return error code

con:

• has a higher complexity

The idea of such kind of wrappers were borrowed from an earlier ros-win project, so we were trying to keep as much unchanged as possible while updating the content. This, I believe, is the major reason why both are kept. The batch script wrappers for catkin executables were introduced during the proof-of-concept phase, they could totally be changed into the win32 wrappers.

The reason why such kind of wrappers are needed is because .exe and .bat files are natively executable on Windows, while .py files are not. .py files could become executable by adding system-wide configuration and associate them with the correct python interpreter. However, we thought it was not the best idea given our experience working with "corrupted" environments (machine with existing python environments).

Meantime, the pros and cons of this wrapper method are as follows:
pro:

• will not change existing package installation logics on Linux/Ubuntu
• only minimum change required to enable packages on Windows

con:

• add_windows_python_xxx_helper needs to be added for every package that would want to be called from the command line (just like catkin_make_isolated)

[dirk-thomas]

does not return error code from the underlying python process

This on its own is reason enough for me to consider to only offer the second option.

[kejxu]

looking back at this change, this seems like a hacky way of fixing Python scripts on Windows. @seanyen-msft found the use of setuptools in Python to generate console_scripts, which should help avoid the executable generation problem (Python scripts needs a wrapper to be called on Windows). what's even better is that this would be a maintained method

I will be doing some prototyping to see if this is going to work. @wjwwood and @dirk-thomas, does this sound like something we/ros could use?

[wjwwood]

We use the console_scripts entry point in all the other python tools that we support and that work on Windows (AFAIK). So it would make sense to use them I think, though I'm not sure the layout of the existing scripts lends themselves to that, so you may need to move code around. But ultimately I think it is probably better than having our own executable for this case.

[kejxu]

We use the console_scripts entry point in all the other python tools that we support and that work on Windows (AFAIK). So it would make sense to use them I think, though I'm not sure the layout of the existing scripts lends themselves to that, so you may need to move code around. But ultimately I think it is probably better than having our own executable for this case.

good to know, I'll update on this thread while I try. we have a feeling that having some sort of wrapper might later on become hard to maintain

[kejxu]

We use the console_scripts entry point in all the other python tools that we support and that work on Windows (AFAIK).

I did some investigation and prototyping on this and here are a little of conclusion:

• changing to use setuptools.setup with entry_points={'console_scripts': [...]} works well for (local) install destination install_isolated (also fairly easy to implement)
• haven't had time to dive deeper into catkin build logic to get this working for devel space, there likely needs to be extra steps during build time
• console_scripts as entry_points from setuptools would associate the generated .exe file with the local Python interpreter on the local machine. that means, if the build machine on which the Windows package is built has Python interpreter installed at C:\Python2, the .exe would always expect to find Python interpreter wherever it's executed. Up to this point, ROS on Windows porting uses a separate Python installation to work around the "Python already exists" problem. This means the Python interpreter for any (Windows) ROS project would be at a customized local directory, basically making it impossible to work on another machine or even maintain.

with Embedded World approaching, my to-do to unblock us/ROS from this pull request will be to rewrite our own version of cmake/templates/ros_bin.cpp. at this point, this version of ros_bin.cpp is already different from the original code given it now returns error code of the process (which is important for process monitoring in ROS).
justifications for a (local) win32 wrapper are as follows:

• the easiest/most straight-forward approach to make Python scripts executable is to add python at the beginning to execute the Python script with the Python interpreter registered in the ROS environment.
• the current problem does not require the .exe win32 wrapper to work independently, it only needs to execute the actual .py file
• keeping the .py file also makes it easy for users to see the actual code

@dirk-thomas do you think we would still have the license requirement if we rewrite everything?

some thoughts from working on this:

• has ROS ever considering working with virtualenv for isolated Python environment? We had tons of troubles working with (polluted) environments with existing Python installation, venv might be one (easy) way to solve it, imo
• console_scripts seems to well for packages like rosinstall_generator, which are installed as Python package. for local ROS packages, I am not sure if it will ever work. maybe it will become feasible once we have an isolated dev environment with virtualenv. any thoughts on this?
• add_python_executable_helper could be further integrated into catkin_python_setup or maybe even catkin_install_python to help add executable wrapper for (Python script) nodes

[kejxu]

One major limitation of console scripts (afaik) is that you can only install them to one location - usually bin - with some effort also into lib/<pkgname>. But you can't install one script into bin and another into lib/<pkgname>.

Thanks for sharing your expertise! This would make console scripts barely practical, thanks!

Please comment on the ticket after committing to the branch in order to notify others of the change.

Sorry for the confusion, updated on this ticket to address all the comments!

[dirk-thomas]

I polished the CMake function add_python_executable a bit in 1810908 (I hope I didn't break anything).

Since the _setup_util.py file will also get an executable wrapper with this PR should the invocation in the setup.bat file be updated to call that executable instead? That would get rid of the custom logic around the Python interpreter in that file.

[kejxu]

I polished the CMake function add_python_executable a bit in 1810908 (I hope I didn't break anything).

thanks for helping out! it looks a lot better than what I could come up with my understand of CMake =) and thanks for moving configure_file(WRAPPER_SOURCE) into if(WIN32)! I did a local build and it worked fine; and I will be testing this with our daily build to make sure it does not break anything else

quick question, do we want to change the name mentioned in wrapper.cpp.in?

Since the _setup_util.py file will also get an executable wrapper with this PR should the invocation in the setup.bat file be updated to call that executable instead? That would get rid of the custom logic around the Python interpreter in that file.

update to invoke _setup_util.py instead in setup.bat

[dirk-thomas]

I will be testing this with our daily build to make sure it does not break anything else

Thanks - better safe than sorry. Especially since releasing a patch for catkin is rather expensive in terms of resources on our buildfarm 😉

do we want to change the name mentioned in wrapper.cpp.in?

Since the file is based on their work I would rather keep it as is.

[kejxu]

updated setup.bat.in to invoke _SETUP_UTIL_EXE=@SETUP_DIR@/_setup_util.py.exe if it exists.

I think it's also safe to skip the %_PYTHON% "%_SETUP_UTIL%" %* step completely, but that seems risky to me without further testing. Given the importance of setup.bat, I think that would be a risk we don't have to take.

better safe than sorry

totally! good news is, this change should not affect any ROS installation on Linux since it's guarded to be Windows-specific.

[dirk-thomas]

I think it's also safe to skip the %_PYTHON% "%_SETUP_UTIL%" %* step completely, but that seems risky to me without further testing. Given the importance of setup.bat, I think that would be a risk we don't have to take.

That doesn't sound reasonable to me. If for whatever reason the _setup_util.py.exe file isn't available that means the CMake function proposed in this PR doesn't work as expected. That would be fairly fatal anyway - I don't think having a backup just for the setup util invocation would help much.

[dirk-thomas]

Thank you for iterating on this patch.

Are you confident that the current state covers the necessary use cases on Windows? Or would you like to have more time for testing? I am mostly asking to avoid doing a patch release soon 😉

[kejxu]

Are you confident that the current state covers the necessary use cases on Windows?

Yes, this should cover basic use cases, should be good for the next catkin release. However, I have to be honest and admit that this might not cover all possible scenarios since our bandwidth has not allowed us to test intensively or thoroughly.

If there should be any further (major) improvement/fix/change, I think that could go into a separate release (depends on future development plans). Given our time frame, (imo) it would be good enough if this immediate release of catkin could provide basic (necessary) functionality (enables building and running natively) for (almost) all ROS packages.

Or would you like to have more time for testing?

At this point, we have verified that current state of catkin builds without a problem. Running test suites or real use cases (like turtlesim or turtlebot) could take much longer, so I would prefer to do that after the changes are merged (into ROS and our mainline).

Is there a release plan in vision? As soon as we get catkin changes merged, I will starting cleaning up our ms-iot forks and doing the verification mentioned above. If there is any obvious issue, I will make sure to have the fix out before EOD Friday (3/1)

[dirk-thomas]

Sounds good. I am able to do a release tomorrow...

[kejxu]

looking into an error found while building desktop_full.

error message:

==> Processing catkin package: 'soem'
==> Creating build directory: 'build_isolated\soem'
==> Building with env: 'C:\opt\ros\melodic\x64\env.bat'
==> cmake D:\a\1\a\_ws\src\soem -DCATKIN_DEVEL_PREFIX=D:\a\1\a\_output\devel_isolated\soem -DCMAKE_INSTALL_PREFIX=C:\opt\ros\melodic\x64 -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_PREFIX_PATH=C:/opt/rosdeps/x64;C:/opt/ros/melodic/x64 -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_PDB_OUTPUT_DIRECTORY=D:\a\1\a\_symbols -DPYTHON_VERSION=2.7 -DPYTHON_EXECUTABLE=c:\opt\python27amd64\python.exe -DPYTHON_LIBRARIES=c:\opt\python27amd64\libs -DCATKIN_SKIP_TESTING=ON -G NMake Makefiles in 'D:\a\1\a\_output\build_isolated\soem'
-- The C compiler identification is MSVC 19.16.27026.1
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Using CATKIN_DEVEL_PREFIX: D:/a/1/a/_output/devel_isolated/soem
-- Using CMAKE_PREFIX_PATH: C:/opt/rosdeps/x64;C:/opt/ros/melodic/x64
-- This workspace overlays: C:/opt/ros/melodic/x64
-- Found PythonInterp: C:/opt/python27amd64/python.exe (found suitable version "2.7.15", minimum required is "2.7") 
-- Using PYTHON_EXECUTABLE: C:/opt/python27amd64/python.exe
-- Using default Python package layout
-- Found PY_em: C:\opt\python27amd64\lib\site-packages\em.pyc  
-- Using empy: C:/opt/python27amd64/lib/site-packages/em.pyc
-- Using CATKIN_SKIP_TESTING: ON (implying CATKIN_ENABLE_TESTING=OFF)
-- Call enable_testing()
-- Using CATKIN_TEST_RESULTS_DIR: D:/a/1/a/_output/build_isolated/soem/test_results
-- Found gtest: gtests will be built
-- Using Python nosetests: C:/opt/python27amd64/Scripts/nosetests-2.7.exe
-- catkin 0.7.14
-- BUILD_SHARED_LIBS is on
OS is win32
LIB_DIR: lib
-- Configuring done
CMake Error: CMake can not determine linker language for target: _setup_util.py_executable_install_python
CMake Error: Cannot determine link language for target "_setup_util.py_executable_install_python".
-- Generating done
CMake Warning:
  Manually-specified variables were not used by the project:

    PYTHON_LIBRARIES


-- Build files have been written to: D:/a/1/a/_output/build_isolated/soem
<== Failed to process package 'soem': 

the root cause for this seems seems to be soem specifies itself to be a C-only project, so CXX compiler is disabled

[dirk-thomas]

Looking at the source of that package I don't see why this is happening. But any package could explicitly not select the C++ compiler by calling e.g. project(<name> LANGUAGE C). And in that case there would be no C++ compiler selected to build the expanded python_win32_wrapper.cpp.in file. Can you please check the content of the CMake variable ENABLED_LANGUAGES in this case (just to figure out why this case fails)?

In general I am not sure if there is a way in CMake to add CXX later if the package explicitly didn't select it. If there this patch could use that to explicitly the C++ compiler on demand when needed to compile the expanded template. If not we might need to reconsider the approach.

[dirk-thomas]

You might want to try enable_language().

[kejxu]

You might want to try enable_language().

Thanks for sharing your expertise! That's also what I looked into, and I have updated this change accordingly.

To verify the enable_language() change, this build was queued. However, it failed with the following error message:

CMake Error at C:/opt/ros/melodic/x64/share/catkin/cmake/platform/windows.cmake:128 (add_executable):
  add_executable cannot create target "xacro_executable_devel" because
  another target with the same name already exists.  The existing target is
  an executable created in source directory "D:/a/1/a/_ws/src/xacro".  See
  documentation for policy CMP0002 for more details.
Call Stack (most recent call first):
  C:/opt/ros/melodic/x64/share/catkin/cmake/catkin_python_setup.cmake:156 (add_python_executable)
  CMakeLists.txt:15 (catkin_python_setup)

I looked into the source code of xacro and found out that xacro\CMakeLists.txt is calling catkin_python_setup twice. I am not sure if that is intended or not, so have raised ros/xacro#205 and proposed a fix.

Meantime, this issue has been addressed for our ms-iot fork with https://github.com/ms-iot/xacro/commit/0c8f06c3a7734633349cc5fd6abcafb33ca7b389, and locally verified. A new build has been queued. The only 4 packages that were not built in the previous build were:

- xacro
- diff_drive_controller
- gripper_action_controller
- joint_trajectory_controller

xacro has been locally verified, and the other 3 should not have any issue either. pretty sure this will result in a successful build.

what's happening next:

• one of our daily builds will exercise --make-args run_tests, that will help me verify if this change would affect any test run (this should not)
• after the binaries come out, I will also test a few basic use cases: roscore, rostopic, rosnode, etc. just to verify we cover basic scenarios, this should be finished before EOD 3/1

[kejxu]

did some verification for https://ros-win.visualstudio.com/ros-win/_build/results?buildId=3344 =)

• this change does not affect running tests. test run didn't have any (new) problem compared to n-1 build. (there seems to be some problem with the number of tests run, I will look into that in a separate change)
• tested roscore, rostopic list, rostopic pub, rostopic pub, rosnode list, rosnode ping, all worked
• tested roslaunch turtlebot3_bringup turtlebot3_model.launch, this verifies that executable built in packages like (xacro) could be launched. (another pull request for this: change how commands are executed ros_comm#1628)

[dirk-thomas]

Thank you for this patch and for iterating on it.