Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rosidl_cli is broken because of missing type_description support #808

Open
marcoesposito1988 opened this issue May 26, 2024 · 4 comments
Open

Comments

@marcoesposito1988
Copy link

Bug report

Required Info:

  • Operating System:
    • Ubuntu 24.04
  • Installation type:
    • binaries from APT
  • Version or commit hash:
    • 4.6.2-1noble.20240513.233918
  • DDS implementation:
    • Fast-RTPS (not relevant)

Steps to reproduce issue

Use rosidl_cli to generate C/C++ code from a (very simple) test message

generate -o /tmp/test my_msgs /home/myuser/my_msgs:msg/MyTestMessage.msg

Expected behavior

Code generated correctly (as it happened with ROS2 humble)

Actual behavior

Fails with:

TypeError when expanding 'idl__description.c.em' into '.../MyTestMessage__description.c': 'NoneType' object is not subscriptable

If I get it correctly, this is due to type_description_info being None here, because it is never added here (see here)

Am I missing anything? Was rosidl_cli just not updated recently to cope with changes in the other generators? Or is a fix already underway?

I guess that fixing this would require hardcoding the type_description generator to be run before any other, and then update the legacy_generator_arguments_file function to be non-legacy anymore.
Can anyone confirm this? If positive, I could attempt a fix

@nkoenig
Copy link

nkoenig commented May 31, 2024

@sloretz , sorry to bug you. Just curious if you have spare cycle to peek at this issue. Thanks!

@marcoesposito1988
Copy link
Author

I found a (inelegant) solution for my problem: https://github.com/marcoesposito1988/rosidl/tree/add-type-description-to-rosidl-cli

On this branch I added:

  • the generation of rosidl_generator_cpp__visibility_control.hpp. This should be trivial enough for me not having messed up anything.
  • a cli entry point for the type_description generator. This should also be fine.
  • the missing arguments to legacy_generator_arguments_file(). This function was already being used for all generators, which is fine because extra arguments would just be ignored. So I guess that I didn't make the situation worse than before.
  • forcing type_description to run before the c and cpp generators: this is what I am unsatisfied with. The cmake scripts ensure topological order of the generators, which I don't have the time to implement myself right now.

If anyone can confirm that I am down the right/desired path, I will try to implement the generator topology in python in rosidl-cli as soon as possible and open a PR

@gergondet-woven
Copy link

@marcoesposito1988 Thanks a lot for your help, I have encountered the same issue with rosidl under Jazzy and your patch has been really helpful, this would be great if it could be integrated into the upstream repository and ultimately make its way back to Jazzy.

If I may, I have a couple of extra notes.

First, type_description generation is required for other generators, so rosidl generate -t cpp -o generated my_msgs my_folder:msg/MyMsg will fail, one should either add -t type_description to the command line, run type_description generation in the same output folder before or patch rosidl to always insert the type_description generator in that case (however, the later changes where the files are output when you were specifying a single type so it's a minor annoyance)

Second, the type_description generator is also more sensitive on having the correct -I/--include-path option, if your message description includes anything besides builtin types or types inside your own package, it will fail if you don't specify the include path properly; that's because this generator needs to have the JSON of your dependencies whereas the other generators only generate the appropriate include/import and don't care too much that the messages exist at all.

@fujitatomoya
Copy link

@marcoesposito1988 thank you for finding the solution. could you consider to open the PR, we can start there.

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

No branches or pull requests

4 participants