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

Copy jsk_pcl_ros/srv and jsk_perception/srv files to jsk_recognition_msgs #1914

Merged
merged 1 commit into from
Oct 29, 2016

Conversation

wkentaro
Copy link
Member

Related to #1827

  • jsk_pcl_ros/srv -> jsk_recognition_msgs/srv
  • jsk_perception/srv -> jsk_recognition_msgs/srv

TODO

    1. Migrate current code for srv files in jsk_recognition_msgs
    1. Remove srv files in jsk_pcl_ros and jsk_perception

- jsk_pcl_ros/srv -> jsk_recognition_msgs/srv
- jsk_perception/srv -> jsk_recognition_msgs/srv

TODO
- 1. Migrate current code for srv files in jsk_recognition_msgs
- 2. Remove srv files in jsk_pcl_ros and jsk_perception
@wkentaro wkentaro force-pushed the copy-srvs-to-be-deprecated branch from d4f5d60 to c80ef1a Compare October 21, 2016 18:28
wkentaro added a commit to wkentaro/jsk_recognition that referenced this pull request Oct 22, 2016
@wkentaro
Copy link
Member Author

wkentaro commented Oct 22, 2016

I sent PR to migrate all msg names in jsk-ros-pkg and start-jsk repos with script below:

@wkentaro
Copy link
Member Author

Could you please merge this?

@k-okada
Copy link
Member

k-okada commented Oct 25, 2016

does everyone understand this? @mmurooka @YuOhara @iory @orikuma ...

◉ Kei Okada

2016-10-25 9:11 GMT+09:00 Kentaro Wada [email protected]:

Could you please merge this?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1914 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAeG3Plq6evNMBsN6jolvwoNJas2LqZPks5q3UlPgaJpZM4KdeG_
.

@mmurooka
Copy link
Member

mmurooka commented Oct 26, 2016

既存のjsk_pcl_ros/.srv と移行後のjsk_recognition_msgs/.srv のどちらを使っても動くようにしているという理解であっているなら,大丈夫です.

@wkentaro
Copy link
Member Author

wkentaro commented Oct 26, 2016

既存のjsk_pcl_ros/.srv と移行後のjsk_recognition_msgs/.srv のどちらを使っても動くようにしているという理解であっているなら,大丈夫です.

いや、まだ移行はしてなくて、このPRはコピーしただけです。
移行は以下のPR全てで、

これらがマージされたらjsk_pcl_ros/srvは消すことになると思います。
(リリースは次のメジャーversion 4.0 に合わせればいいかと考えています。)

@mmurooka
Copy link
Member

このPRをマージした直後は,
列挙してくれたPRのマージ前後のどちらの状態でも動くということで合ってるかな.
それなら大丈夫です.

@wkentaro
Copy link
Member Author

はい、合っています。

@k-okada
Copy link
Member

k-okada commented Oct 26, 2016

確認だけど、#1827 と #1914の違いは mv + cmake vs copy ?
これが必要な理由はなんだっけ? jsk_recognition_msgs をdebでいれて、catkin bt したいんだっけ?
パッケージの中にメッセージをいれると、メッセージだけ入れたパッケージを作りましょう、という人がでてきて、
メッセージだけいれたパッケージを作ると、見に行くのが大変だからパッケージのなかに入れましょう、という人がでてくる気はしているんだけど.

◉ Kei Okada

2016年10月26日 14:26 Kei Okada [email protected]:

で、もんだいは、その後消した時、どうするか、です.
多分 @wkentaro は早い段階でメジャーリリースして消そうとしているはず.
でも @mmurooka は、ずっと「PRをマージした直後」の状態がキープされると思っている.
なきがします.

例えば、

  • とにかく deb を使う
    とか、
  • .rosinsstall を用意して wstool update していく、
    とか、すると綺麗に機能する気がしますが、

本日3号館地下の16号機を動かそうとしたところ,angle-vectorを送る際によくわからないエラーがでました.
両PCのpr2eusをgit pullしたところエラーは起こらなくなりました.

とかしていると、なかなかむづかしいと思います.なので、wstool update をつかっていく作戦を考えてみましょう.
https://github.com/start-jsk/jsk_apc/blob/master/kinect2.rosinstall
https://github.com/start-jsk/jsk_apc/blob/master/fc.rosinstall
が参考になります.

