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

Mathew Hodson mathew.hodson at gmail.com
Wed Aug 14 02:06:08 UTC 2019 

** Description changed:

- In the effort to reduce boost dependency in urdfdom and urdfdom-headers
- [2] regressions wrt locale handling were introduced. Parsing of floating
- point values in URDF (a XML language) was now locale dependent, ie on
- European systems were the decimal separator is "," instead of "."
- parsing would now fail due to std::stod expecting "1,23" instead of
- "1.23" (no, we do not localize xml files, see also [3] for a more
- technical summary). The regressed versions were released as 1.0. Shortly
- after the release (some of) the regressions were detected [3] and fixes
- merged [3][4]. Unfortunately those fixed merges were not released until
- a long time later, too late for Ubuntu 18.04 by far and potentially even
- for 20.04. More regressions were noticed and fixed later on (disclaimer:
- by me) [5].
+ [Impact]
  
- urdfdom is used mostly if not exclusively in the ROS community
- (including gazebo) which is a somewhat slow adopter of newer Ubuntu (and
- ros) releases but more and more people are now seeing these regressions
- in various places. To save more peoples time I propose to do a patch
- version update and pull urdfdom_headers version 1.0.3 as well as urdfdom
- version 1.0.3 from debian sid. I argue against applying a patch because
- the diff between 1.0.0 and 1.0.3 is almost identical to a set of patches
- fixing the regressions. Updating to the upstream patch release will
- therefore improve tracing of the ubuntu downstream version to the
- upstream versioning.
+ * 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.
  
- Both urdfdom_headers and urdfdom now have an extensive set of unit test
- which in the case of urdfdom are also run in a European (ie Dutch)
- locale [6]. Together with the small and homogeneous user base,
- application mostly in research and the fact that most of the library is
- (for unknown reasons) almost header-only make me estimate that scope,
- potential and impact of regressions introduced by this upgrade would be
- extremely low.
+ * 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.
  
- 1: https://github.com/ros/urdfdom/commit/3dc7ee812827cc69ffa457ef01fe7b9623096aed
- 2: https://github.com/ros/urdfdom_headers/commit/9d2b421f3fcbc2a32af40b99ebd9c2cb2d088fb9
- 3: https://github.com/ros/urdfdom_headers/pull/42
- 4: https://github.com/ros/urdfdom/pull/105
- 5: https://github.com/ros/urdfdom_headers/pull/47
- 6: https://github.com/ros/urdfdom/pull/115
+ * 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

** Also affects: urdfdom-headers (Ubuntu Bionic)
   Importance: Undecided
       Status: New

** Also affects: urdfdom (Ubuntu Bionic)
   Importance: Undecided
       Status: New

** Changed in: urdfdom (Ubuntu)
       Status: Confirmed => Fix Released

** Changed in: urdfdom-headers (Ubuntu)
       Status: Confirmed => Fix Released

-- 
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:
  New
Status in urdfdom-headers source package in Bionic:
  New

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

More information about the Ubuntu-sponsors mailing list