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

Refactor generate messages codes pojiro #185

Merged
merged 57 commits into from
Oct 25, 2022

Conversation

pojiro
Copy link
Contributor

@pojiro pojiro commented Oct 21, 2022

コード生成機能を Makefile、msgtype.sh x2 から mix rclex.gen.msgs(lib/mix/tasks/rclex/gen/msgs.ex) に切り出しました。

  • CI は 1.11 以前と以降で mix format の整形が異なるため落ちています。
    • また 1.11 は then がないため msgs.ex が動作しません。
  • test/expected_files はコード生成出力をassertするためのテスト用ファイル群をおさめるディレクトリです。

コミットは開発の進捗に合わせ刻んでしまったため、ヒストリーとしては汚くなってしまいました。

変更を行ったコードにいくつかコメントを行っています。 File changed からご確認をお願いいたします。

質疑を通したレビューが必要であれば打ち合わせをご検討下さい。


使用方法

$ mix new use_rclex
$ cd use_rclex

add rclex to deps of mix.exs

  defp deps do
    [
      {:rclex, git: "https://github.com/rclex/rclex.git", branch: "refactor-generate_messages_codes-pojiro"}
    ]
  end

add rclex config to config/config.exs

import Config

config :rclex, ros2_message_types: ["std_msgs/msg/String"]
$ mix deps.get
$ mix rclex.gen.msgs --from /opt/ros/foxy/share
$ mix deps.compile rclex # compile again, cause we generated rclex's codes
$ source /opt/ros/foxy/setup.bash
$ iex -S mix # WE CAN USE Rclex

$ mix rclex.gen.msgs --clean # this resets generated codes of rclex

Sampe repo is here, https://github.com/pojiro/use_rclex

@pojiro pojiro force-pushed the refactor-generate_messages_codes-pojiro branch from ff6b495 to dc1b1ea Compare October 21, 2022 11:15
@takasehideki
Copy link
Member

mix rclex.gen.msgs --clean するといろいろビルドが走っているように見えるのですが,そういうものでしょうか?(make clean 的なものを想像するとファイル削除だけな気がするのに,なにかいろいろ走っているのが気になります)

@takasehideki
Copy link
Member

mix rclex.gen.msgs で書き換わるファイルがあって,それを commit & push するとよろしくないのがツラいです. mix test には必要だけど書き換え後のファイルをただ push すればよいというものでもなさそうです..gitignore に入れるといいのか,でもそれだとバージョン管理しきれなくなるし,悩ましいです.

$ mix rclex.gen.msgs --from /opt/ros/foxy/share
cc -o /home/takase/rclex/rclex/_build/dev/lib/rclex/priv/rclex_nifs.so /home/takase/rclex/rclex/_build/dev/lib/rclex/obj/msg_nif.o /home/takase/rclex/rclex/_build/dev/lib/rclex/obj/graph_nif.o /home/takase/rclex/rclex/_build/dev/lib/rclex/obj/total_nif.o /home/takase/rclex/rclex/_build/dev/lib/rclex/obj/subscription_nif.o /home/takase/rclex/rclex/_build/dev/lib/rclex/obj/publisher_nif.o /home/takase/rclex/rclex/_build/dev/lib/rclex/obj/init_nif.o /home/takase/rclex/rclex/_build/dev/lib/rclex/obj/wait_nif.o /home/takase/rclex/rclex/_build/dev/lib/rclex/obj/node_nif.o -g -shared -L/home/takase/.asdf/installs/erlang/25.0.3/usr/lib -L/opt/ros/foxy/lib -lrcl -lrmw -lrcutils -lrosidl_runtime_c -lrosidl_typesupport_c -lrosidl_typesupport_introspection_c -lfastcdr -lfastrtps -lrmw_fastrtps_cpp 

$ git status 
On branch refactor-generate_messages_codes-pojiro
Your branch is up to date with 'origin/refactor-generate_messages_codes-pojiro'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   lib/rclex/msg_types_nif.ex
        modified:   src/msg_types_nif.ec
        modified:   src/msg_types_nif.h

no changes added to commit (use "git add" and/or "git commit -a")

