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.

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

XmlRpcValue::_doubleFormat should be used during write. #2003

Merged
merged 9 commits into from
Sep 23, 2020

Conversation

fujitatomoya
Copy link
Contributor

fix #1958

Signed-off-by: Tomoya.Fujita Tomoya.Fujita@sony.com

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@peci1
Copy link
Contributor

peci1 commented Jul 28, 2020

This PR should contain a test that would verify the behavior.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Contributor Author

@dirk-thomas @peci1

could you take a look at it when you got time?

i confirmed catkin_make run_tests_xmlrpcpp_gtest_TestValues XmlRpc.testDouble got green light.

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Beside the inline comment it seems that some unit tests are failing now in the PR builds.

utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
@fujitatomoya
Copy link
Contributor Author

Beside the inline comment it seems that some unit tests are failing now in the PR builds.

I will check on those,

@peci1
Copy link
Contributor

peci1 commented Jul 29, 2020

As the double format is static, it is shared by all code. After your test finishes, you should set doubleformat back to the default "%.16g". This should get the other tests green.

@fujitatomoya
Copy link
Contributor Author

@peci1

nice catch!!! thanks for the information, that helps 😄 (actually i did not have time to look into...)

@fujitatomoya
Copy link
Contributor Author

local test shows green light,

# catkin_make run_tests_xmlrpcpp_gtest_TestValues
Base path: /root/catkin_ws
Source space: /root/catkin_ws/src
Build space: /root/catkin_ws/build
Devel space: /root/catkin_ws/devel
Install space: /root/catkin_ws/install
####
#### Running command: "make cmake_check_build_system" in "/root/catkin_ws/build"
####
####
#### Running command: "make run_tests_xmlrpcpp_gtest_TestValues -j24 -l24" in "/root/catkin_ws/build"
####
[ 50%] Built target gtest
[100%] Built target xmlrpcpp
[100%] Built target TestValues
-- run_tests.py: execute commands
  /root/catkin_ws/devel/lib/xmlrpcpp/TestValues --gtest_output=xml:/root/catkin_ws/build/test_results/xmlrpcpp/gtest-TestValues.xml
[==========] Running 12 tests from 2 test suites.
[----------] Global test environment set-up.
[----------] 11 tests from XmlRpc
[ RUN      ] XmlRpc.Bool
[       OK ] XmlRpc.Bool (0 ms)
[ RUN      ] XmlRpc.testBoolean
[       OK ] XmlRpc.testBoolean (0 ms)
[ RUN      ] XmlRpc.testInt
[       OK ] XmlRpc.testInt (0 ms)
[ RUN      ] XmlRpc.testDouble
[       OK ] XmlRpc.testDouble (0 ms)
[ RUN      ] XmlRpc.testString
[       OK ] XmlRpc.testString (0 ms)
[ RUN      ] XmlRpc.testDateTime
[       OK ] XmlRpc.testDateTime (0 ms)
[ RUN      ] XmlRpc.testArray
[       OK ] XmlRpc.testArray (1 ms)
[ RUN      ] XmlRpc.testStruct
[       OK ] XmlRpc.testStruct (5 ms)
[ RUN      ] XmlRpc.base64
[       OK ] XmlRpc.base64 (0 ms)
[ RUN      ] XmlRpc.int_errors
[       OK ] XmlRpc.int_errors (0 ms)
[ RUN      ] XmlRpc.array_errors
[       OK ] XmlRpc.array_errors (0 ms)
[----------] 11 tests from XmlRpc (6 ms total)

[----------] 1 test from XmpRpc
[ RUN      ] XmpRpc.errors
[       OK ] XmpRpc.errors (0 ms)
[----------] 1 test from XmpRpc (0 ms total)

[----------] Global test environment tear-down
[==========] 12 tests from 2 test suites ran. (6 ms total)
[  PASSED  ] 12 tests.
-- run_tests.py: verify result "/root/catkin_ws/build/test_results/xmlrpcpp/gtest-TestValues.xml"
[100%] Built target run_tests_xmlrpcpp_gtest_TestValues

@fujitatomoya
Copy link
Contributor Author

@dirk-thomas

thanks for checking on this. we got all green, we have a go?

and would you want me to work on backport for melodic?
I am willing to do that, just let me know which version you want this to be put back.

@dirk-thomas
Copy link
Member

and would you want me to work on backport for melodic?

Not need to. Any backport will happen in a combined PR as described in #1496.

…est.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya force-pushed the topic-20200721-fix-1958 branch from e2ec880 to 5ee3a8d Compare August 4, 2020 03:11
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Contributor Author

fujitatomoya commented Aug 4, 2020

@dirk-thomas

could you take a look when you got time?

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
@fujitatomoya
Copy link
Contributor Author

@dirk-thomas

could you check when you got time? thanks!

@fujitatomoya
Copy link
Contributor Author

@dirk-thomas

ping friendly,

@fujitatomoya
Copy link
Contributor Author

@dirk-thomas

i think this one was missed under your radar, ping friendly 😄

utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
utilities/xmlrpcpp/src/XmlRpcValue.cpp Outdated Show resolved Hide resolved
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Contributor Author

@dirk-thomas

I've addressed your comments, could you take a look?

@dirk-thomas
Copy link
Member

Thanks for the patch.

@dirk-thomas dirk-thomas merged commit 44fa5cf into ros:noetic-devel Sep 23, 2020
jacobperron pushed a commit that referenced this pull request Oct 16, 2020
* XmlRpcValue::_doubleFormat should be used during write.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* allocate buffer dynamically for XmlRpcValue::_doubleFormat if necessary.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* add test for XmlRpcValue::_doubleFormat.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* check return code from std::snprintf, save/restore DoubleFormat for test.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* add one time warning message for DoubleFormat.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* use XmlRpcUtil::error instead of ROS_ERROR.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* use static_cast and minor fixes.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* delete unrelated change and fix invalid format case.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

