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

Add SRDFWriter to this repo #14

Merged
merged 1 commit into from
Aug 24, 2016

Conversation

velveteenrobot
Copy link
Contributor

Fixes issue: moveit/moveit#66 from MoveIt.

@velveteenrobot
Copy link
Contributor Author

Merge this before moveit/moveit#142.

/* Author: Dave Coleman */

#ifndef MOVEIT_MOVEIT_SETUP_ASSISTANT_TOOLS_SRDF_WRITER_
#define MOVEIT_MOVEIT_SETUP_ASSISTANT_TOOLS_SRDF_WRITER_
Copy link
Member

@davetcoleman davetcoleman Aug 24, 2016

Choose a reason for hiding this comment

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

should be _SRDFDOM_SRDF_WRITER_

Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be fixed, should not have been merged yet

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed this myself before merging: 8bfdb1d

Copy link
Member

Choose a reason for hiding this comment

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

@rhaschke this is committing directly to the main branch, which circumvents the review process necessary for stable code. if you don't want to wait for @velveteenrobot to make the change I think you should merge this then open a secondary PR rather than commit directly, like i did yesterday here

@rhaschke rhaschke merged commit 054bed6 into moveit:indigo-devel Aug 24, 2016
@rhaschke
Copy link
Contributor

Merged. @130s Could you prepare a release of srdfdom to unblock moveit/moveit#142?
Thanks to @velveteenrobot.

@rhaschke
Copy link
Contributor

Hm. Now that I merged, Travis fails in MoveIt. I will add a travis config to srdfdom too.

@rhaschke
Copy link
Contributor

Fixed srdfdom. Updated .travis.yml to MoveIt's one. Travis is back to normal: https://travis-ci.org/rhaschke/srdfdom
@davetcoleman Do you have an idea, why https://travis-ci.org/ros-planning/srdfdom didn't build for ages?

@130s
Copy link
Contributor

130s commented Aug 24, 2016

Could you prepare a release of srdfdom to unblock moveit/moveit#142?

Let me know when it's ready.

Do you have an idea, why https://travis-ci.org/ros-planning/srdfdom didn't build for ages?

Has travis been on for this repo (I see now it's on at http://travis-ci.org/profile/ros-planning though).

@rhaschke
Copy link
Contributor

Do you have an idea, why https://travis-ci.org/ros-planning/srdfdom didn't build for ages?

Has travis been on for this repo (I see now it's on at http://travis-ci.org/profile/ros-planning though).

Yes, travis was configured to be on, but it nevertheless didn't triggered builds on pushes or PRs.
I just noticed that the detailed settings were configured to be off! Fixed.

@rhaschke
Copy link
Contributor

OK, Travis triggers again: https://travis-ci.org/ros-planning/srdfdom/builds/154885328
@130s: IMHO you could release now. @davetcoleman?

@davetcoleman
Copy link
Member

This is a violation of our review policy and standard software procedures - why are you committing some much unreviewed stuff to this repo?

screenshot from 2016-08-25 00-42-00

@rhaschke
Copy link
Contributor

I tried to hot-fixed the merge, which broke Travis of the main Moveit repo. That's why some urgent action was required. Before doing the original merge, I of course tested locally, and surprisingly I didn't observe any issues. Still not sure why.
Unfortunately, Travis was disabled for this repo, such that the issues didn't show up here automatically.

@130s
Copy link
Contributor

130s commented Aug 25, 2016

Will make a release 0.3.2 now.

Yeah, I have to agree with @davetcoleman about direct commits. Maybe @rhaschke you could have hot-fixed and then if that worked you revert and open a PR?

@v4hn
Copy link
Contributor

v4hn commented Aug 25, 2016

I also hot-fixed things in the past and I don't think it's fatal.
But hot-fixes should be limited to single commits.

a1dd532 is definitely no "hot-fix"
and enabling proper travis support isn't either.

@rhaschke
Copy link
Contributor

a1dd532 is definitely no "hot-fix"

What else? The call to exit() and the use of ros/console.h, which wasn't declared a dependency, broke building of the package. For this reason unrelated PRs of the main moveit repo failed on Travis:
https://travis-ci.org/ros-planning/moveit/pull_requests (237-238). Because of my hot-fix, the down-time was kept as short as possible.

@v4hn
Copy link
Contributor

v4hn commented Aug 26, 2016

On Thu, Aug 25, 2016 at 05:41:40PM -0700, Robert Haschke wrote:

The call to exit() and the use of ros/console.h, which wasn't declared a dependency, broke building of the package.

Then why didn't you write the explanation in the commit message? :-)
Please do that next time.

@130s
Copy link
Contributor

130s commented Aug 29, 2016

0.3.2 along with this change now made to shadow repo for Indigo and JK.

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

5 participants