◉ Kei Okada

2016-10-26 12:42 GMT+09:00 Kentaro Wada [email protected]:

はい、合っています。


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1914 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAeG3Li7ct_X061jebTjX3a59Uv6fESEks5q3swRgaJpZM4KdeG_
.

@wkentaro
Copy link
Member Author

合ってます。

2016年10月26日水曜日、Masaki [email protected]さんは書きました:

このPRをマージした直後は,
列挙してくれたPRのマージ前後のどちらの状態でも動くということで合ってるかな.
それなら大丈夫です.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1914 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEHFk9Q2q9nPF6b9C_6ETsV3c3SiGUY5ks5q3sXZgaJpZM4KdeG_
.

和田 健太郎 / Kentaro Wada
http://wkentaro.com

@k-okada
Copy link
Member

k-okada commented Oct 26, 2016

で、もんだいは、その後消した時、どうするか、です.
多分 @wkentaro は早い段階でメジャーリリースして消そうとしているはず.
でも @mmurooka は、ずっと「PRをマージした直後」の状態がキープされると思っている.
なきがします.

例えば、

  • とにかく deb を使う
    とか、
  • .rosinsstall を用意して wstool update していく、
    とか、すると綺麗に機能する気がしますが、

本日3号館地下の16号機を動かそうとしたところ,angle-vectorを送る際によくわからないエラーがでました.
両PCのpr2eusをgit pullしたところエラーは起こらなくなりました.

とかしていると、なかなかむづかしいと思います.なので、wstool update をつかっていく作戦を考えてみましょう.
https://github.com/start-jsk/jsk_apc/blob/master/kinect2.rosinstall
https://github.com/start-jsk/jsk_apc/blob/master/fc.rosinstall
が参考になります.

◉ Kei Okada

2016-10-26 12:42 GMT+09:00 Kentaro Wada [email protected]:

はい、合っています。


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1914 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAeG3Li7ct_X061jebTjX3a59Uv6fESEks5q3swRgaJpZM4KdeG_
.

@mmurooka
Copy link
Member

でも @mmurooka は、ずっと「PRをマージした直後」の状態がキープされると思っている.

1. このPRをマージ. ( jsk_pcl_ros/msg の下を jsk_recognition_msg/msg にコピー.)
2. 列挙してあるPRをマージして,各プログラムでjsk_pcl_ros/msg を使わないようにして,
jsk_recognition_msg/msg を使うようにする
3. jsk_pcl_ros/msg の下を全部消す.

だと思っています.
いきなり #1827 をやるよりかはマイルドでいいと思います.

@wkentaro
Copy link
Member Author

確認だけど、#1827 と #1914の違いは mv + cmake vs copy ?

はい。

これが必要な理由はなんだっけ?

jsk-ros-pkg/jsk_visualization#623
のようなメッセージを再利用する場合にjsk_pcl_rosのような大きなパッケージに依存しない
ためで、

  • jsk_visualizaion -> jsk_recognition -> jsk_common

という依存関係は消せないにしても、依存度を下げていくべきだと思っているからです。
(将来的にjsk_visualization -> jsk_recognition_msgsという依存関係にして、
jsk_recognition_msgs をjsk_common_msgsに移動するのが理想かと思います。)

jsk_recognition_msgs をdebでいれて、catkin bt したいんだっけ?
パッケージの中にメッセージをいれると、メッセージだけ入れたパッケージを作りましょう、という人がでてきて、
メッセージだけいれたパッケージを作ると、見に行くのが大変だからパッケージのなかに入れましょう、という人がでてくる気はしているんだけど.

そうです。こちらの方針の有効性としては、

  1. パッケージの中にメッセージを入れるというのは
    jsk_apc2016_common のように広く再利用しない場合には有効ですが、
    https://github.com/start-jsk/jsk_apc/tree/master/jsk_apc2016_common/srv
    再利用を想定している jsk_pcl_ros に対しては不適だから。(メッセージを別パッケージにわけるのは複数のパッケージでそれを使いたい場合で、jsk_apc2016_commonは自分でpublishして自分で可視化までやるので分ける必要がないという判断です)
  2. ROSの共通認識だと考えられるから。 https://github.com/ros/common_msgs
  3. jsk_recognition_msgs がある以上、僕みたいに「jsk_pcl_rosに依存する理由がよくわからない」と言い出す人は必ずいると思うので、この流れは避けられない(と思われる)から。

