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

[Major Release] Move service files: jsk_pcl_ros -> jsk_recognition_msgs to reuse #1827

Closed
wants to merge 1 commit into from

Conversation

wkentaro
Copy link
Member

@wkentaro wkentaro commented Aug 6, 2016

To remove dependency of jsk_interactive_marker on jsk_pcl_ros.

Background

Some packages in jsk_visualization depends on jsk_pcl_ros because it uses the srv files.
But jsk_pcl_ros is a very large package and the dependency should be avoided ideally.
The solution is placing msg/srv files into jsk_recognition_msgs.

Plans

  1. Merge this after jsk_recognition_msgs is released: http://repositories.ros.org/status_page/ros_indigo_default.html?q=jsk_recognition_msgs
  2. Release this pacakge.
  3. Remove jsk_pcl_ros/srv.
  4. Release jsk_pcl_ros.

@k-okada
Copy link
Member

k-okada commented Aug 7, 2016

I'm afraid this will affect large set of applications,,,

@wkentaro
Copy link
Member Author

wkentaro commented Aug 7, 2016

I'm afraid this will affect large set of applications,,,

So currently I'm using cmake macro for this migration.
The macro will generate message files both in jsk_recognition_msgs and jsk_pcl_ros.

@wkentaro wkentaro changed the title Move service files: jsk_pcl_ros/srv -> jsk_recognition_msgs/srv to reuse messages Move service files: jsk_pcl_ros -> jsk_recognition_msgs to reuse Aug 9, 2016
@wkentaro wkentaro changed the title Move service files: jsk_pcl_ros -> jsk_recognition_msgs to reuse [Major Release] Move service files: jsk_pcl_ros -> jsk_recognition_msgs to reuse Aug 23, 2016
@k-okada k-okada mentioned this pull request Aug 30, 2016
9 tasks
@wkentaro wkentaro added this to the 0.4.0 milestone Sep 6, 2016
@k-okada
Copy link
Member

k-okada commented Oct 21, 2016

Any comment on this pr? @mmurooka @YuOhara @iory ?

@mmurooka
Copy link
Member

Lots of source codes using jsk_pcl_ros.srvs must be updated, right?

@wkentaro
Copy link
Member Author

wkentaro commented Oct 21, 2016

Lots of source codes using jsk_pcl_ros.srvs must be updated, right?

No: #1827 (comment)
It generates the messages for both jsk_pcl_ros and jsk_recognition_msgs. This takes more build time, but won't break the API (I think).

@mmurooka
Copy link
Member

If currently working program does not get bad effect, this PR is OK.

@wkentaro
Copy link
Member Author

wkentaro commented Oct 21, 2016

If currently working program does not get bad effect, this PR is OK.

The whole bad effects can't be covered by only me.
Please describe about your experience or knowledge: for example

  • you usually use the service files in euslisp code
  • that is used in XXXX package
  • that is used by demo code in euslib

You said below, so you mean you usually the jsk_pcl_ros.srvs?

Lots of source codes using jsk_pcl_ros.srvs must be updated, right?

@mmurooka
Copy link
Member

In drc_task_common, maybe following is the only code using jsk_pcl_ros/srvs as log as I did grep.
https://github.com/jsk-ros-pkg/jsk_demos/blob/master/jsk_2015_06_hrp_drc/drc_task_common/src/drc_task_common/manipulation_data_server.cpp#L251

If my memory is correct, jsk_pcl_ros/EuclideanSegment is used in euslisp code of PR2 fridge demo.

@YuOhara
Copy link
Contributor

YuOhara commented Oct 21, 2016

Related issue seems #634.
Althogh cmake macro exists, is it better to convert jsk_pcl_ros->jsk_recognition_msgs, if this macro is currentry thing?

Scripts in jsk_pcl_ros like https://github.com/jsk-ros-pkg/jsk_recognition/blob/master/jsk_pcl_ros/scripts/tower_detect_viewer_server.py#L27 can be replaced in this PR? (and https://github.com/jsk-ros-pkg/jsk_recognition/blob/master/jsk_pcl_ros/scripts/tower_detect_viewer_server.py#L26 seems to be forgotten by someone?)

@garaemon somehow used sed command for migration.

Also, my grep result by grep -r "jsk_pcl_ros" | grep srv is below.
https://gist.github.com/YuOhara/8edab837dfd41448da3b605a8ca54fc4

@YuOhara
Copy link
Contributor

YuOhara commented Oct 21, 2016

Anyway if the macro works well, i am ok with this PR.
(also I confirmed that some packages like pal_msgs and pr2_msgs includes service files althoth it claims 'msgs')

@wkentaro
Copy link
Member Author

wkentaro commented Oct 21, 2016

Another option without the macro is:

  1. Copy .srv files to jsk_recognition_msgs
  2. Migrate all jsk_pcl_ros/*.srv to jsk_reocgnition_msgs/*.srv for codes under jsk-ros-pkg or start-jsk.
  3. Remove .srv files in jsk_pcl_ros

and this seems better.

Any idea?

wkentaro added a commit to wkentaro/jsk_recognition that referenced this pull request Oct 22, 2016
wkentaro added a commit to wkentaro/jsk_recognition that referenced this pull request Oct 22, 2016
@k-okada
Copy link
Member

k-okada commented Oct 25, 2016

ok, before we merge this PR what is the plan? when we stop using cmake
macro that generate srv file under both jsk_pcl_ros.srv and
jsk_recognition.srv? and who will fix codes using jsk_pcl_ros.srv? until
when?

◉ Kei Okada

2016-10-22 3:09 GMT+09:00 Kentaro Wada [email protected]:

Another option without the macro is:

  1. Copy .srv files to jsk_recognition_msgs
  2. Migrate all jsk_pcl_ros/.srv to jsk_reocgnition_msgs/.srv under
    jsk-ros-pkg or start-jsk.
  3. Remove .srv files in jsk_pcl_ros


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

@wkentaro
Copy link
Member Author

wkentaro commented Oct 25, 2016

before we merge this PR what is the plan?

I think this PR can be closed.
I changed the mind of migration plan: #1827 (comment) , if you have other ideas please tell me.

who will fix codes using jsk_pcl_ros.srv? until
when?

I sent PR for fixing that all jsk-ros-pkg and start-jsk packages.

@k-okada
Copy link
Member

k-okada commented Oct 27, 2016

moved to #1914 , for now

@k-okada
Copy link
Member

k-okada commented Dec 5, 2016

I sent PR for fixing that all jsk-ros-pkg and start-jsk packages.

all done, which PR to be merged next

@wkentaro
Copy link
Member Author

wkentaro commented Dec 5, 2016

Thank you for the cooperation.
The next PR to be merged is jsk-ros-pkg/jsk_visualization#623, which is a goal of this migration.

@k-okada
Copy link
Member

k-okada commented Dec 5, 2016

Remove jsk_pcl_ros/srv.

we also need this.

@wkentaro
Copy link
Member Author

wkentaro commented Dec 5, 2016

Ok, I summarized them: #1980

mizmizo pushed a commit to mizmizo/jsk_recognition that referenced this pull request Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants