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

flexbe_states: create symlink to behavior and state sources in installed package path #56

Open
wants to merge 1 commit into
base: feature/flexbe_app
Choose a base branch
from

Conversation

meyerj
Copy link

@meyerj meyerj commented Jun 20, 2018

I had to add this cmake snippet to the flexbe_states package and also to all our custom state and behavior packages to fix an error message in the FlexBE App when running from install-space:

Setting up ROS connection...
Checking 257 ROS packages for states and behaviors...
ROS connection running!


Parsing sourcecode...
Code parsing completed.
Building behavior state machine...
Unable to find state definition for: CheckConditionState
Please check your workspace settings.
Unable to find state definition for: CalculationState
Please check your workspace settings.
Unable to find state definition for: FlexibleServiceCallerState
Please check your workspace settings.

Another alternative is to always install an additional copy of the src folder to the package dir (${CATKIN_PACKAGE_SHARE_DESTINATION}), independent of the files installed to lib/.

A better approach would be to patch the FlexBE App such that it can find the source code using python:

$ python -c 'import flexbe_states; print(flexbe_states.__path__)'
['/path/to/install/space/lib/python2.7/dist-packages/flexbe_states']

or relative to the package path:

$ echo $(rospack find flexbe_states)/../../lib/python2.7/dist-packages/flexbe_states
/path/to/install/space/share/flexbe_states/../../lib/python2.7/dist-packages/flexbe_states

But I don't know enough about JavaScript and the underlying mechanics to propose such a patch. The relevant code seems to be in src/io/io_packageparser.js.

It does not make sense to modify behaviors in an install-space or if the sources are not writable by the current user, e.g. because they are owned by root. But I assume the FlexBE App would still need to find the sources in order to only run a behavior. It would be a nice-to-have that the respective controls ("Statemachine Editor", "Save Behavior", ...) are disabled in those cases.

@@ -25,6 +25,14 @@ catkin_package(
#install(PROGRAMS bin/hello
# DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION})

install(CODE "
file(MAKE_DIRECTORY \"$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/src\")
Copy link
Author

Choose a reason for hiding this comment

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

Respecting the DESTDIR environment variable is required for Debian-packaging (https://www.gnu.org/prep/standards/html_node/DESTDIR.html).

@pschillinger
Copy link
Member

Thank you for looking into this!

I will check what to do best from the flexbe_app side to handle the case of installed packages.

@DorianScholz
Copy link
Member

DorianScholz commented Jul 1, 2018

I stumbled across the same problem just last week without seeing your bug report.
I fixed it in our branch using a similar solution in the two commits:
5f89d83
540b313

@DorianScholz
Copy link
Member

PR for flexbe_app now also submitted:
FlexBE/flexbe_app#20

@pschillinger
Copy link
Member

pschillinger commented Jul 4, 2018 via email

@pschillinger
Copy link
Member

As suggested in the original PR, the following features have been added:

A better approach would be to patch the FlexBE App such that it can find the source code using python

It would be a nice-to-have that the respective controls ("Statemachine Editor", "Save Behavior", ...) are disabled in those cases.

@meyerj and @DorianScholz can you check flexbe_app@feature/install_support whether this addresses your requirements appropriately before I close this PR?

@meyerj
Copy link
Author

meyerj commented Sep 9, 2018

@meyerj and @DorianScholz can you check flexbe_app@feature/install_support whether this addresses your requirements appropriately before I close this PR?

Sorry, I am not using FlexBe at the moment and have no time to test the branch. I assume that you or @DorianScholz actually tested the approach, but I fear that catkin's special behavior to "link" Python packages to the devel-space using exec(fh.read()) in devel/lib/python2.7/dist-packages/<pkg>/__init__.py (created from init.py.in) could invalidate the approach of getPackagePythonPath() in ros.js. It always returns the package's Python folder in devel-space, but not the actual source-space that contains the editable source files:

$ python -c 'import imp; print(imp.find_module("my_python_pkg")[1])
/path/to/devel/lib/python2.7/dist-packages/my_python_pkg

instead of

/path/to/src/my_python_pkg/src/my_python_pkg

AFAIK there is no built-in way of finding back the actual source folder of a Python package from devel-space. What might work is to:

  • import the module and check the value of <module>.__path__, where the source-folder has been added to by catkin's init.py.in)
  • search for a line __extended_path = '...' in <module>.__file__
  • lookup the package path using rospack find, relying on the fact that the Python and ROS package names are typically identical by convention
  • create a marker file pointing to the source path of the package in ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_PYTHON_DESTINATION} (the virtual package path in devel-space)

@meyerj
Copy link
Author

meyerj commented Nov 19, 2019

We still cannot launch a behavior from install-space using the approach described at Running Behaviors Without Operator without this patch. I assume the be_launcher script still tries to find Python sources in the ROS_PACKAGE_PATH because it uses rospkg internally.

Probably that is what has been fixed by @DorianScholz in the patches referenced in #56 (comment)?

In general I agree that patching flexbe_app and flexbe_onboard to not use the ROS_PACKAGE_PATH to locate package sources is much better than this symlink "hack".

@dcconner
Copy link
Member

@pschillinger What is the status of this issue in the ROS 1 at this point in time? Did this get resolved by other means?

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.

4 participants