-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: use quote to generate code #50
Conversation
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. |
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.
Yes that sounds like a good idea. We could do that later as a separate pr.
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 😄 |
Hi, I pushed some code that sets configuration options depending on the ROS_DISTRO. If you rebase on top of ccdfc1e you can use |
316a833
to
501243b
Compare
Pushed a fix. I tested this under foxy, galactic and humble, and all tests passed. |
Hi! I tried your branch now but I noticed some issues because I have the Also now I think we can just do conditional compilation in |
501243b
to
5056593
Compare
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. |
Hi @JiangengDong, I've done a parallel work on #58 and you may take a look. |
5056593
to
f9675c0
Compare
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 @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). |
@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. |
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. |
Cool @JiangengDong . I just merged #58. |
Rewrite the codegen with
quote!
, so it will be easier to add new features like the static array.Progress:
r2r_msg_gen
cargo check
passescargo test
passesr2r
's build script to also usequote
Other thoughts
r2r_msg_gen
is used as a build-dependency ofr2r
only. After this PR,proc-macro2
andquote
are runtime dependencies ofr2r
, which is not a good idea.syn
to parserust-bindgen
's output, instead of accessingrosidl_typesupport_introspection_c__MessageMembers