Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Catkin plugin: use rosdep for dependency resolution #163
Conversation
sergiusens
reviewed
Dec 11, 2015
| - def pull(self): | ||
| - super().pull() | ||
| - self._setup_deb_packages() | ||
| + source_subdir = getattr(self.options, 'source_subdir', None) |
sergiusens
Dec 11, 2015
Collaborator
this should always be defined in the schema (as in it will only fail due to a programmatic error) so there is no need to getattr
kyrofa
Dec 15, 2015
Member
It doesn't seem to be defined here in __init__() (particularly when unit testing), so getattr seems necessary unless I should make sure all tests explicitly pass it in.
sergiusens
Dec 15, 2015
Collaborator
Right, the options class used in the tests needs to have the attrib. If testing from the plugin handler itself, this should be created with the defaults.
sergiusens
reviewed
Dec 11, 2015
| + 'Did you forget to add it to catkin-packages? If ' | ||
| + 'not, add the Ubuntu package containing it to ' | ||
| + 'stage-packages until you can get it into the ' | ||
| + 'rosdep database.'.format(dependency)) |
|
to be continued |
elopio
reviewed
Dec 14, 2015
| - tree = lxml.etree.parse(f) | ||
| + def _find_dependencies(self): | ||
| + if self.package_deps_found: | ||
| + return |
elopio
Dec 14, 2015
Member
It would be good to explain this in a comment, because it's weird. What are the cases that makes us call find_dependencies twice?
kyrofa
Dec 14, 2015
Member
Great question: I'm not sure (that was already in there). However, I believe it's because it's called from both pull() and build() (since both steps need to know it), but it doesn't want to find deps twice in case it's being run via snapcraft instead of snapcraft <step>.
elopio
reviewed
Dec 14, 2015
| - def _deps_from_packagesxml(self, f, pkg): | ||
| - tree = lxml.etree.parse(f) | ||
| + def _find_dependencies(self): |
elopio
Dec 14, 2015
Member
I would find this clearer if it is a get function that returns the dependencies, instead of a method with no return value that has a side effect of changing the dependencies attribute. Would that be hard to change?
kyrofa
Dec 14, 2015
Member
I don't believe so, although not saving it as an attribute would mean I'd have to interrogate the sources for their dependencies in both pull() and build(). Depending on the size of the workspace that could be a bit heavy.
kyrofa
Dec 14, 2015
Member
I might be able to refactor the build step to not need these though... I'll get back to you.
elopio
Dec 14, 2015
Member
ok. And if you need the sideeffect for efficiency, maybe split it in two: one _get, and one _update that calls the _get. Or if it makes a bigger mess, just add a docstring to what you have explaining the side effect.
elopio
reviewed
Dec 14, 2015
| - self._deb_packages.append( | ||
| - 'ros-'+self.options.rosversion+'-'+dep.replace('_', '-')) | ||
| + for dependency in dependencies: | ||
| + if not dependency: |
elopio
Dec 14, 2015
Member
Why would get_dependencies return an invalid dependency? It seems better to make sure that it returns a clean list of valid deps than to check them on the caller.
kyrofa
Dec 14, 2015
Member
Yeah you're right, this should be checked in get_dependencies(). And to answer your question, it shouldn't, but since I'm parsing stdout I'm trying to account for small changes in the output of rosdep (e.g. an extra line here and there).
elopio
reviewed
Dec 14, 2015
| + # not a dependency of the project, and we don't want to bloat the .snap | ||
| + # more than necessary. So we'll unpack it somewhere else, and use it | ||
| + # from there. | ||
| + logger.info("Fetching rosdep...") |
kyrofa
Dec 14, 2015
Member
Ah, no thanks! I've been trying to adhere but I knew I was going to miss some, haha :) .
elopio
reviewed
Dec 14, 2015
| + ubuntu.get(['python-rosdep']) | ||
| + ubuntu.unpack(self._rosdep_install_path) | ||
| + | ||
| + logger.info("Initializing rosdep database...") |
elopio
reviewed
Dec 14, 2015
| + raise RuntimeError( | ||
| + 'Error initializing rosdep database: {}'.format(e.output)) | ||
| + | ||
| + logger.info("Updating rosdep database...") |
|
Unless there are more comments, I've either fixed the concerns here or will address them in a PR I'll make right after this one is merged. |
elopio
reviewed
Dec 17, 2015
| + rosdep._rosdep_install_path, 'usr', 'lib', 'python2.7', | ||
| + 'dist-packages') and | ||
| + env['ROSDEP_SOURCE_PATH'] == rosdep._rosdep_sources_path | ||
| + and env['ROS_HOME'] == rosdep._rosdep_cache_path and |
elopio
Dec 17, 2015
Member
the and operator should be at the end of the line. flake8 complaints in xenial, it seems to ignore it in the travis's version.
elopio
reviewed
Dec 17, 2015
| - self.assertFalse(self.ubuntu_mock.called, | ||
| - "Ubuntu packages were unexpectedly pulled down") | ||
| + # Verify that rosdep was setup | ||
| + self.rosdep_mock.assert_has_calls([ |
elopio
reviewed
Dec 17, 2015
| + plugin.pull() | ||
| + except FileNotFoundError: | ||
| + self.fail('Unexpectedly raised an exception when the Catkin ' | ||
| + 'workspace was valid') |
elopio
Dec 17, 2015
Member
I generally just leave the exception make the test fail, with a comment above the step that can raise the exception. The exception should say something useful already to understand what went wrong, otherwise the exception is not very good.
If you prefer the try catch, that's good too. Just not common in the projects I've worked.
kyrofa
Dec 17, 2015
Member
Normally I agree with you, but I'm explicitly making sure this code doesn't die if I know it can't find the file. I can make the failure message much more friendly than trying to navigate a traceback. There's also something to be said for making the tests fail if what I'm testing for doesn't pass rather than error. If an exception was raised as a result of some setup code then I feel like that's a valid error, but if it's exactly what I'm testing for I prefer to catch it and turn it into a failure.
elopio
reviewed
Dec 17, 2015
| + self.assertEqual(raised.exception.args[0], |
elopio
Dec 17, 2015
Member
I think it's better to check
str(raised.exception)
instead of
raises.exception.args[0]
sergiusens
Dec 17, 2015
Collaborator
Do you prefer str(raised.exception) over raised.exception.__str__() ?
Just asking since other parts of the code base use the latter in tests.
kyrofa
Dec 17, 2015
Member
Ah, hey @sergiusens I missed your comment here. I feel like the former is better-looking, but I'm happy doing either.
elopio
reviewed
Dec 17, 2015
| + plugin.pull() | ||
| + | ||
| + self.assertEqual( | ||
| + raised.exception.args[0], |
elopio
reviewed
Dec 17, 2015
| + self.addCleanup(patcher.stop) | ||
| + | ||
| + patcher = mock.patch('subprocess.check_output') | ||
| + self.run_mock = patcher.start() |
elopio
Dec 17, 2015
Member
I would understand the tests easier if the mock is called check_output_mock.
|
@kyrofa sorry it took so long. |
|
@elopio excellent review, thank you! PITA comments are appreciated ;) . We'll just wait for @sergiusens to give his OK here, then. |
kyrofa
added
the
enhancement
label
Dec 17, 2015
sergiusens
reviewed
Dec 18, 2015
| self.package_deps_found = False | ||
| self.package_local_deps = {} | ||
| self._deb_packages = [] | ||
| + self.dependencies = [] |
|
Just one comment related to names; it is mostly a nitpick and from being away from the problem, but I'd like to see member names to be Preemptive |
|
@sergiusens yeah, good point. There's a bit of technical debt to be paid there, but I'll take a look at that in the refactor coming up. |
added a commit
that referenced
this pull request
Dec 18, 2015
kyrofa
merged commit 20b33e4
into
snapcore:master
Dec 18, 2015
kyrofa
deleted the
kyrofa:catkin_use_rosdep
branch
Dec 18, 2015
kyrofa
referenced this pull request
Dec 21, 2015
Merged
Backport to 1.x: Catkin plugin: use rosdep for dependency resolution. #183
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
kyrofa commentedDec 11, 2015
The Catkin plugin currently does its own dependency resolution via manually parsing each package.xml. It makes some naive assumptions while doing so, and a tool already exists in ROS for doing this: rosdep. This PR gets rid of the manual parsing in favor of rosdep.