@k-okada
Copy link
Member

k-okada commented Oct 27, 2016

2. 列挙してあるPRをマージして,各プログラムでjsk_pcl_ros/msg を使わないようにして,
jsk_recognition_msg/msg を使うようにする
3. jsk_pcl_ros/msg の下を全部消す.

はいつまでに出来ますか?それまで同じファイルが2箇所にあることになるので、片方変えたら、もう片方かえることになる.

@k-okada k-okada changed the title Copy deprecated srv files to jsk_recognition_msgs Copy jsk_pcl_ros/srv and jsk_perception/srv files to jsk_recognition_msgs Oct 27, 2016
@k-okada k-okada changed the title Copy jsk_pcl_ros/srv and jsk_perception/srv files to jsk_recognition_msgs [Major Release] Copy jsk_pcl_ros/srv and jsk_perception/srv files to jsk_recognition_msgs Oct 27, 2016
@wkentaro
Copy link
Member Author

はいつまでに出来ますか?それまで同じファイルが2箇所にあることになるので、片方変えたら、もう片方かえることになる.

これをMinor Releaseして、 (リリースされるまでに2日)

列挙してあるPRをマージして,各プログラムでjsk_pcl_ros/msg を使わないようにして,
jsk_recognition_msg/msg を使うようにする

これのTravisをリスタートしてマージする。(1日)

jsk_pcl_ros/msg の下を全部消す.

これをMajor Releaseする。

なので、合計3日だと思います。

@k-okada
Copy link
Member

k-okada commented Oct 28, 2016

https://github.com/jsk-ros-pkg/jsk_recognition/blob/master/.travis.yml#L23 があって sync されない通らないので3日にはならないと思いますが、これでいいですか? > @mmurooka @yuki-asano @orikuma @YoheiKakiuchi @YuOhara @iory

@mmurooka
Copy link
Member

長引いて困る問題は,

それまで同じファイルが2箇所にあることになるので、片方変えたら、もう片方かえることになる.

くらいでしょうか.他にもたくさんあるでしょうか.

@mmurooka
Copy link
Member

ROS_REPOSITORY_PATH="http://packages.ros.org/ros/ubuntu"にsyncされるのはどのくらいの頻度でしたでしょうか.

@k-okada
Copy link
Member

k-okada commented Oct 28, 2016

1-3ヶ月に一辺ぐらいでしょうか.リクエストしたら直ぐに出ることもある.

◉ Kei Okada

2016-10-28 12:17 GMT+09:00 Masaki Murooka [email protected]:

ROS_REPOSITORY_PATH="http://packages.ros.org/ros/ubuntu"にs
yncされるのはどのくらいの頻度でしたでしょうか.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1914 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAeG3DHXL0hsAH6bQfERcfHP2LVgK4Zjks5q4WlagaJpZM4KdeG_
.

@mmurooka
Copy link
Member

長いですね.

的外れだったら申し訳ないですが,
https://github.com/jsk-ros-pkg/jsk_recognition/blob/master/.travis.yml#L23
だと,予定している移行のどこかの段階でPRが通らなくなりますでしょうか.
jsk_pcl_rosなどのjsk_recognitionの下のリポジトリは,このテストでもソースを使うんだと思っているのですが,
その場合テストは全部通るようにも思います.

@k-okada
Copy link
Member

k-okada commented Oct 28, 2016

テストは通ります.毎回更から入れるから.
問題は、手元の環境であまりいじりたくないけど、ちょっとちょっとアップデートしています、というような人が
どっかかのだんかいで、あれれ、となるはずです.

@mmurooka
Copy link
Member

通らないので3日にはならないと思いますが、これでいいですか? > @mmurooka

テストが通るなら3日でできるはずでしょうか.(本当に3日かはともかく,@wkentaro のシナリオの前提で成り立たないところはないでしょうか.)

@k-okada
Copy link
Member

k-okada commented Oct 28, 2016

