Skip to content

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jun 4, 2021

This patch builds on top of #567, adding support for passing keyword arguments for CLI extensions along with their spec. This allow for extension specific configuration.

CI up rosidl_cli, rosidl_typesupport_c, rosidl_typesupport_cpp, and rosidl_generator_py:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Michel Hidalgo <[email protected]>
@hidmic
Copy link
Contributor Author

hidmic commented Jun 10, 2021

@clalancette @sloretz friendly ping

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I have enough context to review this. Two questions that might help clarify things a bit:

  1. How is this intended to be used in the end? That is, what is a realistic example of something that might get passed through here?
  2. Do we really have to define our own new string format for the SPECS_PATTERN? Can't we use straight YAML (or XML, or JSON, or....)?

@hidmic
Copy link
Contributor Author

hidmic commented Jun 14, 2021

@clalancette fair questions.

How is this intended to be used in the end? That is, what is a realistic example of something that might get passed through here?

It really depends on each generator implementation. A realistic example, the one that pushed me to do this, is narrowing down the pool of typesupport implementations to be used when generating CPython extensions and C/C++ typesupport trampolines. Simply looking up the ament_index assumes the corresponding code has been generated and built, which is true in the typical rosidl CMake pipeline (where presence in the index implies ament extension registration) but not when manually recreating a subset of it in a different build system :)

Do we really have to define our own new string format for the SPECS_PATTERN? Can't we use straight YAML (or XML, or JSON, or....)?

It is mostly YAML. We could go from py[key: value] to {name: py, kwargs: {}} plus py being a shortcut to {name: py}. I don't have a strong opinion.

@hidmic hidmic requested a review from clalancette June 18, 2021 19:18
@hidmic
Copy link
Contributor Author

hidmic commented Jun 22, 2021

@clalancette @sloretz friendly ping

@hidmic
Copy link
Contributor Author

hidmic commented Jul 12, 2021

Re-running CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor Author

hidmic commented Jul 12, 2021

Alright, thanks for the reviews! I'll get this in and release so we can have the Rolling PR job passing for downstream PRs.

@hidmic hidmic merged commit d6d548a into master Jul 12, 2021
@hidmic hidmic deleted the hidmic/cli-extension-kwargs branch July 12, 2021 17:10
@hidmic
Copy link
Contributor Author

hidmic commented Oct 29, 2021

@cottsay how do you feel about backporting this (along with ros2/rosidl_typesupport#112 and ros2/rosidl_python#133) to Galactic? It is a feature and it does change the expected extension API but it is hardly likely anyone has used it outside of the core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants