-
Notifications
You must be signed in to change notification settings - Fork 30
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
Aikidopy uses outdated WorldInteractiveMarker #599
Conversation
…hough not yet in secrets]
…ics/aikido into egordon/rviz-world
@@ -20,7 +20,7 @@ fi | |||
# Build aikidopy and pytest | |||
if [ $BUILD_AIKIDOPY = ON ]; then | |||
./scripts/internal-run.sh catkin build --no-status --no-deps -p 1 -i --cmake-args -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DTREAT_WARNINGS_AS_ERRORS=OFF -DCODECOV=OFF --make-args aikidopy -- aikido | |||
./scripts/internal-run.sh make -C build/aikido pytest | |||
$SUDO ./scripts/internal-run.sh make -C build/aikido pytest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this change? This may be dumb of me, but it seems unrelated to the interface change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI fails to run without this addition, since the installation step requires sudo.
@@ -94,6 +88,12 @@ set_target_properties(aikidopy | |||
DEBUG_POSTFIX "" | |||
) | |||
|
|||
execute_process(COMMAND ${PYTHON_EXECUTABLE} -c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto: What's the reason for this change? This may be dumb of me, but it seems unrelated to the interface change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small readability change, since this variable is only used in the below lines.
Heads up: I changed the base branch to your GH actions one so we can see a better diff. This seems mostly fine, I just left two comments: I'm a bit confused since those changes don't seem to relate to |
@sniyaz Responded for review comments and updated CHANGELOG. Ready for re-review |
Aikidopy relies on the deprecated WorldInteractiveMarkerViewer which was removed in #521
This updates that interface.
Note: this depends on #598 , as this should fix the one failing (not required) test.
Before creating a pull request
make format
Before merging a pull request
CHANGELOG.md