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

export dll on Windows #373

Merged

Conversation

kejxu
Copy link
Contributor

@kejxu kejxu commented Feb 1, 2019

.so are exported on Linux using LIBRARY property, but .dll needs to use RUNTIME. to fix this, we could remove the LIBRARY specification, and export all the output to package_lib, so library and runtime are all exported

or we could add RUNTIME DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}

same change as in ros-controls/realtime_tools#33

Copy link
Contributor

@mathias-luedtke mathias-luedtke left a comment

Choose a reason for hiding this comment

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

Our CMakeLists.txt follows the recommendations.
I don't know what implications the change would have.

@traversaro
Copy link

traversaro commented Feb 1, 2019

or we could add RUNTIME DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}

Just a small question: the CMake documentation recommends to install RUNTIME library targets in the bin subdirectory of the install folder (see https://cmake.org/cmake/help/v3.13/manual/cmake-packages.7.html) and the vcpkg package manager from Microsoft actually rejects with a check packages that place their .dll files in the lib folder (see https://github.com/Microsoft/vcpkg/blob/5ff5ab7ae29313e95eab07172721800c5fa84c0c/toolsrc/src/vcpkg/postbuildlint.cpp#L206).
This convention is used in most existing C++ libraries, see for example OpenCV (https://github.com/opencv/opencv/blob/8f4e5c2fb8d473c69a3b14983583332738d33372/cmake/OpenCVModule.cmake#L1001) or the gazebo-related ignition libraries developed by Open Robotics (see https://bitbucket.org/ignitionrobotics/ign-cmake/src/3629811d05b872b3d98f1937694984b9a6cc600e/cmake/IgnUtils.cmake#lines-1260).

There is any reason for not following a relatively well established convention and installing .dll files in lib folder instead?

@traversaro
Copy link

traversaro commented Feb 1, 2019

Actually I noticed that catkin documentation itself seems to recommend to install RUNTIME libraries in the ${CATKIN_GLOBAL_BIN_DESTINATION} location: http://docs.ros.org/melodic/api/catkin/html/howto/format2/building_libraries.html

@mathias-luedtke
Copy link
Contributor

If it helps, we can extend it to

install(TARGETS ${PROJECT_NAME}
        ARCHIVE DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}
        LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}
        RUNTIME DESTINATION ${CATKIN_GLOBAL_BIN_DESTINATION})

But not

or we could add RUNTIME DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}

@seanyen
Copy link

seanyen commented Feb 1, 2019

There is any reason for not following a relatively well established convention and installing .dll files in lib folder instead?

@traversaro For those changes in ros-controls, I think it doesn't hurt to follow the guidance to place them to ${CATKIN_GLOBAL_BIN_DESTINATION}. However, JFYI, I noticed in some packages (not in this case), they create shared libs as plugins and the plugin location managed by plugin.xml expect them to be under ${CATKIN_PACKAGE_LIB_DESTINATION}. (One example, https://github.com/PickNikRobotics/rviz_visual_tools/blob/melodic-devel/plugin_description.xml)

@kejxu
Copy link
Contributor Author

kejxu commented Feb 1, 2019

There is any reason for not following a relatively well established convention and installing .dll files in lib folder instead?

@traversaro For those changes in ros-controls, I think it doesn't hurt to follow the guidance to place them to ${CATKIN_GLOBAL_BIN_DESTINATION}. However, JFYI, I noticed in some packages (not in this case), they create shared libs as plugins and the plugin location managed by plugin.xml expect them to be under ${CATKIN_PACKAGE_LIB_DESTINATION}.

Thanks for helping out and elaborate on this Sean! @traversaro and @ipa-mdl, this is an example Sean showed to me earlier, maybe you'd also find it helpful =)

@traversaro
Copy link

traversaro commented Feb 1, 2019

Thanks for the answers @seanyen-msft and @kejxu !

However, JFYI, I noticed in some packages (not in this case), they create shared libs as plugins and the plugin location managed by plugin.xml expect them to be under ${CATKIN_PACKAGE_LIB_DESTINATION}. (One example, https://github.com/PickNikRobotics/rviz_visual_tools/blob/melodic-devel/plugin_description.xml)

I tried to understand why the plugin needs to be located in <prefix>/lib, and I found the core reason is the following plugin's lib function:
https://github.com/ros/pluginlib/blob/0a3c0de442596b46026eb4c738130bce90421927/pluginlib/include/pluginlib/class_loader_imp.hpp#L302
in which the directories in which plugins are searched are just obtained by adding /lib to each path contained in the CMAKE_PREFIX_PATH .

Interestingly, in the ROS2's branch, the equivalent function has been modified to search also in the <prefix>/bin directories, see this PR: ros/pluginlib#97 .

I noticed that in the ROSOnWindows's fork of pluginlib, an equivalent change has been committed in ms-iot/pluginlib#7 . So, as far as I understand now it should be ok to install .dll to /bin in all cases, (even in https://github.com/PickNikRobotics/rviz_visual_tools/blob/melodic-devel/plugin_description.xml) thanks to the fix provided in ms-iot/pluginlib#7 , but perhaps I am missing something.

@seanyen
Copy link

seanyen commented Feb 1, 2019

Interestingly, in the ROS2's branch, the equivalent function has been modified to search also in the /bin directories, see this PR: ros/pluginlib#97 .

@traversaro thanks to point out this change! Sounds like we are converging to some similar logic.

(Sorry for hijacking this PR for discussion) @wjwwood as @traversaro pointed out, it looks like in ROS2 you are also hitting the problem that placing plugin DLL to global path won't be searched by plugin loader on Windows. Since both of our solutions are similar (ms-iot/pluginlib#7), as we move forward, are we safe to say that now the general guidance for Windows (for both ROS1 and ROS2) are like:

  • For executables, ROS recommends to place them under ${CATKIN_PACKAGE_BIN_DESTINATION}. ref
  • For DLL, ROS recommends to place them under ${CATKIN_GLOBAL_BIN_DESTINATION}, no matter it is a plugin or just a regular shared library. ref

@wjwwood
Copy link
Member

wjwwood commented Feb 1, 2019

In ROS 2, there's no CATKIN_* variables. See rviz for an example with both shared libraries and plugins:

Pluginlib was modified in ROS 2 to search in bin as well as lib:

@bmagyar
Copy link
Member

bmagyar commented Feb 3, 2019

I appreciate you working on this and bringing ros_control to more users however I would be reluctant to claim Windows support until we have some sort of CI backing it up. I have no time or instruments to test anything on Windows, without a CI to help out we may keep breaking platform compatibility.

Do you have anything in store for that?

@kejxu
Copy link
Contributor Author

kejxu commented Feb 4, 2019

I appreciate you working on this and bringing ros_control to more users...

thanks for the input! before answer the CI question, here is a bit more justification to this pull request =)

this change and ros-controls/realtime_tools#33 are, as @ipa-mdl and @traversaro mentioned (thanks!), just minor fine-tuning following catkin recommendation. This change is needed so that runtime binaries would be exported on Windows (RUNTIME needs to be specified)

...however I would be reluctant to claim Windows support until we have some sort of CI backing it up. I have no time or instruments to test anything on Windows, without a CI to help out we may keep breaking platform compatibility.

Do you have anything in store for that?

as for Windows hosting (CI), at this point we are hosting a daily Windows build for major ROS distributions, which includes desktop_full. This build currently retrieves source from ms-iot:init_windows, yet we will soon deploy another pipeline for ros-controls:melodic-devel =)

at the same time, an Azure Pipelines based CI for PR is on our planning, and we will start working on that once the bandwidth allows.

Copy link
Contributor

@mathias-luedtke mathias-luedtke left a comment

Choose a reason for hiding this comment

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

The is safe to merge :)

@mathias-luedtke mathias-luedtke merged commit 1accf30 into ros-controls:melodic-devel Feb 4, 2019
@kejxu kejxu deleted the export_dll_on_windows branch February 4, 2019 22:52
nim65s pushed a commit to nim65s/robotpkg that referenced this pull request Mar 9, 2020
Changelog for package joint_limits_interface

