Implementation of pluggable spaces. #458

mikepurvis · 2017-05-26T18:35:25Z

As discussed in mikepurvis/catkin_tools_document#3 (comment).

This permits plugins to register new first-class space types. Some potential candidate uses:

A docs space for stashing doxygen/sphinx output.
A tests space for placing junit testing reports.
A merge space for placing post-build merged installspaces assembled from workspaces built with the --install-isolated option.
A symbols space for placing debug symbols stripped from binaries during/after the the build.

I haven't yet updated the catkin_clean verb to take advantage of this, but will do so shortly.

Test failure is due to a breakage upstream with the Sphinx spellcheck. I'd suggest either turning off the spellcheck for now, or locking into an older version of Sphinx where the problem didn't happen.

mikepurvis · 2017-05-26T18:37:47Z

setup.py

+            'devel = catkin_tools.spaces.devel:description',
+            'install = catkin_tools.spaces.install:description',
+            'log = catkin_tools.spaces.log:description',
+            'source = catkin_tools.spaces.source:description',


If desired, these could be collapsed into a single file, so:

'build = catkin_tools.spaces:build', 'devel = catkin_tools.spaces:devel', 'install = catkin_tools.spaces:install', 'log = catkin_tools.spaces:log', 'source = catkin_tools.spaces:source',

This is fine I think.

Cool. Yes I think it makes sense to give them their own files, particularly if we're likely to see functions attached to these related to cleaning or other behaviours.

mikepurvis · 2017-05-27T02:45:06Z

Okay, the story for properly handling catkin clean is a bit of an interesting one. I'd be delighted for some input on this from the maintainers. Cleaning is really two distinct functions which have very little to do with each other:

clean_profile, which cleans at the space level. This one could I think be handled by a generic implementation which just removes the specified directory, though potentially there could be the option for the space to supply its own more sophisticated function that has other behaviours.
clean_packages, which cleans package-by-package using the functions specified in the build_type plugins. To support this properly I think would require an additional function provided by the space for "clean package X from your space". In some cases that simply may not be applicable, so the default implementation should probably be a no-op. But supplying at least a hook here would permit a catkin document; catkin clean foo sequence which properly removes documentation generated for one of the packages present.

Another interesting thing about this is the question of whether the existing create_catkin_clean_job and create_cmake_clean_job (which are almost identical) should be broken up and have the relevant common space-related cleanup functions moved into space-supplied clean-package functions.

catkin_tools/catkin_tools/jobs/catkin.py

Line 485 in 2442e1f

