[Bug 1817595] Re: [SRU] urdfdom-headers and urdfdom should not use locale dependent parsing for floating point numbers

[simonschmeisser]

That's why I would have preferred a simple update to 1.0.3 patch release
... it's not only cmake but also a support issue as now you have
differentiate between upstream 1.0.0 and ubuntu 1.0.0

I just hope that as soon as this hits the update channels most/all
people will install it automatically and we never hear about this bug
again :)

-- 
You received this bug notification because you are a member of Ubuntu
Sponsors Team, which is subscribed to the bug report.
https://bugs.launchpad.net/bugs/1817595

Title:
  [SRU] urdfdom-headers and urdfdom should not use locale dependent
  parsing for floating point numbers

Status in Urdfdom Headers:
  Unknown
Status in urdfdom package in Ubuntu:
  Fix Released
Status in urdfdom-headers package in Ubuntu:
  Fix Released
Status in urdfdom source package in Bionic:
  Fix Committed
Status in urdfdom-headers source package in Bionic:
  Fix Committed

Bug description:
  [Impact]

  * The parsing done by the code of urdfdom truncate floating values on
  systems that are not using the locales defined by default on English
  systems. The users of this system are getting wrong values which are
  specially sensible on variables with a range between 0 and 1.

  * The URDF schema defines doubles in URDF as xs:double. Looking at the
  XSD document it says that the mantissa for a double is represented as
  a "decimal" number. And looking at the XSD document a decimal number
  is a "finite-length sequence of decimal digits (#x30-#x39) separated
  by a period as a decimal indicator". Thus, the locale should have no
  effect on how the document is parsed, and using std::stod is
  incorrect. This patch switches the Vector3 class to using the stream
  operator, which seems to ignore locale.

  * urdfdom-headers is a key part for the largest open source robotics
  framework, ROS (Robot Operative System). So it potentially affects a
  big part of the robotics open source community. We (the devs and
  maintainers of ROS have been receiving many different issues or bugs
  coming from this problem, as Simon wrote in the initial post of this
  bug.

  [Test Case]

  * We have prepared a test case that demonstrate the problem and the fix. Patched packages for both urdfdom and urdfdom_headers are in this ppa:
  https://launchpad.net/~j-rivero/+archive/ubuntu/urdfdom-headers-sru2/+packages

  * The dockerfile here https://github.com/j-rivero/sru_urdfdom/blob/master/Dockerfile tries to demonstrate the following:
   - Setup the test case, install packages
   - Import locale test from the latest development of urdfdom repo
   - Run tests, failed (demonstration of the bug)
   - Setup the PPA, install new version of packages
   - Run test, success (demonstration of the fix)

  * This is the testing build of the dockerfile
  https://build.osrfoundation.org/job/_urdfdom_sru/15/console

  [Regression Potential]

  * The change affected the result of parsed values from using stof/stod
  functions to use stringstream. Although the patch is not small, the
  same pattern is replied on every chunk of code changed. Standard use
  case of users setting up digits in the form of integer or double is
  unlikely to generate big regressions. Alternative use cases of users
  with weird values in the input would trigger an exception from the
  code. This is no different than the exceptions thrown by previous
  stof/stod funtions (std::invalid_argument or std::out_of_range).

  * The bug was reported in Nov 2017
  (https://github.com/ros/urdfdom_headers/issues/41) the ROS community
  and fixes landed in upstream repositories few weeks after. Many users
  from different countries have been using new versions or these patches
  since them, some of them are commenting/voting in this bug. Note that
  we were also playing with upstream test suite to build the test case
  and no regression was found in there.

  [Other Info]

  * Upstream releases new patched versions with the fix that I’ve
  included into Debian Sid and have been synced by Ubuntu starting from
  Disco: 1.0.3-1

  * debdiffs for versions from the PPA:
   - urdfdom debdiff:
     https://gist.github.com/j-rivero/9983dddc6dbbea99cd3c39774f1d0498

   - urdfdom-headers debdiff
     https://gist.github.com/j-rivero/bc563620802d78129d775b2d5aee69ac

  https://github.com/ros/urdfdom/commit/3dc7ee812827cc69ffa457ef01fe7b9623096aed
  https://github.com/ros/urdfdom_headers/commit/9d2b421f3fcbc2a32af40b99ebd9c2cb2d088fb9
  https://github.com/ros/urdfdom_headers/pull/42
  https://github.com/ros/urdfdom/pull/105
  https://github.com/ros/urdfdom_headers/pull/47
  https://github.com/ros/urdfdom/pull/115

  See also the following upstream bug reports:
  https://github.com/ros/urdfdom_headers/issues/45
  https://github.com/ros/urdfdom/issues/119

  and a few downstream bug reports to the ubuntu package:
  https://github.com/ros-planning/moveit/issues/1333
  https://github.com/ros/urdf/issues/21
  https://github.com/ros-visualization/rviz/issues/1249
  https://github.com/ros-visualization/rviz/issues/1151
  https://github.com/ros-planning/moveit/issues/1050
  https://github.com/ros-visualization/rviz/issues/1298

To manage notifications about this bug go to:
https://bugs.launchpad.net/urdfdom-headers/+bug/1817595/+subscriptions