0.17.0 (2020-02-24)
-------------------
* Use default member initializers
* Use braces for member initializers
* Replace boost with std
* Replace boost ptrs with std ptrs in documentation
* Prefer default member initializers
* Contributors: AbhinavSingh, Bence Magyar, Matt Reynolds

0.16.0 (2020-01-27)
-------------------
* Use more meaningful pair iterator names
* Use range-based for loops in joint_limits_interface
* Update dependencies
  - Dependencies needed to compile are <build_depend>
  - Dependencies used in public headers are <build_export_depend>
  - Dependencies needed to link or run are <exec_depend>
* Update package dependencies
* Remove liburdfdom-dev
* Add missing roscpp & rospy dependencies
* Remove rosunit test_depend from package.xml
* Prefer 0.0 for floating point literals
* Replace header guard with #pragma once
* joint_limits: use an open-loop policy for velocity staturation
  The feedback from the controller is way too slow to be used on an
  actual robot. A robot that had 15 rad.s^-2 on each wheel as
  an acceleration limit could not even reach 2 rad.s^-2
  This is in line with ros_controllers`%23 <https://github.com/ros-controls/ros_control/issues/23>`_
* Apply consistent style to CMakeLists.txt files
* Apply consistent style to package.xml files
* Fix shadowed variables
* -Werror=overloaded-virtual and initialization of fields in constructor
* Contributors: Bence Magyar, Daniel Pinyol, Matt Reynolds, Paul Mathieu, Victor Lopez

Changelog for package hardware_interface

0.17.0 (2020-02-24)
-------------------
* Use default member initializers
* Use braces for member initializers
* Replace boost with std
* Replace boost::ptr_vector<T> with std::vector<T*>
* Contributors: AbhinavSingh, Bence Magyar, Matt Reynolds

0.16.0 (2020-01-27)
-------------------
* Use more meaningful pair iterator names
* Correct typo in interface_manager.h
  Co-Authored-By: Bence Magyar <bence.magyar.robotics@gmail.com>
* Use range-based for loops in hardware_interface
* Resolve Boost dependency issues
* Update dependencies
  - Dependencies needed to compile are <build_depend>
  - Dependencies used in public headers are <build_export_depend>
  - Dependencies needed to link or run are <exec_depend>
* Update package dependencies
* Remove rosunit test_depend from package.xml
* Use nullptr in tests
* Prefer nullptr for null pointers
* Replace header guard with #pragma once
* Extend joint mode interface
* Add torque sensor and absolute encoder support to transmissions and adjust tests
  Add pointer accessors for torque sensor and absoute position encoders
* Modified structures to have absolute encoder and torque sensor parameters
* Fix argument types to use enum
* hardware_interface: fix initialization order
* Created new hardware interface for switching between controller modes
* Remove unnecessary  rosunit/rostest dependencies
* Add missing Boost dependency
* Remove redundant rosconsole dependency
* Apply consistent style to CMakeLists.txt files
* Apply consistent style to package.xml files
* Fix compiler warnings
  - Comment out unused parameters
  - Make some integer literals unsigned to avoid comparison between signed and unsigned
  - Remove unnecessary semicolons
  - Make const void return type to void
* Revert CMake include_directories as SYSTEM
* Fix shadowed variables
* Multi-update cycle mode switch (`%391 <https://github.com/ros-controls/ros_control/issues/391>`_)
  For more info: pal-robotics-forks/ros_control2#5
  * Added tests for ControllerManager update
  * Mocks for controllers and controller loader in update test
  * Divided in tests with and without controllers
  * Controller state initialized in mock
  * Moved mocks to test class
  * All tests using mock class
  * Test for multiple updates in a single controller
  * Added new switchResult() function to RobotHW interface
  ControllerManager uses this function to wait for the result of the
  doSwitch() before starting the new set of controllers
  * Using ranged based loops
  * Switch is now managed in a separate function
  * Option to start controllers as soon as their joints are ready after a switch
  * Tests for controller_interface API
  * Added new STOPPED, WAITING and ABORTED states to ControllerBase
  * Split manageSwitch() into smaller functions
  * Abort pending controllers in case of switch error
  * Changed default behaviour of new switch param
  This way if it not set it will be the same behaviour as previous version
  * Added timeout parameter to switch controller
  * Removed unnecessary includes
  * Using target_include_directories for the test
  * std::all_of instead of std::count_if
  * Deleted autogenerated file
  * Adapted tests to changes in controller_manager
  * Adapted python implementation to new parameters in SwitchController
  * Added missing parameter description docstring
  * Moved all parameters used for switching to a separate object
  * Moved error messages to controller_base
  * State check functions are now const
  * Removed unnecessary comments
  * Added constants for start_asap and timeout default parameters values
