catkin tools plugin: add catkin tools support. #1593

cratliff · 2017-10-06T03:37:13Z

Adding suppot for catkin tools. Initially the only difference is
the build is being performed by catkin tools instead of catkin_make,
while all other function inherits from catkin_make.

LP: #1693380

Have you followed the guidelines for contributing?
Have you signed the CLA?
If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
If this is a new feature. Have you discussed the design on the forum?
Have you successfully run ./runtests.sh static?
Have you successfully run ./runtests.sh unit?

come-maiz

Thanks a lot for this @cratliff !
I just had a quick look and left a couple of nit comments, because I'll leave the full review to Kyle.

I think that we can refactor a base catkin plugin to make the inheritance clearer. One base, and then two implementations, one with catkin_make_isolated and the other with catkin. But your code looks very nice, we can do the refactor afterwards.

However, I'm not entirely convinced that this should be a separate plugin, instead of a single catkin plugin with an attribute like build-tool. But I will just run away and leave that discussion to the two of you :)

come-maiz · 2017-10-07T04:28:49Z

snapcraft/plugins/catkin_tools.py

@@ -0,0 +1,124 @@
+# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
+#
+# Copyright (C) 2015-2016 Canonical Ltd


please update the copyright year here.

come-maiz · 2017-10-07T04:38:49Z

snapcraft/plugins/catkin_tools.py

+        super().__init__(name, options, project)
+        self.build_packages.extend(['python-catkin-tools'])
+
+    def _prepare_build(self):


I feel that this method is too long. Maybe it can be split into _clean_catkin, _add_catkin_profile and _install_catkin_packages

cratliff · 2017-10-11T04:58:39Z

I edited the copyright and broke up the _prepare_build function to be to be a shorter.

That refactor makes sense. I would be fine to do it now or later.

@kyrofa and I talked about that at the rally. He had some thoughts about why this should be another plugin. One I know of is that catkin tools has a number of options that may be added later at the plugin is expanded that wouldn't be compatible with catkin.

kyrofa

This is looking pretty good. I'm not a catkin tools pro, so I have a few questions in here. Also, have you tried it with content sharing at all? Finally, we will definitely need a snapd integration test for this.

kyrofa · 2017-10-13T15:09:37Z

snapcraft/plugins/catkin_tools.py

+    def _add_catkin_profile(self):
+        # Overwrite the default catkin profile to ensure builds
+        # aren't affected by profile changes.
+        catkincmd = ['catkin', 'profile', 'add', '-f', 'default']


Is there any downside to potentially overwriting this profile? Is it possible for the developer to create one with settings that are necessary for the build to succeed, and our overwriting it here causes it to fail? Likewise, I'm sure it's possible for their changes to break the build in the snap... perhaps that's more likely. Just want to get your thoughts on this.

As a user I would probably prefer to be able to use in my own profile. It would mean the catkin tools plugin would be less maintenance as well, since almost anything you would want to add can be done directly through a profile. My thought was that a user's settings could break the build in the snap if they were utilizing some features, use source not in the parts directory, or not install properly. Would a field in the schema be appropriate to allow the user to use their profile understanding it could break things? Even if we allow the user's profile we would still want to change the source, build, and install space.

Your logic seems sound, here. Indeed, the schema seems a good place for this, but I think what we have here is a good and safe first step. Anything more could come as a follow-up.

kyrofa · 2017-10-13T15:11:03Z

snapcraft/plugins/catkin.py

@@ -401,7 +401,7 @@ def pull(self):

        # Pull our own compilers so we use ones that match up with the version
        # of ROS we're using.
