/snapcraft

Catkin plugin: Add support for ROS tools. #152

Merged
merged 1 commit into from Dec 7, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@kyrofa @sergiusens @elopio @ted-gould
@kyrofa
Member

kyrofa commented Dec 4, 2015

The current Catkin plugin distributes only the built version of a ROS package that would typically go into the devel workspace, but that version doesn't include non-source, non-binary files, such as .launch files. This PR changes the plugin to actually install ROS packages, thereby including any non-source, non-binary files specified by the developer.

This PR also updates the wrapper environment to correctly setup the ROS environment, which allows for the use of typical ROS tools within the snapcraft.yaml, such as roslaunch, rosrun, etc., and updates the ROS examples to make use of them.

Finally, this PR also increases the test coverage of the Catkin plugin twofold.
examples/ros/src/beginner_tutorials/CMakeLists.txt
+ LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}
+ RUNTIME DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION}
+)
@sergiusens

sergiusens Dec 4, 2015

Collaborator

💯

snapcraft/plugins/catkin.py
@@ -105,6 +105,7 @@ def env(self, root):
root,
self.options.rosversion),
'ROS_MASTER_URI=http://localhost:11311',
@sergiusens

sergiusens Dec 4, 2015

Collaborator

Not part of the PR, just something to keep in mind.

@kyrofa

kyrofa Dec 4, 2015

Member

Heh, I don't quite understand what you're trying to draw my attention to. That you'd like that variable removed eventually?

@sergiusens

sergiusens Dec 4, 2015

Collaborator

That maybe it eventually is expected to run on a different port. But I'm not sure that is configurable.

@kyrofa

kyrofa Dec 4, 2015

Member

Ah, okay. It is actually configurable, I just need to figure out how to do that with Snappy, which I'll make a different PR.

snapcraft/tests/test_plugin_catkin.py
+ plugin = catkin.CatkinPlugin('test-part', self.properties)
+
+ with mock.patch.object(builtins, 'open',
+ mock.mock_open(read_data=package_xml)):
@elopio

elopio Dec 4, 2015

Member

Could you remove this mock by writting the xml file to the sourcedir?

@kyrofa

kyrofa Dec 4, 2015

Member

Indeed I could, but I feel like actually hitting the filesystem when I don't need to feels a bit brittle and slow.

@elopio

elopio Dec 4, 2015

Member

In unit tests, I prefer to access the filesystem in temp directories instead of mocking that part.
But your test is correct, if you prefer it this way, I'm ok with that.

@kyrofa

kyrofa Dec 4, 2015

Member

I don't feel strongly about this. I'll update it :) .

@kyrofa

kyrofa Dec 4, 2015

Member

Done.

snapcraft/tests/test_plugin_catkin.py
+ 'ros-foo-buildtool-depend',
+ 'ros-foo-build-depend',
+ 'ros-foo-run-depend']),
+ mock.call().unpack(plugin.installdir)])
@elopio

elopio Dec 4, 2015

Member

This looks good.

snapcraft/plugins/catkin.py
@@ -105,6 +105,7 @@ def env(self, root):
root,
self.options.rosversion),
'ROS_MASTER_URI=http://localhost:11311',
+ 'ROS_HOME=$SNAP_APP_USER_DATA_PATH/.ros',
@sergiusens

sergiusens Dec 4, 2015

Collaborator

To avoid future misgivings, can we add a comment on why this is needed?

@kyrofa

kyrofa Dec 4, 2015

Member

Good call, done.

snapcraft/plugins/catkin.py
- 'opt/ros/' + self.options.rosversion + '/.rosinstall',
- 'opt/ros/' + self.options.rosversion + '/setup.sh',
- 'opt/ros/' + self.options.rosversion + '/_setup_util.py'],
+ 'opt/ros/' + self.options.rosversion + '/.rosinstall'],
@sergiusens

sergiusens Dec 4, 2015

Collaborator 
os.remove(os.path.join(self.installdir, self.options.rosversion, '.rosinstall'))
@kyrofa

kyrofa Dec 4, 2015

Member

Haha, alright I'll clean that up now.

@elopio

elopio Dec 4, 2015

Member

This deserves an integration test, to check that the files you want end up in the right dirs.
Feel free to add that in a later branch, because that's debt from before.

@sergiusens

sergiusens Dec 4, 2015

Collaborator

Oh, and if you know why it is being removed, a comment would be great 😄

@kyrofa

kyrofa Dec 4, 2015

Member

@elopio agreed 100%. I'll do it in a later PR though.

@ted-gould

ted-gould Dec 4, 2015

Contributor

It shouldn't need to be removed, but can be filtered now that the filters are fixed. Never got around to replacing the rm with proper stage filters.

@kyrofa

kyrofa Dec 4, 2015

Member

I got rid of all the removals here, none of them seemed to be necessary.

snapcraft/plugins/catkin.py
cwd=self.installdir)
+ # Fix the shebang in _setup_util.py to be portable
@elopio

elopio Dec 4, 2015

Member

It would be nice to have a unittest for the shebang.
Aren't we doing this already in the python plugin? Maybe we can share some code, and unit test that.

@sergiusens

sergiusens Dec 4, 2015

Collaborator

It's been moved to meta. If we want to share it I'd move it to another module.

@kyrofa

kyrofa Dec 4, 2015

Member

I agree, but ALL the build steps here need test coverage, which I plan on adding in a later PR.

@sergiusens
Collaborator

sergiusens commented Dec 4, 2015

I like this very much. One followup question is; do we need the ros plugin at all? I always found it redundant as maybe the core can be pulled in through some logic here.
@kyrofa
Member

kyrofa commented Dec 4, 2015

@sergiusens I was wondering that myself. Let me experiment, this PR may have enabled the removal of it.
@kyrofa
Member

kyrofa commented Dec 4, 2015

Alright I believe I've resolved all the concerns for this one. I will continue to increase test coverage throughout subsequent PRs, and will remove the roscore plugin once the blocking bug has been resolved.
@elopio
Member

elopio commented Dec 4, 2015

The example fails to build with:
Error: parts catkin-tutorials and roscore have the following file paths in common which have different contents:

usr/bin/xml2-config
@kyrofa
Member

kyrofa commented Dec 5, 2015

@elopio oh interesting, I didn't consider the interplay between plugins! I will add that to my manual test procedure. xml2-config isn't a required file, so I have temporarily added the removal of it back to the catkin plugin until we can remove roscore.
@kyrofa
Catkin plugin: Add support for ROS tools. 
The current Catkin plugin distributes only the built version of
a ROS package that would typically go into the devel workspace,
but that version doesn't include non-source, non-binary files,
such as .launch files. This commit changes the plugin to actually
install ROS packages, thereby including any non-source, non-binary
files specified by the developer.

This commit also updates the wrapper environment to correctly
setup the ROS environment, which allows for the use of typical
ROS tools within the snapcraft.yaml, such as roslaunch, rosrun,
etc., and updates the ROS examples to make use of them.

This commit also increases the test coverage of the Catkin plugin
twofold.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.3%) to 95.772%
Details
edd1367
@sergiusens
Collaborator

sergiusens commented Dec 6, 2015

@elopio and @kyrofa fwiw, plugins aren't supposed to solve all the conflicts in the world, that is to some extent part of the part author's responsibility. That does no rule out plugins themselves should try to deliver the cleanest environment possible, but not in detriment of its functionality.

In the example of xml-config, if I as an author ever create another part that also ships xml-config I would need to solve if somehow, luckily, that is somewhat what the stage, snap and organize keywords are for.
@sergiusens
Collaborator

sergiusens commented Dec 6, 2015

I'm 👍 on this branch btw 😉
@elopio
Member

elopio commented Dec 7, 2015

Me too. Merging.

elopio added a commit that referenced this pull request Dec 7, 2015

@elopio
Merge pull request #152 from kyrofa/catkin_support_roslaunch 
Catkin plugin: Add support for ROS tools.
fc51421

@elopio elopio merged commit fc51421 into snapcore:master Dec 7, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 95.772%
Details

@kyrofa kyrofa deleted the kyrofa:catkin_support_roslaunch branch Dec 7, 2015

@kyrofa kyrofa referenced this pull request Dec 21, 2015

Merged

Backport to 1.x: Catkin plugin: Add support for ROS tools. #180

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

@elopio
Merge pull request #152 from kyrofa/catkin_support_roslaunch 
Catkin plugin: Add support for ROS tools.
069b6cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment