Add rosidl generate CLI. #567

hidmic · 2021-01-28T18:34:35Z

Connected to #565. This patch puts the basic functionality for an extensible rosidl generate CLI in place. Largely inspired in ros2/ros2cli (minus troublesome dependencies like rclpy).

CI up to rosidl_cli:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

sloretz

Partial review

rosidl_cli/package.xml

rosidl_cli/rosidl_cli/common.py

sloretz · 2021-02-24T22:32:38Z

rosidl_cli/rosidl_cli/command/generate/helpers.py

+    This function assumes ROS interface definition files follow the typical
+    ``rosidl`` install space layout i.e. 'package_name/subfolder/interface.idl'.
+    """
+    return pathlib.Path(path).parents[1].name


Recommend documenting that it assumes path is a relative path starting from the package's install prefix

Hmm, it should work with absolute paths too. I do realize I should've resolved the path. See 3a75bcd.

sloretz · 2021-02-24T23:47:34Z

rosidl_cli/rosidl_cli/cli.py

+    return subparser
+
+
+def main(*, script_name='rosidl', argv=None, description=None, commands=None):


There's no need for arguments; justdef main(): is fine because console_scripts entry points are called with no arguments

This I took from ros2cli. I understand it's there to (cheaply) support reuse. I can drop it if you think it's not warranted.

I'd recommend removing them. See 4fa6682 for a suggested way of doing it.

See 1140a09.

rosidl_cli/rosidl_cli/cli.py

rosidl_cli/rosidl_cli/command/generate/__init__.py

sloretz · 2021-02-25T22:10:44Z

rosidl_cli/test/rosidl_cli/test_entry_points.py

+    with pytest.raises(ValueError):
+        normalize_entry_point_specs(['name?'])
+
+    specs = normalize_entry_point_specs(['c', 'cpp==0.1.0'])


What's the motivation for the version specification? IIUC this would mean two different packages could do

# First package entry_points={ 'rosidl_cli.command.generate.type_extensions': [ 'cpp = first_pkg.v1.CppExtension', ], # Second package entry_points={ 'rosidl_cli.command.generate.type_extensions': [ 'cpp = second_pkg.v2.CppExtension', ],

but why would this situation arise?

What's the motivation for the version specification?

It's there to:

Enforce tooling versioning to ensure API/ABI stability and simplify deprecation cycles

I envision packages pinning plugin versions to guarantee API/ABI stability.

IIUC this would mean two different packages could do

That would cause a warning at least, and an error at most. I do not expect the same extension to be provided by different Python distributions (i.e. packages), I expect source code generation to be attempted against different versions of the same Python distribution.

I envision packages pinning plugin versions to guarantee API/ABI stability.

Pin in what context? As in, someone has a package with message files and they want to be sure they're only generated with cpp==0.1.0? If they're releasing their package they can pin using the version_* attributes in the <*depend> tags of the package.xml.

As in, someone has a package with message files and they want to be sure they're only generated with cpp==0.1.0?

Precisely.

If they're releasing their package they can pin using the version_* attributes in the <*depend> tags of the package.xml.

Hmm, do build tools like colcon enforce them? What if there's no package.xml e.g. if source code generation is performed outside a ROS package? At any rate, it seems way simpler and cheaper to me to have plugin versions (that ultimately map to that of the package) than to overcome those limitations in ROS' package management system.

Hmm, do build tools like colcon enforce them? What if there's no package.xml e.g. if source code generation is performed outside a ROS package?

You're right that a version requirement in the package.xml AFAIK is ignored by colcon, but I don't see how that or generating messages inside or outside a ROS package would make a user choose to give a version specifier here. If the generator is installed from a deb/rpm and the packaged version doesn't meet a user's requirements, then they need to build the generator from source. If they're building the generator from source, then they get to pick the version by checking out the one they want to build.

Have a look at 4c39003 to see how much less code there is when the PEP 440 specifiers are removed.

If the generator is installed from a deb/rpm and the packaged version doesn't meet a user's requirements, then they need to build the generator from source. If they're building the generator from source, then they get to pick the version by checking out the one they want to build.

That's absolutely correct whenever you have full control over sources. But it quickly becomes unmanageable as you integrate third-party code or start building software layers on top.

Let's say you want to use some third-party packages: foo and bar. Both have the same dependencies at the same version. Both use interfaces from a baz_msgs package at its 1.0.0 version. And yet, it may not be possible to use them in your workspace, or even together at all. Which baz_msgs API/ABI do these packages depend on? Version 1.0.0 only tells half the story. That exact same baz_msgs package could exhibit completely different API/ABIs depending on the state of the workspace it was built in.

Today, we "ensure" consistency by resting on the assumption that no API/ABI breaks will (should?) occur within a ROS distro, and thus no API/ABI break will occur in code generated by released generators. In a sense, we're (implicitly) versioning packages by distro: package foo depends on package baz_msgs at version 1.0.0 in Foxy, but breaks for version 1.0.0 in Rolling because a relevant generator had a major version bump.

IMHO spooky actions at a distance like these are brittle (and break semver if I'm not mistaken).

I don't see how that or generating messages inside or outside a ROS package would make a user choose to give a version specifier here.

By pinning a version specifier, you can make sure interfaces API/ABIs will stay the same (or fail to build with a self explanatory message) regardless of the state of workspace (and underlays).

An alternative solution would be to have each package that generates interfaces explicitly depend on the versions of the generators it uses. And drop all checks. It's less friendly for non-ROS packages consuming ROS, but it's an option.

As per offline discussion with @sloretz, the issue I'm trying to address is not exclusive to generators... A C++ package can see its public ABI changed if one of its dependencies changed its own -- all without modifying a single source file. Which kinda explains why our buildfarm has to use package versions, distro names, and build timestamps to ensure binaries come out OK.

It doesn't make sense to have an ad-hoc solution for the rosidl pipeline alone. I'll strip away version specifiers. Thanks for the discussion Shane !!

Still, we should have a discussion about this @ros2/team. It's not semantic versioning what we do, or at least it's not preventing the dependency hell it is meant to prevent. Relevant discussion at semver/semver#341.

See 9939a1b for an alternative to 4c39003 that keeps a few checks (make sure specs are satisfied, complain on duplicate entry points).

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

rosidl_cli/rosidl_cli/command/generate/helpers.py

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

No need for ` command`, rosidl has contol over all sub commands and can make sure none adds an argument called _command. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2021-03-01T18:18:47Z

BTW made a few minor tweaks here and there in 9036d3b.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

sloretz

I'm almost done reviewing, just submitting a couple thoughts before lunch.

rosidl_cli/rosidl_cli/command/__init__.py

rosidl_cli/rosidl_cli/extensions.py

rosidl_cli/rosidl_cli/command/generate/helpers.py

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

sloretz

LGTM. One last recommendation for a corner case, but I don't think it's blocking.

sloretz · 2021-03-02T00:25:18Z

rosidl_cli/rosidl_cli/entry_points.py

+                raise RuntimeError(msg)
+            logger.warning(msg)
+            continue
+        if specs and name not in specs:


I would recommend putting this check first so that if someone asked for the "c" entrypoint but there were two "cpp" entrypoints then it could still use the "c" extension without being blocked by the problem with the duplicate "cpp" extensions.

Sounds like a great idea. See 0128d9d.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2021-03-02T17:13:11Z

Re-running CI up to rosidl_cli:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2021-03-02T20:45:19Z

Try again after b24e4fa and f5f5e2b.

Linux
Linux-aarch64
macOS
Windows

hidmic · 2021-03-02T21:02:24Z

So, pathlib.Path.resolve() doesn't work on Windows when the file doesn't exist (see https://bugs.python.org/issue39090)... I'll update this patch, and all connected ones.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2021-03-02T21:24:28Z

Finally. @sloretz would you mind to take another look?

hidmic · 2021-03-03T17:58:11Z

Ok, going in. Thanks for the thorough review @sloretz!

Add rosidl generate CLI.

8211947

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic requested a review from sloretz January 28, 2021 18:34

hidmic mentioned this pull request Jan 28, 2021

Unified code generation CLI #565

Closed

21 tasks

hidmic requested a review from azeey January 29, 2021 22:35

hidmic added 3 commits February 9, 2021 13:03

Adjust rosidl generate interface_files help string

8445319

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Deal with pre-3.3.0 importlib.metadata

5d89971

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Add helper functions for rosidl generate extensions

6c90160

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

This was referenced Feb 9, 2021

Expose C code generation via rosidl generate CLI #569

Merged

Expose C++ code generation via rosidl generate CLI #570

Merged

Expose Python code generation via rosidl generate CLI ros2/rosidl_python#123

Merged

hidmic added 2 commits February 11, 2021 19:11

Add a few tests and docs

2dcc401

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Add helper function to generate visibility control files

f7a9d3a

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

This was referenced Feb 15, 2021

Expose C introspection typesupport generation via rosidl generate CLI #572

Merged

Expose C++ introspection typesupport generation via rosidl generate CLI #573

Merged

sloretz reviewed Feb 25, 2021

View reviewed changes

hidmic added 2 commits February 26, 2021 14:32

Address peer review comments.

3a75bcd

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Address more peer review comments.

d892fc2

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic mentioned this pull request Feb 26, 2021

Add rosidl translate CLI. #575

Merged

hidmic requested a review from sloretz February 26, 2021 21:29

hidmic added 2 commits February 26, 2021 19:38

Simplify get_first_line_doc()

219869e

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Handle no docstring case

b148c07

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

sloretz reviewed Feb 27, 2021

View reviewed changes

rosidl_cli/rosidl_cli/command/generate/helpers.py Show resolved Hide resolved

hidmic and others added 7 commits March 1, 2021 10:34

Drop rosidl.cli.main() optional arguments.

1140a09

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Remove unused cli_name and argv

e6cc39d

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Comment tweaks for clarity

f19a759

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Remove unused dest argument

687a78f

No need for ` command`, rosidl has contol over all sub commands and can make sure none adds an argument called _command. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Drop version specifiers

9939a1b

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Add TODO to use target_dependencies

b581585

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Apply minor tweaks

9036d3b

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix specs use cases

fc87e45

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

sloretz reviewed Mar 1, 2021

View reviewed changes

rosidl_cli/rosidl_cli/command/__init__.py Show resolved Hide resolved

rosidl_cli/rosidl_cli/extensions.py Outdated Show resolved Hide resolved

rosidl_cli/rosidl_cli/command/generate/helpers.py Outdated Show resolved Hide resolved

Address peer review comments

76bd5cc

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

sloretz approved these changes Mar 2, 2021

View reviewed changes

Address final peer review comments.

0128d9d

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Handle tempfile quirks on Windows.

b24e4fa

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Use os.path.abspath() to get absolute paths.

f5f5e2b

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

sloretz approved these changes Mar 3, 2021

View reviewed changes

hidmic merged commit f0b42cf into master Mar 3, 2021

delete-merged-branch bot deleted the hidmic/rosidl-cli branch March 3, 2021 17:58

hidmic mentioned this pull request Mar 9, 2021

Align rosidl_cli package version with the rest of the repo. #579

Merged

hidmic mentioned this pull request Jun 4, 2021

Support passing keyword arguments to rosidl CLI extensions #597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rosidl generate CLI. #567

Add rosidl generate CLI. #567

hidmic commented Jan 28, 2021

sloretz left a comment

sloretz Feb 24, 2021

hidmic Feb 26, 2021

sloretz Feb 24, 2021

hidmic Feb 26, 2021

sloretz Feb 26, 2021

hidmic Mar 1, 2021

sloretz Feb 25, 2021

hidmic Feb 26, 2021 •

edited

sloretz Feb 26, 2021

hidmic Feb 26, 2021

sloretz Feb 26, 2021

hidmic Mar 1, 2021 •

edited

hidmic Mar 1, 2021 •

edited

hidmic Mar 1, 2021

hidmic commented Mar 1, 2021

sloretz left a comment

sloretz left a comment

sloretz Mar 2, 2021

hidmic Mar 2, 2021

hidmic commented Mar 2, 2021

hidmic commented Mar 2, 2021 •

edited

hidmic commented Mar 2, 2021

hidmic commented Mar 2, 2021

hidmic commented Mar 3, 2021 •

edited

		return subparser


		def main(*, script_name='rosidl', argv=None, description=None, commands=None):

Add rosidl generate CLI. #567

Add rosidl generate CLI. #567

Conversation

hidmic commented Jan 28, 2021

sloretz 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

hidmic Feb 26, 2021 • 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

hidmic Mar 1, 2021 • edited

Choose a reason for hiding this comment

hidmic Mar 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Mar 1, 2021

sloretz left a comment

Choose a reason for hiding this comment

sloretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Mar 2, 2021

hidmic commented Mar 2, 2021 • edited

hidmic commented Mar 2, 2021

hidmic commented Mar 2, 2021

hidmic commented Mar 3, 2021 • edited

hidmic Feb 26, 2021 •

edited

hidmic Mar 1, 2021 •

edited

hidmic Mar 1, 2021 •

edited

hidmic commented Mar 2, 2021 •

edited

hidmic commented Mar 3, 2021 •

edited