@pojiro
Copy link
Contributor Author

pojiro commented Oct 24, 2022

@takasehideki

hex publish 対象から package.txt は削っていいものだと思っています. https://github.com/rclex/rclex/blob/main/mix.exs#L54

逆に入れないといけないファイルが無いかが気になっています.

ddeaf01: package.txt の削除
b9188bd: priv の追加

で対応しました。

@pojiro
Copy link
Contributor Author

pojiro commented Oct 24, 2022

@takasehideki

mix rclex.gen.msgs --clean するといろいろビルドが走っているように見えるのですが,そういうものでしょうか?(make clean 的なものを想像するとファイル削除だけな気がするのに,なにかいろいろ走っているのが気になります)

mix task として実行する場合、task 実行前に mix compile が必要なためと考えられます。
「一度 mix compileはしているはずだから、動作不要なはずでは?」という指摘は最もなのですが、現状分かっていません。

@pojiro
Copy link
Contributor Author

pojiro commented Oct 25, 2022

@takasehideki

mix rclex.gen.msgs で書き換わるファイルがあって,それを commit & push するとよろしくないのがツラいです. mix test には必要だけど書き換え後のファイルをただ push すればよいというものでもなさそうです..gitignore に入れるといいのか,でもそれだとバージョン管理しきれなくなるし,悩ましいです.

現行の rclex が抱えている課題で、本PRでも解消していないものです。コード生成により自己改変的であるため回避できていません。
本課題は rclex の開発者を悩ませるもので、運用としての回避策は無くはないようです

https://qiita.com/usamik26/items/56d0d3ba7a1300625f92

ただし、設定していることを忘れてしまうと嵌りそうだと思います。

@pojiro
Copy link
Contributor Author

pojiro commented Oct 25, 2022

@s-hosoai

test/expected_files/*_function.txt は、コード生成に向けた中間形式の期待値比較用という認識で合ってますかね?

はい、そのとおりです。

@takasehideki
Copy link
Member

hex publish の話しと mix task の話しはOKです.後者はそういうものだと認識しました.

@takasehideki
Copy link
Member

ROS2 -> ROS 2 の修正をしたときにVSCodeの auto-formatter が走ったらしく,生成先コードとテストコードとの不整合が発生したみたいです. 47e771d#diff-b3997e2b41c0ee0df58c81ab0409b89d8f927e10de2b061a942afe2fa1b471c5L655
a9e3068 で修正しました

@takasehideki
Copy link
Member

コード生成の話は悩ましいです.私はうっかり git commit . しちゃうことが多く,このPRでも何度かやらかしました.
このようにするのでいかがでしょうか?

  1. 生成元のテンプレートコードを用意し,これはgit管理対象に(というかもう用意されていると認識しています)
  2. 生成先のコードは管理外にする .gitignore にも追加する
  3. 各自でローカルからも管理外にする(まだそんなにユーザも開発者もいないので,いまがチャンス?)

@pojiro pojiro force-pushed the refactor-generate_messages_codes-pojiro branch 2 times, most recently from 0c8d28e to 02491e9 Compare October 25, 2022 02:44
@pojiro pojiro force-pushed the refactor-generate_messages_codes-pojiro branch from 02491e9 to 64e8061 Compare October 25, 2022 02:49
@pojiro
Copy link
Contributor Author

pojiro commented Oct 25, 2022

@takasehideki

コード生成の話は悩ましいです.私はうっかり git commit . しちゃうことが多く,このPRでも何度かやらかしました. このようにするのでいかがでしょうか?

  1. 生成元のテンプレートコードを用意し,これはgit管理対象に(というかもう用意されていると認識しています)
  2. 生成先のコードは管理外にする .gitignore にも追加する
  3. 各自でローカルからも管理外にする(まだそんなにユーザも開発者もいないので,いまがチャンス?)

上記の3も不要にできるような対応を 64e8061 として考えてみました。

@takasehideki
Copy link
Member

64e8061 is perfect for me!!

Copy link
Member

@takasehideki takasehideki left a comment

Choose a reason for hiding this comment

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

awesome thanks!! go merging!!!

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.

None yet

3 participants