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

[rbdl-orb] Adding RBDL-ORB to vcpkg #18883

Merged
merged 20 commits into from
Aug 5, 2021
Merged

[rbdl-orb] Adding RBDL-ORB to vcpkg #18883

merged 20 commits into from
Aug 5, 2021

Conversation

ju6ge
Copy link
Contributor

@ju6ge ju6ge commented Jul 9, 2021

This PR adds the rbdl-orb library to vcpkg. Rbdl-orb is an updated version of rbdl, maintained by the robotics group at the University of Heidelberg.

RBDL-ORB has been tested on linux, windows and mac with x86 and x64.

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

@ghost
Copy link

ghost commented Jul 9, 2021

CLA assistant check
All CLA requirements met.

@PhoebeHui PhoebeHui changed the title Adding RBDL-ORB to vcpkg [rbdl-orb] Adding RBDL-ORB to vcpkg Jul 12, 2021
@PhoebeHui PhoebeHui added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jul 12, 2021
Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

@ju6ge, thanks for your contribution!

vcpkg_configure_cmake and vcpkg_install_cmake has been deprecated in favor of vcpkg_cmake_configure and [vcpkg_cmake_install] from the vcpkg-cmake port.

Could you update it? it requires to depend on vcpkg-cmake port.

ports/rbdl-orb/portfile.cmake Outdated Show resolved Hide resolved
ports/rbdl-orb/portfile.cmake Outdated Show resolved Hide resolved
ports/rbdl-orb/portfile.cmake Outdated Show resolved Hide resolved
ports/rbdl-orb/portfile.cmake Outdated Show resolved Hide resolved
ports/rbdl-orb/CONTROL Outdated Show resolved Hide resolved
@ju6ge
Copy link
Contributor Author

ju6ge commented Jul 12, 2021

Hey,

regarding your suggested updates.

Using the "vcpkg_from_github" to download the repository is not possible because it does not recursively clone the repository. We have put some dependencies that where previously directly vendored into the code base and moved them to a sub repository, to be better able to test them and keep the versions in sync.

I tried using the commands "vcpkg_cmake_configure" and "vcpkg_cmake_install" in both cases vcpkg tells me that it does not know these commands.

Building the tests is off by default, so it does not need to be specified explicitly ;) the boost dependencies are required by the urdfmodel addon. The luamodel and urdfmodel addons are regarded as quite important because they enable reading the models from these file types instead of having to construct the models using cpp code.

Moving to vcpkg.json should not be a problem.

Kind regards,
ju6ge

@PhoebeHui
Copy link
Contributor

@ju6ge, thanks for clarifying this!

We have put some dependencies that where previously directly vendored into the code base and moved them to a sub repository, to be better able to test them and keep the versions in sync.

Do mean the https://github.com/ORB-HD/URDF_Parser repo? if so, generally, we'd recommend to add URDF_Parser to vcpkg firstly, and then let rbdl-ord depend on URDF_Parser.

I tried using the commands "vcpkg_cmake_configure" and "vcpkg_cmake_install" in both cases vcpkg tells me that it does not know these commands.

You need add vcpkg-cmake port to dependency, eg: https://github.com/microsoft/vcpkg/blob/master/ports/recast/vcpkg.json

@ju6ge
Copy link
Contributor Author

ju6ge commented Jul 13, 2021

Hey so I updated to use vcpkg.json as well as the new cmake commands.

Regarding creating a separate urdfparser library in vcpkg. I think I should clarify that the "we" mostly means me. I work for the robotics research group at the university of Heidelberg while completing my masters degree. The original RBDL was written by Martin Felis as his doctoral thesis in our group and since then we have maintained our own fork because other researchers have added functionality, while Martins version has mostly stayed the same. We also have some other tools building uppon rbdl-orb and the reason we are interested in having rbdl-orb in vcpkg is to be able to get our tools running on windows. But since I am the only person actively doing all this work as a part time job, separating out the urdfparser as its own library would just make my workload explode. I would need to write more build files to import urdfparser on all systems (linux, mac, windows) instead of just having it build with the project. Which also would mean updating the CI of all downstream tools. This just increases the complexity while adding no real value at the moment. If we had a bigger team to deal with this I think it would be a sensible thing to do. But given that it is just me, keeping urdfparser in sync between rbdl-orb and our downstream tools is just best handled by using a subrepository which is already a lot better than having the code replicated in each repository directly and having to manually apply the changes to all repositories.

ports/rbdl-orb/vcpkg.json Outdated Show resolved Hide resolved
ports/rbdl-orb/portfile.cmake Outdated Show resolved Hide resolved
ports/rbdl-orb/portfile.cmake Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

We refuse to use any method other than vcpkg's internal download functions (such as vcpkg_from_github / vcpkg_from_gitlab / vcpkg_from_git / vcpkg_from_sourceforge / vcpkg_download_distfile) to download the source code package, so please check my change requests.

@ju6ge
Copy link
Contributor Author

ju6ge commented Jul 13, 2021

Hey @JackBoosY, thanks for the suggested update. I updated the PR and ran the auto-formatter.

@JackBoosY
Copy link
Contributor

separating out the urdfparser as its own library would just make my workload explode. I would need to write more build files to import urdfparser on all systems (linux, mac, windows) instead of just having it build with the project. Which also would mean updating the CI of all downstream tools. This just increases the complexity while adding no real value at the moment.
But given that it is just me, keeping urdfparser in sync between rbdl-orb and our downstream tools is just best handled by using a subrepository which is already a lot better than having the code replicated in each repository directly and having to manually apply the changes to all repositories.

We recommend separating dependent libraries and establishing dependencies on their use, but this does not mean that we must do this.
Many ports in vcpkg use internal third-party dependencies, and their dependencies are also downloaded through the download method provided by vcpkg. As my suggestion.

We can set the dependency between their versions by setting the REF value in vcpkg_from_github.

@ju6ge ju6ge requested a review from JackBoosY July 13, 2021 10:05
@ju6ge
Copy link
Contributor Author

ju6ge commented Jul 13, 2021

Okay so now it should be correct. I am only using the vcpkg_ commands to download the sources and place the URDF_Parser repo into the correct subdirectory within rbdl-orb \o/

@PhoebeHui
Copy link
Contributor

PhoebeHui commented Jul 14, 2021

@ju6ge, rbdl-orb failed on our CI testing, do you test them locally? please let me know if you need the CI failure logs.

@ju6ge
Copy link
Contributor Author

ju6ge commented Jul 14, 2021

@PhoebeHui i have a windows server 2019 instance on which I tested the install of the port with vcpkg for x64-windows and x86-windows both install fine with the most resent commit. I looked into the CI logs for x64-windows which mentions that the hashes don't match after I updated the port to point to newest version v3.0.0 commit. It mentions running ./vcpkg x-add-version --all which I already did so I am not sure why that failed.

On my linux workstation the install of x64-linux it also is failing because of some lua error. I have not gotten around to debugging that.

The main reason I want rbdl-orb in vcpkg is to have it on windows though, so that is more of my focus now. Using rbdl-orb in linux and oxs already works for me without vcpkg.

@PhoebeHui
Copy link
Contributor

@ju6ge rbdl-orb also failed with x64-uwp, arm_uwp, x64-windows-static, x64_linux, x64_osx and x64-windows_static_md.

It passed with x64-windows and arm64-windows.
For x86-windows, it needs execute './vcpkg x-add-version -overwrite-version rbdl-orb' after you committed all changes, it's used to update the baseline version, and should be updated when everything is done.

@JackBoosY
Copy link
Contributor

When building uwp:

     5>D:\installed\x64-uwp\include\eigen3\Eigen\src/Core/util/Meta.h(357,2): error C4996: 'std::equal_to<double>::result_type': warning STL4007: Many result_type typedefs and all argument_type, first_argument_type, and second_argument_type typedefs are deprecated in C++17. You can define _SILENCE_CXX17_ADAPTOR_TYPEDEFS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. (compiling source file D:\buildtrees\rbdl-orb\src\528962cc0b-f01583996e.clean\src\Joint.cc) [D:\buildtrees\rbdl-orb\x64-uwp-dbg\rbdl.vcxproj]

When building osx:

/Users/vagrant/Data/buildtrees/rbdl-orb/src/528962cc0b-f01583996e.clean/include/rbdl/rbdl_errors.h:40:23: error: exception specification of overriding function is more lax than base version
  virtual const char* what() const noexcept;
                      ^

@ju6ge
Copy link
Contributor Author

ju6ge commented Jul 15, 2021

When building uwp:

     5>D:\installed\x64-uwp\include\eigen3\Eigen\src/Core/util/Meta.h(357,2): error C4996: 'std::equal_to<double>::result_type': warning STL4007: Many result_type typedefs and all argument_type, first_argument_type, and second_argument_type typedefs are deprecated in C++17. You can define _SILENCE_CXX17_ADAPTOR_TYPEDEFS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. (compiling source file D:\buildtrees\rbdl-orb\src\528962cc0b-f01583996e.clean\src\Joint.cc) [D:\buildtrees\rbdl-orb\x64-uwp-dbg\rbdl.vcxproj]

That is just a warning, can you send me the entire log?


Nevermind got it working, the error was that the define needed to be declared in the build system not in an include header since the error originated from eigen.

I think now all checks should pass, fingers crossed 🤞

@PhoebeHui
Copy link
Contributor

@ju6ge, I noticed that the cmake warning generated with x64-uwp and arm-uwp in CI testing, does this expect?
I double checked there is no pdbs generated for release configuration, however, for debug configuration, it has these pdbs.

CMake Warning at scripts/cmake/vcpkg_copy_pdbs.cmake:69 (message):
  Could not find a matching pdb file
  for:D:/packages/rbdl-orb_x64-uwp/bin/rbdl.dll

      D:/packages/rbdl-orb_x64-uwp/bin/rbdl_geometry.dll
      D:/packages/rbdl-orb_x64-uwp/bin/rbdl_luamodel.dll
      D:/packages/rbdl-orb_x64-uwp/bin/rbdl_urdfreader.dll

@ju6ge
Copy link
Contributor Author

ju6ge commented Jul 23, 2021

Sorry for the delayed response, there where some other issues that needed my attention.

I checked and for release there where no pdbs generated. I am really not sure what would be required to get them, since they are present in the normal windows-x86/x64 builds.

So no it is not expected, but i never actually needed those. They where just there when building using vcpkg, except for the case of x64-uwp. This seems like a bug in the build system not anything specific to rbdl-orb. For my use case this state would be fine. If you require those file to be present then i would need guidance on how to fix this, otherwise i think since all test are passing that this port is finished and can be merged.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Aug 3, 2021
@PhoebeHui
Copy link
Contributor

@ju6ge, I can't repro this issue locally now, it seems the problem has been fixed. and the pdb files exist now.

E:\vcpkg\18883\vcpkg\packages\rbdl-orb_x64-uwp\bin>dir
 Volume in drive E is New Volume
 Volume Serial Number is B273-EF86

 Directory of E:\vcpkg\18883\vcpkg\packages\rbdl-orb_x64-uwp\bin

08/03/2021  02:13 AM    <DIR>          .
08/03/2021  02:13 AM    <DIR>          ..
08/03/2021  02:13 AM           673,280 rbdl.dll
08/03/2021  02:13 AM        50,507,776 rbdl.pdb
08/03/2021  02:13 AM            84,992 rbdl_geometry.dll
08/03/2021  02:13 AM         2,330,624 rbdl_geometry.pdb
08/03/2021  02:13 AM           203,264 rbdl_luamodel.dll
08/03/2021  02:13 AM         4,362,240 rbdl_luamodel.pdb
08/03/2021  02:13 AM           176,128 rbdl_urdfreader.dll
08/03/2021  02:13 AM         4,763,648 rbdl_urdfreader.pdb
               8 File(s)     63,101,952 bytes
               2 Dir(s)  338,876,710,912 bytes free

@ju6ge
Copy link
Contributor Author

ju6ge commented Aug 3, 2021

Great news 👏

@dan-shaw dan-shaw merged commit 43df49d into microsoft:master Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants