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: Refactor build. #175
Conversation
sergiusens
reviewed
Dec 18, 2015
| @@ -25,6 +25,9 @@ | ||
| - catkin-packages: | ||
| (list of strings) | ||
| List of catkin packages to build. | ||
| + - source-space: |
sergiusens
Dec 18, 2015
Collaborator
Is source-space the word used upstream, if not, it feels weird. Maybe just workspace?
kyrofa
Dec 18, 2015
Member
Yeah, line 262. Workspace would be confusing since that's what source/source-subdir should be pointing to (the catkin workspace). Catkin has a "build space," a "devel space," an "install space," and a "source space." And a workspace
sergiusens
reviewed
Dec 18, 2015
| + of `source` and `source-subdir` keys) needs to point to a valid Catkin | ||
| + workspace containing another subdirectory called the "source space." By | ||
| + default, this is a directory named "src," but it can be remapped via | ||
| + the `source-space` key. It's important that the source space is not the |
sergiusens
reviewed
Dec 18, 2015
| schema['required'].append('catkin-packages') | ||
| return schema | ||
| def __init__(self, name, options): | ||
| - super().__init__(name, options) | ||
| + """Sanity-check the plugin config regarding source code paths. |
sergiusens
Dec 18, 2015
Collaborator
__init__ is much more than this. I am not entirely sure this is the best one liner docstring
kyrofa
Dec 18, 2015
Member
Is it though? Even the super class only sets a few variables in preparation for more work... doesn't it?
sergiusens
Dec 18, 2015
Collaborator
I have no quarrel with what it does, just the docstring, not sure you noticed, but __init__ above is also a link
sergiusens
reviewed
Dec 18, 2015
| + | ||
| + Catkin packages can specify their system dependencies in their | ||
| + package.xml. In order to support that, the Catkin packages are | ||
| + interrogated for their dependencies here. Since "stage-packages" are |
sergiusens
Dec 18, 2015
Collaborator
We might to check for consistency on use of "" versus `` and document it.
sergiusens
reviewed
Dec 18, 2015
| @@ -252,71 +229,55 @@ def build(self): | ||
| '{}', ';' | ||
| ]) |
sergiusens
Dec 18, 2015
Collaborator
no incentive to change this as well
Nothing that can't be done with os.walk and re or str().replace
sergiusens
reviewed
Dec 18, 2015
| # Don't clutter the real ROS workspace-- use the Snapcraft build | ||
| # directory | ||
| catkincmd.extend(['--directory', self.builddir]) | ||
| + # Account for a non-default source space by always specifying it | ||
| + catkincmd.extend(['--source-space', os.path.join( |
sergiusens
reviewed
Dec 18, 2015
| + # This command must run in bash due to a bug in Catkin that causes it | ||
| + # to explode if there are spaces in the cmake args (which there are). | ||
| + # This has been fixed in Catkin Tools... perhaps we should be using | ||
| + # that instead. |
sergiusens
reviewed
Dec 18, 2015
| @@ -340,40 +352,41 @@ def __init__(self, ros_distro, ros_package_path, rosdep_path, | ||
| self._rosdep_cache_path = os.path.join(self._rosdep_path, 'cache') | ||
| def setup(self): | ||
| + # Make sure we can run multiple times without error, while leaving the | ||
| + # capability to re-initialize, by making sure we clear the sources. | ||
| + shutil.rmtree(self._rosdep_sources_path, ignore_errors=True) |
sergiusens
Dec 18, 2015
Collaborator
What is the intention of ignore_errors here? If it is to clean after use, maybe contextlib is better here.
This will ignore the errors even if only some files failed to remove, I'll make it your call though.
kyrofa
Dec 18, 2015
Member
Ah, good point. I threw that in there to make sure it was okay if the files didn't exist, but I should just be catching that exception. Good catch. Fixing.
sergiusens
reviewed
Dec 18, 2015
| - mock.call().setup()]) | ||
| + def test_schema(self): | ||
| + plugin = catkin.CatkinPlugin('test-part', self.properties) | ||
| + schema = plugin.schema() |
sergiusens
Dec 18, 2015
Collaborator
schema = catkin.CatkinPlugin.schema()as schema is a classmethod
sergiusens
reviewed
Dec 18, 2015
| # sourcedir is expected to be the root of the Catkin workspace. Since | ||
| - # source_subdir was specified to be the same as the root, this should | ||
| + # source_space was specified to be the same as the root, this should | ||
| # fail. | ||
| with self.assertRaises(RuntimeError) as raised: | ||
| plugin = catkin.CatkinPlugin('test-part', self.properties) | ||
| plugin.pull() |
sergiusens
Dec 18, 2015
Collaborator
this call is never reached. Might want to check the test coverage for this file to spot others and remove dead test code
kyrofa
Dec 18, 2015
Member
Oh this isn't here to DO anything. This test is saying "at least one of these lines had better throw an exception." I went back a forth a few times where I wanted the safety checks, so this makes the test less fragile to future changes.
sergiusens
Dec 18, 2015
Collaborator
You can make this a oneline then ;-)
catkin.CatkinPlugin('test-part', self.properties).pull()
|
Just had a couple nitpicks here and there. |
|
Thanks for the good review @sergiusens! I've pushed a new version that should address your concerns. |
|
Looks good, just adding one more comment about you have my |
|
@sergiusens Fixed both. You're right, the |
added a commit
that referenced
this pull request
Dec 19, 2015
kyrofa
merged commit 59434ff
into
snapcore:master
Dec 19, 2015
kyrofa
deleted the
kyrofa:catkin_refactor_build
branch
Dec 19, 2015
kyrofa
referenced this pull request
Dec 21, 2015
Merged
Backport to 1.x: Catkin plugin: Refactor build. #184
pushed a commit
to smoser/snapcraft
that referenced
this pull request
Sep 14, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
kyrofa commentedDec 18, 2015
This PR contains a number of small refactors:
env()to only contain variables for run-time, and document each of them.pull()-- forbuild()they should just be ready to go.build()to be less clever and more simple/verbose/robust.source-spacekeyword to get rid of custombuild().