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

Separate moveit_experimental packages #1606

Merged
merged 3 commits into from
Aug 12, 2019

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Aug 8, 2019

This is a replacement for #1549, factoring out all packages in moveit_experimental as proper ROS packages. This allows to build them separately as intended by #1549.
Currently, only jog_arm is enabled, all other packages are catkin-ignored (some of them are not even catkinized yet). In contrast to #1549, this PR leaves all folders in their place.
That is, moveit_experimental just acts as a container folder for these packages.
As we don't plan to release moveit_experimenal packages, there is no need to define a corresponding meta package.

@rhaschke rhaschke mentioned this pull request Aug 8, 2019
7 tasks
Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

Looks good to me

publish_joint_positions: true
publish_joint_velocities: false
publish_joint_accelerations: false

Copy link
Member

Choose a reason for hiding this comment

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

Delete 2 lines here

@rhaschke rhaschke requested a review from v4hn August 8, 2019 19:12
@@ -1,31 +1,68 @@
set(JOG_ARM_NAME jog_arm)
cmake_minimum_required(VERSION 2.8.3)
project(jog_arm)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we happy naming this jog_arm or should this have a moveit prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm definitely agree to name this package moveit_jog_arm in order to distinguish it from the existing jog_arm package. Actually, I wanted to ask @AndyZe to apply this change after the present PR is merged ;-)
Do you think, it should be part of the present PR instead?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I like the shorter name.

jog_arm was never released as a debian for Melodic so there won't be a conflict there (and I don't mind removing the Kinetic debian from rosdistro). So don't worry about name conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

Made a PR to remove the old version of jog_arm from rosdistro here:

ros/rosdistro#21987

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All packages of the MoveIt universe are prefixed with moveit_. This should hold for jog_arm as well. That's the "downside" of your decision to migrate the package into MoveIt 😉

Copy link
Member

Choose a reason for hiding this comment

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

I like moveit_jog_arm

The upside is we have really good tools in the moveit repo for code quality and maintenance :-)

Make jog_arm an own package located in the moveit_experimental folder.
@v4hn v4hn merged commit 73feeb8 into moveit:master Aug 12, 2019
@v4hn
Copy link
Contributor

v4hn commented Aug 12, 2019

You're right, renaming does not have to be part of this request.
I created a new issue for it here: #1612 .

@rhaschke , you mentioned you merge and push pull-requests almost exclusively on the command line. I just tried to squash your commits and merge (granted, this triggers the old squash-vs-plain-merge debate) from my system (opposed to the github web page), but the repository setup does not let me push, saying:

remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: Required status check "continuous-integration/travis-ci" is expected. At least 1 approving review is required by reviewers with write access.
To github.com:ros-planning/moveit.git
 ! [remote rejected]     master -> master (protected branch hook declined)
error: failed to push some refs to 'git@github.com:ros-planning/moveit.git'

How do you handle this in your review process?

I am rather unhappy with safety measures that make it impossible to work with git the conventional way, @davetcoleman .

@rhaschke rhaschke deleted the separate-experimental-pkgs branch August 12, 2019 11:14
@rhaschke
Copy link
Contributor Author

@v4hn, I perform the review process locally, but usually, I use github's web interface to finally merge.
Not doing so, the manual commits would not (automatically) refer to github's pull request ID.

@davetcoleman
Copy link
Member

image

;-)

@tylerjw tylerjw mentioned this pull request May 8, 2020
20 tasks
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request May 12, 2020
* Make jog_arm an own package located in the moveit_experimental folder.

* adapt package location

* minor cleanup
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request May 12, 2020
* Make jog_arm an own package located in the moveit_experimental folder.

* adapt package location

* minor cleanup
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request May 20, 2020
* Make jog_arm an own package located in the moveit_experimental folder.

* adapt package location

* minor cleanup
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request Jun 24, 2020
* Make jog_arm an own package located in the moveit_experimental folder.

* adapt package location

* minor cleanup
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request Jun 24, 2020
* Make jog_arm an own package located in the moveit_experimental folder.

* adapt package location

* minor cleanup
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
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

4 participants