-        compilers = _Compilers(
+        compilers = Compilers(


It's on my TODO to split this out into a common area, FYI.

kyrofa · 2017-10-13T15:21:38Z

snapcraft/plugins/catkin_tools.py

+        catkincmd = ['catkin', 'config']
+
+        # Use the newly created default profile.
+        catkincmd.extend(['--profile', 'default'])


Why not set this profile to be active in _add_catkin_profile()? It would save your needing to explicitly use it in every subsequent command, no?

Because there was only one subsequent command, I'm good to change that.

No need, just a thought.

kyrofa · 2017-10-13T15:24:03Z

snapcraft/plugins/catkin_tools.py

+        catkincmd = ['catkin', 'build']
+
+        # Prevent notification that the build is complete.
+        catkincmd.append('--no-notify')


Does this explode on headless systems? Might be kinda useful, really.

Not that I'm aware of. My thought was that it could be confusing to receive a notification that your build is complete when you still need to go through the stage, prime, and snap steps of the build.

Yeah that's a good point.

kyrofa · 2017-10-13T15:29:53Z

snapcraft/plugins/catkin_tools.py

+            self._compilers_path, self.PLUGIN_STAGE_SOURCES, self.project)
+
+        # 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).


And this bug still bites catkin tools? What about this bug from of old?

kyrofa · 2017-10-13T15:30:12Z

snapcraft/tests/commands/test_list_plugins.py

+        'make   nil                python   qmake  scons      \n'
+        'autotools  cmake         go      gulp    kbuild   '
+        'maven  nodejs             python2  ruby   tar-content\n'
+        'catkin     copy          godeps  jdk     kernel   meson  '


Haha, this is always so much fun to edit, right?

kyrofa · 2017-10-13T15:46:41Z

However, I'm not entirely convinced that this should be a separate plugin, instead of a single catkin plugin with an attribute like build-tool. But I will just run away and leave that discussion to the two of you :)

@kyrofa and I talked about that at the rally. He had some thoughts about why this should be another plugin. One I know of is that catkin tools has a number of options that may be added later at the plugin is expanded that wouldn't be compatible with catkin.

I don't have super strong opinions here, but let me lay out all my thoughts.

The Catkin plugin is stable, and it uses the build tool recommended by ROS.
Catkin Tools is still in beta, and as such could break API.
Now in ROS2 they're moving to ament, so I'm not sure how long work will continue to go into Catkin Tools.
As @cratliff mentions, Catkin Tools is a much more capability-rich tool than Catkin. I feel that combining them in such a way that both Catkin and Catkin Tools are supported would make for a very complex plugin over time as we try to expose various capabilities. And man... it's already complex.
Having a Catkin Tools plugin means we can label it as a beta plugin.

Long-term, if development continues on Catkin Tools and we don't see breakage between Catkin and Catkin Tools, I envision eventually replacing the Catkin plugin with this. We just need to ensure this plugin continues to be a superset of capabilities from the Catkin plugin, and they don't diverge.

kyrofa · 2017-10-13T15:47:57Z

snapcraft/plugins/catkin_tools.py

+class CatkinToolsPlugin(snapcraft.plugins.catkin.CatkinPlugin):
+    def __init__(self, name, options, project):
+        super().__init__(name, options, project)
+        self.build_packages.extend(['python-catkin-tools'])


Could you please add a note that this is a beta plugin? See the ruby plugin for reference.

cratliff · 2017-10-14T16:33:51Z

This is looking pretty good. I'm not a catkin tools pro, so I have a few questions in here. Also, have you tried it with content sharing at all? Finally, we will definitely need a snapd integration test for this.

No, by content sharing do you mean an overlay or underlay of the workspaces?

Long-term, if development continues on Catkin Tools and we don't see breakage between Catkin and Catkin Tools, I envision eventually replacing the Catkin plugin with this. We just need to ensure this plugin continues to be a superset of capabilities from the Catkin plugin, and they don't diverge.

My initial transition to catkin tools required cleaning the build from catkin_make then using catkin tools. Transitioning should be simple enough.

kyrofa · 2017-10-19T17:13:06Z

No, by content sharing do you mean an overlay or underlay of the workspaces?

Right, take a look at demos/shared-ros/. I'd like to see if that continues to work with catkin-tools. Note that this is not a blocker for this PR, but something we'll need to ensure works before stabilizing this plugin.

kyrofa · 2017-10-24T17:35:14Z

I'm mostly +1 on this. I'm curious about the bash bug and I'd like it to be marked a beta plugin, but other than that you just have a conflict to fix.

cratliff · 2017-10-24T17:47:25Z

I tested the bash bug and am seeing it. I also tested content sharing, it did not work, but didn't get a chance to look into why. Since we had been talking about them I also tested the profile overwriting and cleaning a more. They might not be necessary and just me being overcautious. I'll make sure to add the beta plugin.

kyrofa · 2017-10-24T18:19:07Z

I tested the bash bug and am seeing it.

So it's not actually fixed in catkin tools? Bah.

I also tested content sharing, it did not work, but didn't get a chance to look into why.

That's okay, we can figure it out after this lands.

Adding suppot for catkin tools. Initially the only difference is the build is being performed by catkin tools instead of catkin_make, while all other function inherits from catkin_make. LP: #1693380

cratliff · 2017-10-25T04:51:13Z

Added the beta warning and fixed the conflict.

kyrofa · 2017-10-26T21:20:24Z

snapcraft/plugins/catkin_tools.py

+        self.build_packages.extend(['python-catkin-tools'])
+
+        # Beta Warning
+        # Remove this comment and warning once ruby plugin is stable.


Check the comment here-- still says "ruby" 😛 .

kyrofa

There's a typo in the beta warning comment, but overall +1 from me.

kyrofa · 2017-11-14T00:47:51Z

Sorry for the delay @cratliff, our testing infra fell over and we're trying to get a release out the door. As soon as that happens stuff can start landing again.

Adding suppot for catkin tools. Initially the only difference is the build is being performed by catkin tools instead of catkin_make, while all other function inherits from catkin_make. LP: #1693380

come-maiz reviewed Oct 7, 2017

View reviewed changes

sergiusens added this to Robotics in 17.10 Oct 7, 2017

cratliff force-pushed the catkin-tools-plugin branch 2 times, most recently from 5821d1d to c466ae1 Compare October 11, 2017 00:27

kyrofa reviewed Oct 13, 2017

View reviewed changes

cratliff added 3 commits October 25, 2017 00:45

catkin tools plugin: add catkin tools support.

9a87571

Adding suppot for catkin tools. Initially the only difference is the build is being performed by catkin tools instead of catkin_make, while all other function inherits from catkin_make. LP: #1693380

clean up catkin tools

1be5a92

add beta warning

9b0eee0

cratliff force-pushed the catkin-tools-plugin branch from c466ae1 to 9b0eee0 Compare October 25, 2017 04:48

sergiusens added this to the 2.36 milestone Oct 26, 2017

kyrofa reviewed Oct 26, 2017

View reviewed changes

kyrofa approved these changes Oct 26, 2017

View reviewed changes

cratliff and others added 2 commits October 26, 2017 17:29

fix typo

62fcda8

Merge branch 'master' into catkin-tools-plugin

8f20437

Kyle Fazzari and others added 4 commits November 16, 2017 09:15

Merge branch 'master' into catkin-tools-plugin

1874326

Merge remote-tracking branch 'origin/master' into catkin-tools-plugin

4bec236

add the two missing tests

c784522

add-tools-test

0aa0a29

sergiusens merged commit 7e31fa0 into canonical:master Nov 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

catkin tools plugin: add catkin tools support. #1593

catkin tools plugin: add catkin tools support. #1593

cratliff commented Oct 6, 2017 •

edited by kyrofa

come-maiz left a comment

come-maiz Oct 7, 2017

come-maiz Oct 7, 2017

cratliff commented Oct 11, 2017

kyrofa left a comment

kyrofa Oct 13, 2017

cratliff Oct 14, 2017 •

edited

kyrofa Oct 24, 2017

kyrofa Oct 13, 2017

kyrofa Oct 13, 2017

cratliff Oct 14, 2017

kyrofa Oct 24, 2017

kyrofa Oct 13, 2017

cratliff Oct 14, 2017

kyrofa Oct 24, 2017

kyrofa Oct 13, 2017

kyrofa Oct 24, 2017

kyrofa Oct 13, 2017 •

edited

kyrofa commented Oct 13, 2017

kyrofa Oct 13, 2017

kyrofa Oct 24, 2017

cratliff commented Oct 14, 2017

kyrofa commented Oct 19, 2017 •

edited

kyrofa commented Oct 24, 2017

cratliff commented Oct 24, 2017

kyrofa commented Oct 24, 2017

cratliff commented Oct 25, 2017

kyrofa Oct 26, 2017

kyrofa left a comment

kyrofa commented Nov 14, 2017

catkin tools plugin: add catkin tools support. #1593

catkin tools plugin: add catkin tools support. #1593

Conversation

cratliff commented Oct 6, 2017 • edited by kyrofa

come-maiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cratliff commented Oct 11, 2017

kyrofa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cratliff Oct 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Oct 13, 2017 • edited

Choose a reason for hiding this comment

kyrofa commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cratliff commented Oct 14, 2017

kyrofa commented Oct 19, 2017 • edited

kyrofa commented Oct 24, 2017

cratliff commented Oct 24, 2017

kyrofa commented Oct 24, 2017

cratliff commented Oct 25, 2017

Choose a reason for hiding this comment

kyrofa left a comment

Choose a reason for hiding this comment

kyrofa commented Nov 14, 2017

cratliff commented Oct 6, 2017 •

edited by kyrofa

cratliff Oct 14, 2017 •

edited

kyrofa Oct 13, 2017 •

edited

kyrofa commented Oct 19, 2017 •

edited