このシナリオはユーザはみんなdebを使っているか、ソースを使っているけど、自分が何をやっているかちゃんと理解しているという
前提になっていると思います.jsk-ros-pkg/jsk_pr2eus#237 (comment) からの、
jsk-ros-pkg/jsk_pr2eus#237 (comment) をみてもわかるように、ほとんどの人は
何が起こっているかわかっていないはず.なので、どこをベースラインにするかだけど、8割ぐらいの人はカバーしたほうがいいと思う.
とばっちりだけど、titanの環境もいれてもらったし、もうできるだけ弄りたくないので、なにかやるんだったら、0からやら
ずに今あるものに必要と思われるところだけ、つぎ足しつぎ足し、コピーコピーで、できるだけ、もういじりたくありません、
という風な状態になっているような人は、こういう変更があると、あれれ、となる気がします.
あれれ、となったときに、大騒ぎして直す方向に行けばいいですが、それでも、大騒ぎになった時にどうすんの?という問題と、それ以上に
騒がずにこっそり、バックアップしていたディレクトリを元に戻して、二度とアップデートしませんと、なった時に、治ってるはずのバグで
つまずくとか、こういうコンパイルできないパッケージは意味わかんないから新しい綺麗なのを作りました、といいだしてくること、とか
毎週毎週、コンパイルしていました、という人がでてくるとか、そういうことが想定されます.
でも、もう21世紀だし、大丈夫かもしれないから、どんどん変えてみて、変なことになっても、それはそれで、こういうふうにしたら、
こういう風に困るよ、というイベントは1年に1回ぐらい起こしておくのがいちいち説明する面倒なくていいかな、とも思います.

@mmurooka
Copy link
Member

難しいですね..まだまだ勉強不足です.
jsk-ros-pkg/jsk_pr2eus#237 (comment) の自分なりの反省は,何か意見を聞かれたらスルーせずに答えようということなのですが,
今回の

これでいいですか?

については,すみませんがまだちょっとよくわかりません,でおねがいします..

@wkentaro
Copy link
Member Author

wkentaro commented Oct 28, 2016

jsk-ros-pkg/jsk_pr2eus#237 は影響のあるところ(でわかっているところ)にPRを送らなかったというのが一つ大きな原因なのではないでしょうか?
(それをしない代わりに@で聞いていたんだと思いますが)

おそらく21世紀的な解決方法は、作られたメッセージのcppファイルやpythonファイル、euslispファイルがincludeされたらdeprecationメッセージ出すようにすることで、
3日で削除は無理そうですが、deprecationメッセージを出して3ヶ月位たってjsk_pcl_ros/srvを消すということになりそうだと考えています。
僕が3日といったのは、jsk_pcl_rosのメッセージはほとんどJSKでしか使われていないという仮定に基づいて、start-jskとjsk-ros-pkg以下の全てを直せばいいという考えに依るものです。
やはり出せるようにするのがベストですよね?

これでいいですか?

については,すみませんがまだちょっとよくわかりません,でおねがいします..

は僕の解釈では、
「これ以外のより良い解決策を知らない?」
「これでクリティカルな問題にならないとwkentaroは言ってるけど賛成する?」
「もしこれで問題が起こったら、直すのに協力してくれる?」
的な意味だと思います。

すみません、質問の意味がわからないということではなかったのですね。。

@k-okada k-okada merged commit 904ada4 into jsk-ros-pkg:master Oct 29, 2016
@k-okada
Copy link
Member

k-okada commented Oct 29, 2016

jsk-ros-pkg/jsk_pr2eus#237 は影響のあるところ(でわかっているところ)にPRを送らなかったというのが一つ大きな原因なのではないでしょうか?

そうです.これは影響があるのがロボットで、それがどれかもわかっていたので、そのままにしてみましたが、それでも使っている人は誰も気が付かずに、作った人は、なんでマージしてくれないんだろう、となっている例です.

