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

[noetic] Rosrun uses heuristic to pick python devel-space relay script #233

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jan 23, 2020

This is to support ros/catkin#1044 .That PR adds a devel-space relay script for installed python executables so their shebang can be rewritten to match ROS_PYTHON_VERSION. It has a drawback that rosrun will warn about non-unique executables because it sees one in the devel space and one in src space. This PR adds an exception to that warning.

The first commit is a slight refactoring that should not change rosrun's behavior.

  1. 6773140 makes the script use pkg_name and file_name instead of $1 and $2 because it is confusing to read when adding a function that also uses arguments.
  2. 61d017c makes rosrun pick the first executable if there are exactly two and one came from the --libexec list and another came from the --share list.

@dirk-thomas
Copy link
Member

06a2805 splits the catkin_find invokaction into two: once with --libexec and once with --share.

This will slow down the command since Python needs to be invoked another time. Since this will affect every invocation of rosrun I am not sure that is ok. The alternative would be to leave the single invocation as is and only if it returned 2 matches query catkin_find once again (with only one of the two arguments). That would only increase the runtime when there are actually two matches which should be much less common.

@sloretz
Copy link
Contributor Author

sloretz commented Jan 23, 2020

This will slow down the command since Python needs to be invoked another time. Since this will affect every invocation of rosrun I am not sure that is ok.

It does seem to slow it down; I'll implement an alternative. The average run time of 6 runs on my machine (bailing before exec) is 0.498s before 06a2805, and 0.781s after.

@sloretz
Copy link
Contributor Author

sloretz commented Jan 24, 2020

The alternative would be to leave the single invocation as is and only if it returned 2 matches query catkin_find once again (with only one of the two arguments). That would only increase the runtime when there are actually two matches which should be much less common.

Implemented in 35c874f 61d017c

@sloretz
Copy link
Contributor Author

sloretz commented Jan 31, 2020

@dirk-thomas mind having another look at this PR? I assume this will need a noetic-devel branch since it's only purpose is to support ros/catkin#1044 which has been merged already on catkin's noetic-devel branch.

@dirk-thomas dirk-thomas changed the title Rosrun uses heuristic to pick python devel-space relay script [noetic] Rosrun uses heuristic to pick python devel-space relay script Jan 31, 2020
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Will do the branching and merging when we are about to release this repo.

@sloretz
Copy link
Contributor Author

sloretz commented Feb 10, 2020

@ros-pull-request-builder retest this please

1 similar comment
@dirk-thomas
Copy link
Member

@ros-pull-request-builder retest this please

@dirk-thomas dirk-thomas mentioned this pull request Feb 11, 2020
@dirk-thomas
Copy link
Member

Moved the first commit into a separate PR: #239.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Feb 11, 2020

Looks like Noetic PR job passed 🎉

@sloretz
Copy link
Contributor Author

sloretz commented Feb 11, 2020

Since this is a companion to ros/catkin#1044 which was merged onto the noetic-devel branch, I think this PR might need to be re-targeted. @dirk-thomas mind creating a noetic-devel branch?

@dirk-thomas
Copy link
Member

I think this PR might need to be re-targeted.

Yes, as the [noetic] prefix in the title indicates.

mind creating a noetic-devel branch?

Not yet, before forking I need to land code cleanup changes.

@dirk-thomas
Copy link
Member

before forking I need to land code cleanup changes.

Done in #240 and #241.

@dirk-thomas dirk-thomas changed the base branch from kinetic-devel to noetic-devel February 12, 2020 00:36
@dirk-thomas dirk-thomas merged commit d15a82a into ros:noetic-devel Feb 12, 2020
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

2 participants