Skip to content
This repository has been archived by the owner on Jul 11, 2020. It is now read-only.

Deprecate treeFromXml #8

Merged
merged 3 commits into from
May 19, 2020
Merged

Deprecate treeFromXml #8

merged 3 commits into from
May 19, 2020

Conversation

rotu
Copy link

@rotu rotu commented May 1, 2020

Clears the way for #7 in Foxy+1

Copy link

@clalancette clalancette 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 with green CI (don't forget to build and test robot_state_publisher as well). @sloretz any thoughts here?

@rotu
Copy link
Author

rotu commented May 1, 2020

Looks good to me with green CI

CI seems to have autorun and failed: http://build.ros2.org/job/Fpr__kdl_parser__ubuntu_focal_amd64/2/ I'm guessing because it's looking for unreleased binaries. Ran CI manually:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

(don't forget to build and test robot_state_publisher as well).

robot_state_publisher doesn't use treeFromXml. Did I miss something?

@sloretz
Copy link

sloretz commented May 1, 2020

@rotu restarted CI testing only packages above kdl_parser. We're a bit tight on CI capacity right now; so please make sure to use build and test args to limit the time a job takes when possible.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status <- edit Looks like mini1 needs PyQT5, will restart this job in a bit
    • rerun: Build Status
  • Windows Build Status
    Triggering a new build of ci_windows

@clalancette
Copy link

clalancette commented May 1, 2020

robot_state_publisher doesn't use treeFromXml. Did I miss something?

No, but it is the only in-tree user of kdl_parser, and it is cheap to test. So we may as well test that things are still good there.

@rotu
Copy link
Author

rotu commented May 1, 2020

@sloretz I don't know what I'm doing! Thank you for covering for me.

@clalancette
Copy link

@ros-pull-request-builder retest this please

@rotu rotu requested a review from clalancette May 14, 2020 21:14
@clalancette
Copy link

@rotu So we are still getting a warning in CI about using a deprecated method: https://ci.ros2.org/job/ci_linux/10766/gcc/new/. The problem is that kdl_parser::initXml is calling urdf::Model::initXml, which we've deprecated in ros2/urdf#12. Two thoughts on how to fix this:

  1. Add pragmas to disable deprecation warnings when we #include urdf headers. Probably a bit tricky amongst all platforms, but doable.
  2. Change the implementation of kdl_parser::initXml to convert the TiXmlDocument to a string, then call urdf::Model::initString. This will hurt performance, but since our conjecture here is that nobody is using this API, that shouldn't matter too much.

I'd personally go for the second, but I'll leave it up to you how to fix it.

Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looking good assuming we get green CI.

@clalancette
Copy link

For future reference, CI for this was in ros2/rviz#531 (comment)

@clalancette clalancette merged commit 8716008 into ros2:ros2 May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants