-
Notifications
You must be signed in to change notification settings - Fork 914
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
change how commands are executed #1628
Merged
dirk-thomas
merged 7 commits into
ros:melodic-devel
from
kejxu:change_how_commands_are_executed
Mar 16, 2019
Merged
change how commands are executed #1628
dirk-thomas
merged 7 commits into
ros:melodic-devel
from
kejxu:change_how_commands_are_executed
Mar 16, 2019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* rework shell=True for Windows change * remove .exe handling, prepend python for python scripts instead * update Windows search logic * move comments around and remove unnecessary comments * minor refactor * file is a keyword in python2, rename as f * remove unnecessary \ at the end of line
I checked CI log, but not exactly sure about why the build failed. is there anything specific I should check for? |
@ros-pull-request-builder retest this please |
dirk-thomas
reviewed
Feb 26, 2019
just pinging on this thread =) is there any change we would need to make? |
Thank you for the patch. |
tahsinkose
pushed a commit
to tahsinkose/ros_comm
that referenced
this pull request
Apr 15, 2019
* Use non-posix way to split command string. * Use shell feature for roslaunch command. * Resolved the merge again. * rework shell=True for Windows change (ros#51) * rework shell=True for Windows change * remove .exe handling, prepend python for python scripts instead * update Windows search logic * move comments around and remove unnecessary comments * minor refactor * file is a keyword in python2, rename as f * remove unnecessary \ at the end of line * use sys.executable to launch Python interpreter * follow inline comment style in PEP8 * invert logic
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this is another problem due to the lack of shebang support on Windows, there are 2 common ways of using the
command
attribute in the launch file:PATH
variable, for example,rosversion
, an entry point that comes with an executable. this kind of executables tend to sit in<python_install_dir>\Scripts
, and can be called and executed directly. because of this, usually only the executable's name (not an absolute path) is given, like this:the first scenario works like a charm on Windows too, thanks to the
console_script
support. However, for the second scenario, things are a little bit different:$(find xacro)/xacro
would point to a file without any extension, that means the file would not be executable on Windowsxacro.exe
. unless the subprocess (subprocess.Popen(command, stdout=subprocess.PIPE)
) is spawned with an extrashell=True
flag, the following error would be returned:to fix this, here is our proposed solution:
.exe
files on Windows), but no extension (e.g..exe
) is given$(find pkg)/exec
, try to find an executable (exec.exe
on Windows) of the same namequestions I asked myself:
is this the best fix?
not necessarily. since
rosrun
already does something very similar, this would create a separate logic from existing code. the actual right fix (imo) is to change commands liketo
speaking of which, we're working on another pull request to create a Windows-version of
rosrun
. which just like the existing shell script, would be a batch script. however, it would probably be helpful to Python-izerosrun
in the future so we/ROS only need to maintain one code base across all platforms.is this fix still needed? can't we just change those commands to use
rosrun
?I believe so (that's why we're still sending this out =) ), the reasons are as follows:
command
attribute, it would be hard to find and fix them allrosrun
cannot be used, and for some reason, still don't want to add.exe
extension, maybe for better portability across platforms. in this case, it would be helpful ifroslaunch
could search for an executable of the same name under the same directory