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

Fix python executable on Windows. #23

Merged
merged 1 commit into from
Dec 28, 2013

Conversation

AurelienBallier
Copy link

Hi,

Please consider adding this small patch.
It's useful to make sure the python script is executed using python and not the system default program.

Regards,

@dirk-thomas
Copy link
Member

That should not be necessary. If you have installed Python using the installer it associates the .py extension with Python (http://docs.python.org/2/faq/windows#how-do-i-make-python-scripts-executable). Have you installed Python manually?

@AurelienBallier
Copy link
Author

Actually no I have installed Python properly, but I then have switched the default program for .py files.
Even switching back to python executable was giving me troubles.
But I find it much safer to use this small trick to ensure a proper execution of the script, it's not system dependent. By the way it's done this way in the same file, and in some others.

@dirk-thomas
Copy link
Member

gencpp and genlisp invoke the script with the Python executable. But e.g. actionlib_msgs does invoke its script without (like genpy). And I am pretty sure there are several other locations where we invoke Python scripts without being prefixed with the interpreter (e.g. all dry compatibility code).

One more thing is that using the Python executable explicitly assumes that the caller knows which Python version the script should be run with. Otherwise the scripts shebang line can make that decision. So invoking it without seems to be the better choice imo.

@stonier Can you comment on this ticket with your experience building/running ROS under Windows? How does it work for you that genpy invokes its Python script without prefixing it with the Python executable? Is there anything specific to setup/configure under Windows?

@stonier
Copy link
Contributor

stonier commented Dec 20, 2013

Since it works for our message generation, I'm guessing it's just able to utilise the default. Since @AurelienBallier changed that, I can imagine the result is catostrophic.

Not that he's doing something wrong, windows doesn't have the schebang to split the execution program from the default opening program. At least not until python 3.3 when apparently some mechanism is coming to windows.

Some observations:

  • Seeing as it's used everywhere else in catkin, avoiding it here seems to have no practical purpose
  • Hopefully one day we can make use of the schebang plans in python 3.3 for windows
  • Perhaps we can determine schebang usage/non-usage from detecting the shell catkin is setting up/using?
    • And then implement throughout catkin across the many other usages of PYTHON_EXECUTABLE.

@AurelienBallier
Copy link
Author

Ok, first of all I have found that my weird behavior was coming from file association but not because of the 'system', here is some explanations :

  • Windows use some registry entries for file association (HKEY_CURRENT_USER\Software\Classes or HKEY_LOCAL_MACHINE\Software\Classes), these are used by python to define the interpreter. In my case everything was fine.
  • My problems were coming from another registry entry (HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\FileExts), which is the windows explorer file association and is hidding system file association. I just had to remove .py entry to get rid of this annoying behavior.
  • The result of this problem was not python being not called, but arguments were not passed as the command was not properly formed.

This solution make my patch useless as far as we keep python as the associated program to .py files.

But as @stonier pointed out, shebang is not interpreted on Windows. So, there is no way to enforce a specific version right now.

As for shebang policy, I also made some research about this new shebang interpreter in python 3.3, and I've found some interesting informations on http://pymolurus.blogspot.fr/2012/07/python-launcher-brings-shebang-line.html.
Long story short, there is a software that interpret the shebang and launch the approriate python version. This program is associated to .py files instead of python interpreter.
We actually do not need to wait until python 3.3 to interpret shebang we could just ask windows users to install the launcher (https://bitbucket.org/vinay.sajip/pylauncher/downloads). Because basically python 3.3 is just including this piece of software.

@dirk-thomas
Copy link
Member

I will close this pull request since the patch is not necessary to make Python scripts be executed by the Python interpreter on Windows.

In order to rely on the shebang line other locations should even avoid using PYTHON_EXECUTABLE explicitly when invoking Python scripts.

dirk-thomas added a commit that referenced this pull request Dec 26, 2013
dirk-thomas added a commit to ros/genlisp that referenced this pull request Dec 26, 2013
dirk-thomas added a commit to ros/gencpp that referenced this pull request Dec 26, 2013
dirk-thomas added a commit to ros/catkin that referenced this pull request Dec 26, 2013
@stonier
Copy link
Contributor

stonier commented Dec 26, 2013

@dirk-thomas By not including this, you're effectively saying that windows people should never assign a different default to .py extensions. That is something that ros should not and can't dictate and is a difficult situation for windows since handling executions in the shell and opening editors is governed by the same rule.

I'm curious as to why @AurelienBallier switched the default. Is that an esoteric or fairly common use case?

@dirk-thomas
Copy link
Member

In several locations where Python scripts are invoked the Python interpreter is not listed explicitly. Since that has been working until now under Windows my recent commits only unified this behavior (by generally not using the Python executable when invoking executable Python scripts).

@stonier Do you think we should prepend the Python executable to every invocation of Python scripts in order to allow users to assign a different handler to .py files? If yes, that would require numerous locations in ROS to be updated.

@stonier
Copy link
Contributor

stonier commented Dec 27, 2013

Looking at python.cmake in catkin, it seems as if the initial design of catkin was to utilise cmake for python discovery (this is where the PYTHON_EXECUTABLE gets set).

This is also where some custom decisions for python could probably be made. It already customises the install dir for python depending on python setuptools being WIN32 or no. Creating and using a variable that would get set to PYTHON_EXECUTABLE if a non-schebang platform (currently WIN32), and empty otherwise would satisfy both parties until a windows implementation with schebang support is made ready.

And yes, you're right, there are other places where it's needed but at least this gives robust support to windows for catkin related stuff. Other areas can be incrementally improved as we hit on problems. I know roslaunch/rosrun have an ugly (and naive) commit which could be improved to enable windows to run python scripts without .py endings as well.

@dirk-thomas
Copy link
Member

On non-Windows systems we definitely don't want to use the PYTHON_EXECUTABLE when invoking Python scripts. As mentioned before then each script can make a reasonable decision in its own shebang line.

For Windows I would consider that .py files should be run by a Python interpreter. If you disagree on this and think that we need to support configurations where users have mapped it differently we need a new variable for that PYTHON_EXECUTABLE_ONLY_SET_ON_WINDOWS and use that everywhere where Python scripts are invoked from CMake. Can you imagine a better name for this new variable?

@stonier
Copy link
Contributor

stonier commented Dec 27, 2013

On non-Windows systems we definitely don't want to use the PYTHON_EXECUTABLE when invoking Python scripts. As mentioned before then each script can make a reasonable decision in its own shebang line.

👍 totally agreed.

For Windows I would consider that .py files should be run by a Python interpreter. If you disagree on this and think that we need to support configurations where users have mapped it differently we need a new variable for that PYTHON_EXECUTABLE_ONLY_SET_ON_WINDOWS and use that everywhere where Python scripts are invoked from CMake. Can you imagine a better name for this new variable?

I was actually thinking about a name for that variable in the previous post, but nothing really decent came to mind! I think that is a good descriptive name, or possibly better might be PYTHON_EXECUTABLE_ONLY_SET_FOR_NON_SCHEBANG_PLATFORMS, but that's getting ridiculously long.

@AurelienBallier
Copy link
Author

I'm curious as to why @AurelienBallier switched the default. Is that an esoteric or fairly common use case?

Nothing fancy, I just wanted to be able to fast double-click to open .py files with my editor.

For Windows I would consider that .py files should be run by a Python interpreter. If you disagree on this and think that we need to support configurations where users have mapped it differently we need a new variable for that PYTHON_EXECUTABLE_ONLY_SET_ON_WINDOWS and use that everywhere where Python scripts are invoked from CMake. Can you imagine a better name for this new variable?

Yes, that will allow end users to mess up with their file association, and still take advantages of shebangs.
Users should be asked to install shebang interpreter as a prerequisite for ROS on Windows.
Another name proposition : PYEXEC_SHEBANG_AWARE which point to the shebang interpreter.

dirk-thomas added a commit to ros/catkin that referenced this pull request Dec 28, 2013
@dirk-thomas
Copy link
Member

We talked about that more and don't want the behavior on different platform to diverge further. Therefore we will keep the PYTHON_EXECUTABLE in front of scripts (and ignore their shebang line) for now.

@dirk-thomas dirk-thomas reopened this Dec 28, 2013
dirk-thomas added a commit that referenced this pull request Dec 28, 2013
Fix python executable on Windows.
@dirk-thomas dirk-thomas merged commit 2f5e6c7 into ros:hydro-devel Dec 28, 2013
@dirk-thomas
Copy link
Member

If you find further locations where Python scripts are invoked without PYTHON_EXECUTABLE please comment here or open new tickets on the corresponding issue tracker.

@AurelienBallier
Copy link
Author

Ok, thanks for the merge.

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 this pull request may close these issues.

None yet

3 participants