def create_catkin_clean_job(

catkin_tools/catkin_tools/jobs/cmake.py

Line 328 in 2442e1f

def create_cmake_clean_job(

I think in the short term I will focus on the clean_profile side of things as my immediate interest is getting support for catkin clean -y to do the right thing. But perhaps if there's a resolution on how to handle cleaning individual packages for plugin-supplied spaces, that can be tackled in a follow-on PR.

mikepurvis · 2017-06-01T17:22:18Z

@wjwwood Thoughts on the above discussion? IMO this PR is mergeable as-is, and the work to improve catkin_clean could be done as a follow-up. That might actually be desirable in terms of reducing the overall size of each change.

wjwwood

Generally looks good. I had a few comments and questions which should be addressed.

wjwwood · 2017-06-02T00:04:20Z

catkin_tools/argument_parsing.py

@@ -89,15 +89,15 @@ def add_cmake_and_make_and_catkin_make_args(parser):

    add = parser.add_mutually_exclusive_group().add_argument
    add('--make-args', metavar='ARG', dest='make_args', nargs='+', required=False, type=str, default=None,
-        help='Arbitrary arguments which are passes to make.'
+        help='Arbitrary arguments which are passes to make. '


Why the trailing space, also there is a typo in this help (not new), passes -> passed

The string gets concatenated with the string immediately below. Without the change, you get:

$ catkin build -h [ ... ] --make-args ARG [ARG ...] Arbitrary arguments which are passes to make.It collects all of following arguments until a "--" is read.

Will fix passes -> passed as well.

wjwwood · 2017-06-02T00:04:30Z

catkin_tools/argument_parsing.py

             'It collects all of following arguments until a "--" is read.')
    add('--no-make-args', dest='make_args', action='store_const', const=[], default=None,
        help='Pass no additional arguments to make (does not affect --catkin-make-args).')

    add = parser.add_mutually_exclusive_group().add_argument
    add('--catkin-make-args', metavar='ARG', dest='catkin_make_args',
        nargs='+', required=False, type=str, default=None,
-        help='Arbitrary arguments which are passes to make but only for catkin packages.'
+        help='Arbitrary arguments which are passes to make but only for catkin packages. '


Same question and typo, lol.

wjwwood · 2017-06-02T00:05:57Z

catkin_tools/context.py

+            setattr(self, '__%s_space_abs' % space, os.path.join(self.__workspace, value))
+
+        def space_exists(self):
+            "Returns true if the space exists"


Use """ here, unless you have a specific reason not to?

This is a C&P from the old source_space_exists function. Will update.

wjwwood · 2017-06-02T00:07:21Z

catkin_tools/context.py

+
+    @classmethod
+    def setup_space_keys(cls):
+        '''


I believe """ is the convention most used in this file, please use that rather than '''.

Fixed.

Potentially this could be called on import, but it felt saner to defer and do it lazily on first use.

wjwwood · 2017-06-02T00:14:29Z

catkin_tools/context.py

@@ -411,20 +446,20 @@ def summary(self, notes=[]):
                "--init`.")]
        if not self.source_space_exists():
            summary_warnings += [clr(
-                "Source space `@{yf}{_Context__source_space_abs}@|` does not yet exist.")]
+                "Source space `@{yf}{__source_space_abs}@|` does not yet exist.")]


Shouldn't this use the self.source_space_abs property? Why is the _Context prefix no longer needed, isn't it required due to the name mangling:

https://docs.python.org/2/tutorial/classes.html#private-variables-and-class-local-references

I would have though so, but this was what worked. It may be an indication that I'm not using property correctly in the setup of these generated properties. I shall investigate and report back.

Yeah, I'm not really sure what is going on here, but the behaviour is definitely correct in that each instance of Context has its own set of these properties, eg:

$ ~/env/bin/python Python 2.7.6 (default, Oct 26 2016, 20:30:19) [GCC 4.8.4] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> from catkin_tools.context import Context >>> Context.setup_space_keys() >>> c = Context('.') >>> c.source_space 'src' >>> d = Context('.') >>> d.source_space 'src' >>> c.source_space = 'foo' >>> c.source_space 'foo' >>> c.source_space_abs '/home/mikepurvis/test_ws/foo' >>> d.source_space 'src'

wjwwood · 2017-06-02T00:15:49Z

docs/mechanics.rst

@@ -235,7 +235,6 @@ Sourcing these setup scripts adds this workspace and any "underlaid" workspaces

 .. [1] GNU/Linux Only
 .. [2] Mac OS X Only
-.. [3] Windows Only


Was this unused, or...?

Yes, it was producing a Sphinx build warning.

wjwwood · 2017-06-02T00:16:02Z

setup.py

+            'devel = catkin_tools.spaces.devel:description',
+            'install = catkin_tools.spaces.install:description',
+            'log = catkin_tools.spaces.log:description',
+            'source = catkin_tools.spaces.source:description',


This is fine I think.

wjwwood · 2017-06-02T00:18:48Z

I'm in favor of merging this and dealing with the generalization of catkin clean afterwards. Honestly I didn't have much to do with catkin clean, so I probably have less of an idea of how to proceed there than you do. But everything you said makes sense with respect to updating clean_profile being straightforward, but clean_package needing to be generalized to support different approaches to cleaning a package out of a space.

mikepurvis · 2017-06-02T16:52:44Z

catkin_tools/context.py

-    DEFAULT_INSTALL_SPACE = 'install'
+    CATKIN_SPACES_GROUP = 'catkin_tools.spaces'
+
+    SPACES = {}


Given the usage patterns seen below, there might be a case to be made here for using a list rather than a dict, and creating a new Space class rather than just assigning the entry_point dict directly.

I don't think it much matters for now, so my inclination would be to leave it and add the extra layer as need demands.

mikepurvis commented May 26, 2017

View reviewed changes

mikepurvis force-pushed the pluggable-spaces branch 2 times, most recently from 50017fc to 20cbf3b Compare May 27, 2017 02:37

mikepurvis mentioned this pull request May 31, 2017

Disable Sphinx spell check for now. #461

Merged

Basic implementation of pluggable spaces.

4fd084d

mikepurvis force-pushed the pluggable-spaces branch from 20cbf3b to 4fd084d Compare May 31, 2017 18:29

wjwwood approved these changes Jun 2, 2017

View reviewed changes

Review fixups.

561182c

mikepurvis commented Jun 2, 2017

View reviewed changes

wjwwood approved these changes Jun 2, 2017

View reviewed changes

wjwwood merged commit c60687f into catkin:master Jun 2, 2017

mikepurvis deleted the pluggable-spaces branch October 24, 2017 19:43

mikepurvis mentioned this pull request Mar 13, 2018

Use pluggable spaces to define docs space mikepurvis/catkin_tools_document#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of pluggable spaces. #458

Implementation of pluggable spaces. #458

mikepurvis commented May 26, 2017 •

edited

mikepurvis May 26, 2017 •

edited

wjwwood Jun 2, 2017

mikepurvis Jun 2, 2017

mikepurvis commented May 27, 2017 •

edited

mikepurvis commented Jun 1, 2017

wjwwood left a comment

wjwwood Jun 2, 2017

mikepurvis Jun 2, 2017

wjwwood Jun 2, 2017

wjwwood Jun 2, 2017

mikepurvis Jun 2, 2017

wjwwood Jun 2, 2017

mikepurvis Jun 2, 2017

wjwwood Jun 2, 2017

mikepurvis Jun 2, 2017

mikepurvis Jun 2, 2017

wjwwood Jun 2, 2017

mikepurvis Jun 2, 2017

wjwwood Jun 2, 2017

wjwwood commented Jun 2, 2017

mikepurvis Jun 2, 2017

Implementation of pluggable spaces. #458

Implementation of pluggable spaces. #458

Conversation

mikepurvis commented May 26, 2017 • edited

mikepurvis May 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepurvis commented May 27, 2017 • edited

mikepurvis commented Jun 1, 2017

wjwwood left a 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

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

wjwwood commented Jun 2, 2017

Choose a reason for hiding this comment

mikepurvis commented May 26, 2017 •

edited

mikepurvis May 26, 2017 •

edited

mikepurvis commented May 27, 2017 •

edited