一般的にはこういうのは、stableブランチをみんなにつかってもらって、開発者はdevelopブランチで進めていく、というのがよくある方法で、ROSの場合は
indigo-devel, jade-devel, kinetic-devel として、例えばいまなら、新しい変更は全てkinetic-develでやってくれ、とか、kinetic-develはリリースしたからmasterで
やってくれ、いうのがひとつの方法だと思いますが、結局ロボットがkineticで動いていない以上、みんなbackportしてくれ、となって、最終的にロボットで何かを
見せようとしている人はなかなかそうはいかないはずです.
これでうまくいっている場合は、かなり狭い範囲で物事を考えている場合で、認識した結果から動作計画して、実機にデータを送りながら、センサ情報をうけとってフィードバックする、
とか考えると、制約が多すぎて、なかなかいい解決策がないですね、という結論になるんだと思う.
また、そうでなくても、複数のブランチを管理していくのはかなり大変で、最初はみんなこれやろう、といいますが、すぐに飽きているように見える..rosdistro/kinetic$ grep version: distribution.yaml

なので、本当はこういう変更をしたら、コードも綺麗になるし、コンパイル時間も減るし、絶対いいよ、となっても、制約条件はいろいろあって、まぁ、むつかしいしどうするかな、
というような例だんだとおもうんだけど、あまりそういう声は聞かれないので、@mmurooka さんからはしごを外された格好になっているけど、マージしました.

@k-okada
Copy link
Member

k-okada commented Oct 29, 2016

で、リリースすると、pipでハマると.
リリースする、pipを使う、alphabet order キープする、の3つも、それぞれが制約になってうまく行かないので、どれかは諦めて、パーフェクトではないけど、まぁまぁ、と言う感じで進めるのが良いパターンだともっています.

Releasing package 'jsk_recognition_utils' for 'indigo' to: 'release/indigo/jsk_recognition_utils'
 [git-bloom-patch import]: Applied 1 patches
Releasing package 'jsk_perception' for 'indigo' to: 'release/indigo/jsk_perception'
 [git-bloom-patch import]: 'execute_command' failed to call 'git am /tmp/tmpz9PStB/*.patch' which had a return code (1):
 [git-bloom-patch import]: ```
Applying: comment out pip depends
error: patch failed: package.xml:70
error: package.xml: patch does not apply
Patch failed at 0001 comment out pip depends
The copy of the patch that failed is found in:
   /tmp/tmpAE7Fhl/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

 [git-bloom-patch import]: ```
 [git-bloom-patch import]: Failed to apply one or more patches for the 'Command 'git am /tmp/tmpz9PStB/*.patch' returned non-zero exit status 1' branch.


>>> Resolve any conflicts and when you have resolved this problem run 'git am --resolved' and then exit the shell using 'exit 0'. <<<
    To abort use 'exit 1'

@k-okada
Copy link
Member

k-okada commented Oct 29, 2016

リリースしました ros/rosdistro#13101
http://repositories.ros.org/status_page/ros_indigo_default.html?q=jsk_recognition で状況確認できます.

@wkentaro wkentaro deleted the copy-srvs-to-be-deprecated branch October 29, 2016 10:35
@wkentaro
Copy link
Member Author

wkentaro commented Oct 29, 2016

で、リリースすると、pipでハマると.

すみません。いつもお手数掛けます。

wkentaro referenced this pull request Oct 30, 2016
wkentaro added a commit to wkentaro/jsk_recognition that referenced this pull request Nov 21, 2016
@wkentaro wkentaro changed the title [Major Release] Copy jsk_pcl_ros/srv and jsk_perception/srv files to jsk_recognition_msgs Copy jsk_pcl_ros/srv and jsk_perception/srv files to jsk_recognition_msgs Nov 24, 2016
k-okada added a commit that referenced this pull request Nov 28, 2016
…_recognition_msgs

[Major Release] Migrate srv files from jsk_pcl_ros to jsk_recognition_msgs for #1914
@yuki-asano
Copy link

https://github.com/jsk-ros-pkg/jsk_recognition/blob/master/.travis.yml#L23 があって sync されない通らないので3日にはならないと思いますが、これでいいですか? > @mmurooka @yuki-asano @orikuma @YoheiKakiuchi @YuOhara @iory

かなり遅くなりましたが,
一応名前が挙がっているので返答いたしますと,
自分も @mmurooka 君と同じで,

これでいいですか?
については,すみませんがまだちょっとよくわかりません,でおねがいします..

という状況です.
私も最近演習などでやっと,jsk_pcl_rosの文字列を見るようになってきましたが,
今のところ問題なく動いていそうですので,こちらで大丈夫です.

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