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.

Sign up for GitHub

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

Jump to bottom

Group state locale #44

Merged
merged 7 commits into from
Feb 23, 2019
Merged

Group state locale #44

merged 7 commits into from
Feb 23, 2019

Conversation

simonschmeisser
Copy link
Contributor

when parsing the group state std::stod is used to convert from string to double which uses the c locale and thus assumes , as a decimal separator for some languages.

This PR resurrects the cpp unit tests, ports them to gtest, runs them with classic and Dutch locale and finally removes the call to std::stod

In order for the first two commits to fail during unit testing somewhere sudo locale-gen nl_NL.UTF-8 needs to be added, @rhaschke @davetcoleman you probably know where

rhaschke
rhaschke reviewed Feb 15, 2019
View reviewed changes
test/test_parser.cpp
#define EXPECT_TRUE(arg) \
if (!(arg)) \
throw std::runtime_error("Assertion failed at line " + std::to_string(__LINE__))
#include <gtest/gtest.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for migration to gtest!

rhaschke
rhaschke reviewed Feb 15, 2019
View reviewed changes
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Apart from clang-format issues, looks good to me. Thanks for fixing this throughout whole ROS 😉

test/srdf_parser_cpp.test
<env name="LC_ALL" value="C" />
</test>
<test test-name="test_srdf_parser_cpp_locale" pkg="srdfdom" type="test_cpp">
<env name="LC_ALL" value="nl_NL.UTF-8" />
Copy link
Contributor

Choose a reason for hiding this comment

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

In order for the first two commits to fail during unit testing somewhere sudo locale-gen nl_NL.UTF-8 needs to be added, @rhaschke @davetcoleman you probably know where

I don't get this comment. Isn't this env setting correctly setting the locale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but in order for the env variable to have an effect the locale needs to be created first

see ros/urdfdom@1857a55 for an example without using moveit-ci

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I added an appropriate command to .travis.yml

@rhaschke
Copy link
Contributor

Could you please squash-in the clang-format fix I pushed (I cannot rebase your branch myself). Then this PR should be ready for merging.

@simonschmeisser
Copy link
Contributor Author

I squashed/fixuped the two commits marked fixup

@rhaschke
Copy link
Contributor

Thanks. Let's wait for Travis now...

@rhaschke
Copy link
Contributor

@simonschmeisser Could you please have another look. One unit test failed.

@simonschmeisser
Copy link
Contributor Author

It works here, I have no idea, sry

@rhaschke
Copy link
Contributor

Ok. Needs to postpone this for the weekend then.

simonschmeisser and others added 4 commits February 15, 2019 13:48
@simonschmeisser
Reenable cpp test
38381c1
@simonschmeisser
Test with both no locale and Dutch locale
f0e2f94 
note that localegen needs to be run for this to have any effect
sudo locale-gen nl_NL.UTF-8

see urdfdom test for example ros/urdfdom@1857a55
@simonschmeisser
Don't assume group positions to be localized
82a5b26
@rhaschke @simonschmeisser
create locale for parser test
b3a9408
@rhaschke
Copy link
Contributor

Looks like urdfdom-1.0.0 cannot parse with the locale set. Probably you are already using the newer version of urdfdom locally (as I do).

# export LC_ALL=nl_NL.UTF-8; devel/lib/srdfdom/test_cpp 
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from TestCpp
[ RUN      ] TestCpp.testSimple
Error:   Unable to parse component [.001] to a double (while parsing a vector value)
         at line 157 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/link.cpp
Error:   Could not parse collision element for Link [base_laser_link]
         at line 517 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/link.cpp
Error:   Unable to parse component [.001] to a double (while parsing a vector value)
         at line 157 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/link.cpp
Error:   Could not parse collision element for Link [imu_link]
         at line 517 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/link.cpp
Error:   Unable to parse component [.001] to a double (while parsing a vector value)
         at line 157 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/link.cpp
Error:   Could not parse collision element for Link [laser_tilt_link]
         at line 517 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/link.cpp
Error:   Unable to parse component [.0244] to a double (while parsing a vector value)
         at line 102 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/pose.cpp
Error:   Malformed parent origin element for joint [r_gripper_led_joint]
         at line 473 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/joint.cpp
Error:   joint xml is not initialized correctly
         at line 206 in /build/urdfdom-ACOcA8/urdfdom-1.0.0/urdf_parser/src/model.cpp
/root/ws_moveit/src/srdfdom/test/test_parser.cpp:74: Failure
Value of: u != NULL
  Actual: false
Expected: true
Segmentation fault (core dumped)

I fixed this issue by temporarily setting the C locale for parsing the URDF.

If an unknown locale is set, just a warning is printed, but the test simply passes. There should be at least a warning, e.g. from cmake, that the locale is not available. Otherwise the test is useless.

$ export LC_ALL=foo; devel/lib/srdfdom/test_cpp
bash: warning: setlocale: LC_ALL: cannot change locale (foo): No such file or directory
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from TestCpp
[ RUN      ] TestCpp.testSimple
[       OK ] TestCpp.testSimple (13 ms)
[ RUN      ] TestCpp.testComplex
[       OK ] TestCpp.testComplex (6 ms)
[----------] 2 tests from TestCpp (19 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (19 ms total)
[  PASSED  ] 2 tests.

@simonschmeisser Can you please extend the cmake file accordingly?

@simonschmeisser
Copy link
Contributor Author

Thanks! Good catch, I'll add a warning on Monday. I should also file a ubuntu bug report to ask for a patch version update now that OSRF finally managed to have releases of urdfdom(_headers) ...

@simonschmeisser
Copy link
Contributor Author

@rhaschke I searched a bit on Monday but did not find a solution for adding this to cmake. If you know a way please feel invited to add it, otherwise I would suggest to merge and add an issue

@rhaschke rhaschke merged commit a4564a5 into moveit:melodic-devel Feb 23, 2019
@rhaschke
Copy link
Contributor

I added a simple check for the locale to be available in cmake and squash-merged.

@simonschmeisser
Copy link
Contributor Author

thanks!

@RoboticsYY RoboticsYY mentioned this pull request Nov 17, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhaschke rhaschke rhaschke left review comments

@davetcoleman davetcoleman davetcoleman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@simonschmeisser @rhaschke @davetcoleman