* Option to start controllers as soon as their joints are ready after a switch
* Switch is now managed in a separate function
* Added new switchResult() function to RobotHW interface
  ControllerManager uses this function to wait for the result of the
  doSwitch() before starting the new set of controllers
* Contributors: Bence Magyar, Dave Coleman, Hilario Tome, Jordan Palacios, Markus Vieth, Matt Reynolds, Paul Mathieu, Victor Lopez

Changelog for package controller_manager

0.17.0 (2020-02-24)
-------------------
* Use auto keyword
* Use default member initializers
* Replace boost with std
* Replace boost mutexes & locks with std
* Replace boost::bind with std::bind
* Contributors: AbhinavSingh, Bence Magyar, Matt Reynolds, suab321321

0.16.0 (2020-01-27)
-------------------
* Use range-based for loops in controller_manager
* Resolve Boost dependency issues
* Update dependencies
  - Dependencies needed to compile are <build_depend>
  - Dependencies used in public headers are <build_export_depend>
  - Dependencies needed to link or run are <exec_depend>
* Add ${catkin_EXPORTED_TARGETS} dependencies
* Update package dependencies
* Add missing roscpp & rospy dependencies
* Prefer nullptr for null pointers
* Replace header guard with #pragma once
* Apply consistent style to CMakeLists.txt files
* Apply consistent style to package.xml files
* Fix build error in clang error: non-aggregate type 'std::vector' (aka 'vector >') cannot be initialized with an initializer list
* Multi-update cycle mode switch (`%391 <https://github.com/ros-controls/ros_control/issues/391>`_)
  For more info: pal-robotics-forks/ros_control2#5
  * Added tests for ControllerManager update
  * Mocks for controllers and controller loader in update test
  * Divided in tests with and without controllers
  * Controller state initialized in mock
  * Moved mocks to test class
  * All tests using mock class
  * Test for multiple updates in a single controller
  * Added new switchResult() function to RobotHW interface
  ControllerManager uses this function to wait for the result of the
  doSwitch() before starting the new set of controllers
  * Using ranged based loops
  * Switch is now managed in a separate function
  * Option to start controllers as soon as their joints are ready after a switch
  * Tests for controller_interface API
  * Added new STOPPED, WAITING and ABORTED states to ControllerBase
  * Split manageSwitch() into smaller functions
  * Abort pending controllers in case of switch error
  * Changed default behaviour of new switch param
  This way if it not set it will be the same behaviour as previous version
  * Added timeout parameter to switch controller
  * Removed unnecessary includes
  * Using target_include_directories for the test
  * std::all_of instead of std::count_if
  * Deleted autogenerated file
  * Adapted tests to changes in controller_manager
  * Adapted python implementation to new parameters in SwitchController
  * Added missing parameter description docstring
  * Moved all parameters used for switching to a separate object
  * Moved error messages to controller_base
  * State check functions are now const
  * Removed unnecessary comments
  * Added constants for start_asap and timeout default parameters values
