colcon / colcon-ros

Extension for colcon to support ROS packages
http://colcon.readthedocs.io
Apache License 2.0
13 stars 26 forks source link

support ros.meson packages #141

Closed christianrauch closed 2 years ago

christianrauch commented 2 years ago

Packages of plain type meson are already supported by colcon-meson. This adds support for meson packages with a package.xml, which are identified as ros.meson, by simply forwarding the ros.meson task extension to the meson task extension.

Without this, meson packages with a package.xml for binary dependency resolution will be ignored and the warning:

WARNING:colcon.colcon_core.verb:No task extension to 'build' a 'ros.meson' package

will be shown.

Note: This does not add support for building ROS nodes via meson. While finding and linking the core ROS libraries will probably be possible, none of the CMake macros have been ported.

I am not sure why there is even a distinction between cmake vs ros.cmake and meson vs ros.meson. I believe those ros. prefixed types could as well be handled via the colcon-cmake and colcon-meson extensions. The colcon_ros.task.cmake.build:CmakeBuildTask looks rather trivial.

Can someone explain why plain CMake packages with a package.xml have to be build by colcon-ros instead of colcon-cmake?

cottsay commented 2 years ago

This adds support for meson packages with a package.xml, which are identified as ros.meson, by simply forwarding the ros.meson task extension to the meson task extension.

There's no particular reason to do this in colcon-ros. colcon-meson can declare the entry point itself, and need not declare a dependency on colcon-ros either.

Can someone explain why plain CMake packages with a package.xml have to be build by colcon-ros instead of colcon-cmake?

I can see two main reasons, but there may be some historical reasons as well:

  1. ros.cmake packages implicitly get PKG_CONFIG_PATH hooks, where cmake packages don't appear to.
  2. Dependencies and other metadata sourced from a package.xml rather than a CMakeLists.txt carry implications that might be important to other extensions. For example, any dependencies listed for any ros.* package types will always be either other ros.* packages or rosdep keys. That isn't necessarily true of dependencies detected from a CMakeLists.txt.
christianrauch commented 2 years ago

There's no particular reason to do this in colcon-ros. colcon-meson can declare the entry point itself, and need not declare a dependency on colcon-ros either.

I tried to register a build task for meson and ros.meson via:

colcon_core.task.build =
    meson = colcon_meson.build:MesonBuildTask
    ros.meson = colcon_meson.build:MesonBuildTask

but this results in an error due to duplicated arguments for both MesonBuildTask instances:

argparse.ArgumentError: argument --meson-args: conflicting option string: --meson-args

This is because add_arguments gets called twice for the same parser and hence tries to register --meson-args twice.

Is there an example for declaring the same entry point for both types in the same package?

christianrauch commented 2 years ago

@cottsay Can you have a look at this and how it is supposed to work? If I cannot use the same entry point twice within one extension (colcon-meson), then I guess this has to be added to colcon-ros.

cottsay commented 2 years ago

...this results in an error due to duplicated arguments for both MesonBuildTask instances

I don't see how declaring the entry point here is going to change that. There's still going to be more than one entry point trying to add the same arguments.

This is probably one of the main reasons why the CMake tasks defined in colcon-ros don't inherit directly from colcon-cmake ones, but rather wrap them. You could probably do something similar in colcon-meson.

christianrauch commented 2 years ago

You are right. This problem happens in any case. I am still unclear about the inner workings of all of this. I created a "thin wrapper":

class RosMesonBuildTask(TaskExtensionPoint):
    def __init__(self):
        super().__init__()

    async def build(self):
        meson_extension = MesonBuildTask()
        meson_extension.set_context(context=self.context)
        rc = await meson_extension.build()

But now, the --meson-args is only recognised by the raw meson extension:

$ colcon build -h
[...]
Arguments for 'meson' packages:
  --meson-args [* ...]  Pass arguments to Meson projects.

Is this indirectly handled by the self.context? How do I forward the meson parameters to meson packages with and without the packages.xml.

cottsay commented 2 years ago

... the --meson-args is only recognised by the raw meson extension

I haven't dug very deep into this, but I believe that the build/test verbs are aware of the "destination" the arguments are bound for, and only add them if they match.

You might try importing the standalone build task class as another name and naming the wrapper class the same as the original standalone build task class name, as is done in the wrapper example that I linked.

christianrauch commented 2 years ago

I am not sure how this works, but --meson-args is indeed passed to ros.meson packages, although colcon build -h lists this only for "plain" meson packages.

I am closing this in favour of https://github.com/colcon/colcon-meson/pull/3. @cottsay I would be grateful, if you could have a look at this new PR.