* get rid of redundant condition from if statement.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
Tobias-Fischer added a commit to RoboStack/ros-noetic that referenced this pull request Dec 7, 2020
nim65s added a commit to nim65s/robotpkg that referenced this pull request Mar 12, 2021
Because DEPEND_ABI.ros-comm.noetic?= ros-comm>=1.15

1.15.9 (2020-10-16)
-------------------
* Fix deadlock when service connection is dropped (ros/ros_comm#2074)
* Update maintainers (ros/ros_comm#2075)
* Fix case where accessing cached parameters shuts down another node (ros/ros_comm#2068)
* Fix spelling (ros/ros_comm#2066)
* Fix Lost Wake Bug in ROSOutAppender (ros/ros_comm#2033)
* Fix compatibility issue with boost 1.73 and above (ros/ros_comm#2023)
* Gracefully stop recording upon SIGTERM and SIGINT (ros/ros_comm#2038)
* Use heapq.merge instead of custom merge sort code (ros/ros_comm#2017)
* Fix handling of single quotes in command arguments on Windows (ros/ros_comm#2051)
* Clearer error message (ros/ros_comm#2035)
* Ignore underscores when parsing literal numeric values for Python 3 compatibility (ros/ros_comm#2022)
* Clear cached URI for a node that has gone offline (ros/ros_comm#2010)
* Add skip_cache parameter to rosnode_ping() (ros/ros_comm#2009)
* Install advertisetest (ros/ros_comm#2046)
* Use range instead of xrange for Python 3 compatibility (ros/ros_comm#2013)
* Fix to address CVE-2020-16124 (ros/ros_comm#2065)
* Fix XmlRpcValue::_doubleFormat being unused (ros/ros_comm#2003)

1.15.8 (2020-07-23)
-------------------
* change is_async_connected to use epoll when available (ros/ros_comm#1983)
* allow mixing latched and unlatched publishers (ros/ros_comm#1991)
* remove not existing NodeProxy from rospy __all_\_ (ros/ros_comm#2007)
* fix typo in topics.py (ros/ros_comm#1977)
* fix bad relative import (still Python 2 style) (ros/ros_comm#1973)
* improve shutdown message with duplicate node name (ros/ros_comm#1992)
* remove dependency on rostopic from rostest package (ros/ros_comm#2002)
* fix missing reload() function in Python 3 (ros/ros_comm#1968)
* add latch param to throttle (ros/ros_comm#1944)
* add const versions of XmlRpcValue converting operators (ros/ros_comm#1978)

1.15.7 (2020-05-28)
-------------------
* fix Windows build break (ros/ros_comm#1961)
* fix NameError in launch error handling (ros/ros_comm#1965)

1.15.6 (2020-05-21)
-------------------
* fix a bug that using a destroyed connection object (ros/ros_comm#1950)

1.15.5 (2020-05-15)
-------------------
* check if async socket connect is success or failure before TransportTCP::read() and TransportTCP::write() (ros/ros_comm#1954)
* fix bug that connection drop signal related funtion throw a bad_weak exception (ros/ros_comm#1940)
* multiple latched publishers per process on the same topic (ros/ros_comm#1544)
* fix negative numbers in ros statistics (ros/ros_comm#1531)
* remove extra \n in ROS_DEBUG (ros/ros_comm#1925)
* add option to repeat latched messages at the start of bag splits (ros/ros_comm#1850)
* fix bag migration failures caused by typo in connection_header assignment (ros/ros_comm#1952)
* fix brief description comments after members (ros/ros_comm#1920)
* add --sigint-timeout and --sigterm-timeout parameters (ros/ros_comm#1937)
* roslaunch-check: search dir recursively (ros/ros_comm#1914)
* sort printed nodes by namespace alphabetically (ros/ros_comm#1934)
* remove pycrypto import (not used) (ros/ros_comm#1922)
* avoid infinite recursion in rosrun tab completion when rosbash is not installed (ros/ros_comm#1948)
* fix bare pointer in topic_tools::ShapeShifter (ros/ros_comm#1722)
* clear message queue on simtime jumping back (ros/ros_comm#1518)
* use undefined dynamic_lookup on macOS (ros/ros_comm#1923)
* check if enough FDs are free, instead counting the total free FDs (ros/ros_comm#1929)

1.15.4 (2020-03-19)
-------------------
* restrict boost dependencies to components used (ros/ros_comm#1871)
* add exception for ConnectionAbortedError (ros/ros_comm#1908)
* fix mac trying to use epoll instead of kqueue (ros/ros_comm#1907)
* fix AttributeError: __exit__ (ros/ros_comm#1915, regression from 1.14.4)

1.15.3 (2020-02-28)
-------------------
* remove Boost version check since Noetic only targets platforms with 1.67+ (ros/ros_comm#1903)

1.15.2 (2020-02-25)
-------------------
* export missing Boost dependency (ros/ros_comm#1898)
* add timestamp formatting for rosconsole (ros/ros_comm#1892)

1.15.1 (2020-02-24)
-------------------
* fix missing boost dependencies (ros/ros_comm#1895)
* use setuptools instead of distutils (ros/ros_comm#1870)
* increase time limit of advertisetest/publishtest.test to reduce flakyness (ros/ros_comm#1897)

1.15.0 (2020-02-21)
-------------------
* fix dictionary changed size during iteration (ros/ros_comm#1894)
* update test to pass with old and new yaml (ros/ros_comm#1893)

Packaging changes:
- removed patch-an, as there are no more boost version checks
- updated patch-ao
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XmlRpcValue ignores the value of _doubleFormat
3 participants