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

feat: use quote to generate code #50

Conversation

JiangengDong
Copy link

Rewrite the codegen with quote!, so it will be easier to add new features like the static array.

Progress:

  • rewrite r2r_msg_gen
  • cargo check passes
  • cargo test passes
  • rewrite r2r's build script to also use quote

Other thoughts

  • redesign crate dependency so r2r_msg_gen is used as a build-dependency of r2r only. After this PR, proc-macro2 and quote are runtime dependencies of r2r, which is not a good idea.
  • use syn to parse rust-bindgen's output, instead of accessing rosidl_typesupport_introspection_c__MessageMembers

@m-dahl
Copy link
Collaborator

m-dahl commented May 31, 2023

Looks like a good start! I think the errors you get on galactic are related to the ros constants either being renamed or maybe moved between the ros versions. I vaguely remember hacking around it, I can take a closer look when I find some time.

@m-dahl
Copy link
Collaborator

m-dahl commented Jun 2, 2023

I looked at the issue with the constants and I see I had already written a comment about it in the code which I'm sure you saw. I think the cleanest way to handle it is to do conditional compilation on the ROS_DISTRO environment variable, and just have different cases for humble and foxy, galactic (I think these should behave the same). Then we don't need to duplicate the constants.

redesign crate dependency so r2r_msg_gen is used as a build-dependency of r2r only. After this PR, proc-macro2 and quote are runtime dependencies of r2r, which is not a good idea.

Yes that sounds like a good idea. We could do that later as a separate pr.

use syn to parse rust-bindgen's output, instead of accessing rosidl_typesupport_introspection_c__MessageMembers

My initial feeling is that this will be more trouble than its worth, but feel free to take a stab at it, it could be fun 😄

@m-dahl
Copy link
Collaborator

m-dahl commented Jun 2, 2023

Hi, I pushed some code that sets configuration options depending on the ROS_DISTRO. If you rebase on top of ccdfc1e you can use #[cfg(r2r__ros__distro__galactic)] etc to trigger conditional compilation.

@JiangengDong JiangengDong force-pushed the jiangeng/use_quote_for_code_generation branch from 316a833 to 501243b Compare June 10, 2023 21:57
@JiangengDong
Copy link
Author

Pushed a fix. I tested this under foxy, galactic and humble, and all tests passed.

@m-dahl
Copy link
Collaborator

m-dahl commented Jun 11, 2023

Hi! I tried your branch now but I noticed some issues because I have the test_msgs package (ros-humble-test-msgs if you are on ubuntu), which are good to have sourced when working on this. I will update the CI to include those for more comprehensive testing.

Also now I think we can just do conditional compilation in FieldType::new and use the different ROS enums there, then we don't need to hard code the constants and we don't need the code that checks if they have changed.

@m-dahl m-dahl force-pushed the jiangeng/use_quote_for_code_generation branch from 501243b to 5056593 Compare June 19, 2023 06:52
@m-dahl
Copy link
Collaborator

m-dahl commented Jun 19, 2023

Just a heads up that I force pushed some changes to your branch. I rebased on top of the new CI, fixed the issues with test_msgs, and had to add a temporary hack around the type name for static arrays for constants. But I figure we deal with that last bit once we rework the parsing of the constants.

@jerry73204
Copy link
Contributor

Hi @JiangengDong, I've done a parallel work on #58 and you may take a look.

@JiangengDong JiangengDong force-pushed the jiangeng/use_quote_for_code_generation branch from 5056593 to f9675c0 Compare June 25, 2023 18:06
@JiangengDong
Copy link
Author

JiangengDong commented Jun 25, 2023

Hi @m-dahl sorry for my slow progress, and thank you for fixing it. I just rebased the branch to resolve the conflict with master.

@jerry73204 I think your PR has more features than mine. One suggestion about the codegen part is that the snippet for checking the array size appears repeatedly in fields, from_native, and copy_to_native blocks. On my PR, I check them once, store them as enum, and generate code based on the enum. You can feel free to copy those from my PR.

@m-dahl @jerry73204 what's your idea about these two PRs? I don't mind closing mine because the other one looks more feature-complete (need more cleanup, though).

@m-dahl
Copy link
Collaborator

m-dahl commented Jun 25, 2023

@JiangengDong @jerry73204 Thank you both for the improvements. It's unfortunate that there was duplicate work but that is just how it is. I am travelling at the moment and do not have much time to look at the PRs in detail the coming week. But as you say @JiangengDong it looks like @jerry73204's PR includes both using syn for parsing and also using quote in the build.rs files. But you hade some other nice cleanups included in this PR that I think we want. My suggestion would be to merge @jerry73204's pr to master as it is, and then we can open new PRs that adds some of @JiangengDong's more general code cleanups.

@JiangengDong
Copy link
Author

This sounds good to me. I am going to close this PR and open a new one after #58 is merged.

@jerry73204 let me know if you need any help on your PR.

@m-dahl
Copy link
Collaborator

m-dahl commented Jun 25, 2023

Cool @JiangengDong . I just merged #58.

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

Successfully merging this pull request may close these issues.

4 participants