* Remove extra {
* Adding Pal Robotics to copyright notice
  Co-Authored-By: Bence Magyar <bence.magyar.robotics@gmail.com>
* Added constants for start_asap and timeout default parameters values
* Moved error messages to controller_base
* Moved all parameters used for switching to a separate object
* Added missing parameter description docstring
* Adapted python implementation to new parameters in SwitchController
* Deleted autogenerated file
* std::all_of instead of std::count_if
* Cosmetics
* Fixed lincenses
* Removed unnecessary includes
* Added timeout parameter to switch controller
* Changed default behaviour of new switch param
  This way if it not set it will be the same behaviour as previous version
* Abort pending controllers in case of switch error
* Split manageSwitch() into smaller functions
* Added new STOPPED, WAITING and ABORTED states to ControllerBase
* Option to start controllers as soon as their joints are ready after a switch
* Switch is now managed in a separate function
* Using ranged based loops
* Added new switchResult() function to RobotHW interface
  ControllerManager uses this function to wait for the result of the
  doSwitch() before starting the new set of controllers
* Test for multiple updates in a single controller
* All tests using mock class
* Moved mocks to test class
* Controller state initialized in mock
* Divided in tests with and without controllers
* Mocks for controllers and controller loader in update test
* Added tests for ControllerManager update
* fix install destination (`%377 <https://github.com/ros-controls/ros_control/issues/377>`_)
* catch ROSInterruptException
* specify RUNTIME DESTINATION for libraries (`%373 <https://github.com/ros-controls/ros_control/issues/373>`_)
  needed for exporting DLLs on Windows
* use this_thread::sleep_for instead of usleep (`%375 <https://github.com/ros-controls/ros_control/issues/375>`_)
* remove unused pthread.h
* Contributors: Bence Magyar, Gérald Lelong, James Xu, Jordan Palacios, Matt Reynolds, Victor Lopez, jordan-palacios

0.15.1 (2018-09-30)
-------------------
* Updated for compatibility with Python2 or Python3
* Initialize controller_manager node using init_node.
* back to Python3 prints, add '-s to remaining places
* pep8 styling
* added quotes in python code too, also changed python prints to rosconsole
* added quotes for controller name and controller type in warnings and errors
* Contributors: Daniel Ingram, Jasper Güldenstein, Stefan Profanter, Gennaro Raiola, Bence Magyar

Changelog for package combined_robot_hw

0.17.0 (2020-02-24)
-------------------
* Use auto keyword
* Use default member initializers
* Improve controller and resource filtering for CombinedRobotHW
* Prefer default member initializers
* Contributors: AbhinavSingh, Bence Magyar, Matt Reynolds, Toni Oliver

0.16.0 (2020-01-27)
-------------------
* Use range-based for loop
* Update dependencies
  - Dependencies needed to compile are <build_depend>
  - Dependencies used in public headers are <build_export_depend>
  - Dependencies needed to link or run are <exec_depend>
* Use #pragma once
* Replace header guard with #pragma once
* Remove unused Boost dependencies
* Apply consistent style to CMakeLists.txt files
* Apply consistent style to package.xml files
* Fix build error in clang error: non-aggregate type 'std::vector' (aka 'vector >') cannot be initialized with an initializer list
* fix install destination
* specify RUNTIME DESTINATION for libraries needed for exporting DLLs on Windows
* Contributors: Bence Magyar, James Xu, Matt Reynolds, Victor Lopez

Changelog for package transmission_interface

0.17.0 (2020-02-24)
-------------------
* Use default member initializers
* Use braces for member initializers
* Replace boost with std
* Replace boost ptrs with std ptrs in documentation
* Replace boost::lexical_cast<double> with std::stod
* Prefer default member initializers
* Use auto
* Contributors: AbhinavSingh, Bence Magyar, Matt Reynolds

0.16.0 (2020-01-27)
-------------------
* Use more meaningful pair iterator names
* Use range-based for loops in transmission_interface
* Resolve Boost dependency issues
* Update dependencies
  - Dependencies needed to compile are <build_depend>
  - Dependencies used in public headers are <build_export_depend>
  - Dependencies needed to link or run are <exec_depend>
* Update package dependencies
* Remove rosunit test_depend from package.xml
* Prefer nullptr for null pointers
  Use #pragma once
* Add missing header guard to loader_utils.h
* Replace header guard with #pragma once
* Add torque sensor and absolute encoder support to transmissions and adjust tests
  Add pointer accessors for torque sensor and absoute position encoders
* Modified structures to have absolute encoder and torque sensor parameters
* Fix transmission_interface dependencies
* Apply consistent style to CMakeLists.txt files
* Apply consistent style to package.xml files
* Fix compiler warnings
  - Comment out unused parameters
  - Make some integer literals unsigned to avoid comparison between signed and unsigned
  - Remove unnecessary semicolons
  - Make const void return type to void
* Fix build error in clang
  error: non-aggregate type 'std::vector' (aka 'vector >') cannot be initialized with an initializer list
* Fix typo in docs
* TransmissionParser private -> protected
* fix install destination (`%377 <https://github.com/ros-controls/ros_control/issues/377>`_)
* Contributors: Bence Magyar, Gennaro Raiola, Hilario Tome, James Xu, Markus Vieth, Matt Reynolds, Victor Lopez

Changelog for package controller_manager_msgs

0.16.0 (2020-01-27)
-------------------
* Update package dependencies
* Apply consistent style to CMakeLists.txt files
* Apply consistent style to package.xml files
* Fix exception module
* Contributors: Bence Magyar, Jordan Palacios, Matt Reynolds, Ryohei Ueda

Changelog for package controller_interface

0.17.0 (2020-02-24)
-------------------
* Use default member initializers
* Prefer default member initializers
* Contributors: Bence Magyar, Matt Reynolds

0.16.0 (2020-01-27)
-------------------
* Update dependencies
  - Dependencies needed to compile are <build_depend>
  - Dependencies used in public headers are <build_export_depend>
  - Dependencies needed to link or run are <exec_depend>
* Replace header guard with #pragma once
* Remove unnecessary  rosunit/rostest dependencies
* Apply consistent style to CMakeLists.txt files
* Apply consistent style to package.xml files
* Multi-update cycle mode switch (`%391 <https://github.com/ros-controls/ros_control/issues/391>`_)
  For more info: pal-robotics-forks/ros_control2#5
  * Added tests for ControllerManager update
  * Mocks for controllers and controller loader in update test
  * Divided in tests with and without controllers
  * Controller state initialized in mock
  * Moved mocks to test class
  * All tests using mock class
  * Test for multiple updates in a single controller
  * Added new switchResult() function to RobotHW interface
  ControllerManager uses this function to wait for the result of the
  doSwitch() before starting the new set of controllers
  * Using ranged based loops
  * Switch is now managed in a separate function
  * Option to start controllers as soon as their joints are ready after a switch
  * Tests for controller_interface API
  * Added new STOPPED, WAITING and ABORTED states to ControllerBase
  * Split manageSwitch() into smaller functions
  * Abort pending controllers in case of switch error
  * Changed default behaviour of new switch param
  This way if it not set it will be the same behaviour as previous version
  * Added timeout parameter to switch controller
  * Removed unnecessary includes
  * Using target_include_directories for the test
  * std::all_of instead of std::count_if
  * Deleted autogenerated file
  * Adapted tests to changes in controller_manager
  * Adapted python implementation to new parameters in SwitchController
  * Added missing parameter description docstring
  * Moved all parameters used for switching to a separate object
  * Moved error messages to controller_base
  * State check functions are now const
  * Removed unnecessary comments
  * Added constants for start_asap and timeout default parameters values
* Removed unnecessary comments
* State check functions are now const
* Moved error messages to controller_base
* Using target_include_directories for the test
* Fixed lincenses
* Removed unnecessary includes
* Abort pending controllers in case of switch error
* Added new STOPPED, WAITING and ABORTED states to ControllerBase
* Tests for controller_interface API
* Contributors: Bence Magyar, Jordan Palacios, Matt Reynolds, jordan-palacios

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Changelog for package rqt_controller_manager
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0.17.0 (2020-02-24)
-------------------
* added missing controller state: 'initialised'
* Contributors: ThibaultRouillard

0.16.0 (2020-01-27)
-------------------
* Update package dependencies
* Add missing roscpp & rospy dependencies
* Update package.xml descriptions
* Apply consistent style to CMakeLists.txt files
* Apply consistent style to package.xml files
* Contributors: Bence Magyar, Matt Reynolds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants