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

[rtabmap] Fix platform and linkage support #37651

Merged
merged 8 commits into from
Apr 1, 2024
Merged

[rtabmap] Fix platform and linkage support #37651

merged 8 commits into from
Apr 1, 2024

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Mar 23, 2024 •
edited

rtabmap-res_tool is moved to a separate port because it is a host dependency, and actual dependencies of rtabmap are heavy and non opt-out. Only minimal patching needed.

CC @matlabbe.

@Cheney-W Cheney-W added the category:port-update The issue is with a library, which is requesting update new revision label Mar 25, 2024
@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 25, 2024 

/mnt/vcpkg-ci/installed/x64-linux/share/vtk/vtk-prefix.cmake(1):  set(_vtk_module_import_prefix ${CMAKE_CURRENT_LIST_DIR} )
/mnt/vcpkg-ci/installed/x64-linux/share/vtk/vtk-prefix.cmake(2):  get_filename_component(_vtk_module_import_prefix ${_vtk_module_import_prefix} DIRECTORY )
/mnt/vcpkg-ci/installed/x64-linux/share/vtk/vtk-prefix.cmake(3):  get_filename_component(_vtk_module_import_prefix ${_vtk_module_import_prefix} DIRECTORY )
/mnt/vcpkg-ci/installed/x64-linux/share/vtk/vtk-config.cmake(110):  set(${CMAKE_FIND_PACKAGE_NAME}_PREFIX_PATH ${_vtk_module_import_prefix} )
/mnt/vcpkg-ci/installed/x64-linux/share/vtk/vtk-config.cmake(112):  unset(_vtk_module_import_prefix )
/mnt/vcpkg-ci/installed/x64-linux/share/vtk/vtk-config.cmake(113):  list(INSERT CMAKE_PREFIX_PATH 0 ${${CMAKE_FIND_PACKAGE_NAME}_PREFIX_PATH} )

Ouch. vtk breaking vcpkg CMAKE_PREFIX_PATH for debug configurations. (Because of vcpkg moving cmake config.)
I think we can just remove this line. CC @Neumann-A

@Neumann-A
Copy link
Contributor

That seems to be 95052b8 what I fixed in the 5.12 PR

This was referenced Mar 26, 2024
Closed
Open
Merged
BillyONeal pushed a commit that referenced this pull request Mar 27, 2024
@dg0yt
[g2o] Fix suitesparse lookup and export (#37748)
d0b3e8e 
Ported from #37651.
@dg0yt dg0yt mentioned this pull request Mar 28, 2024
Open
@dg0yt dg0yt mentioned this pull request Mar 28, 2024
Merged
@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 28, 2024

The vtk patch is part of #37119.

BillyONeal pushed a commit that referenced this pull request Mar 28, 2024
@dg0yt
[pcl] Fix build issues, refresh patches (#37785)
fbc542f 
Ported from  #37651.
Fix include dir in pc files.
Fix #37655.
@dg0yt dg0yt mentioned this pull request Mar 31, 2024
Merged
1 task
@dg0yt dg0yt changed the title [rtabmap] More CI [rtabmap] Fix platform and linkage support Mar 31, 2024
dg0yt
dg0yt commented Mar 31, 2024
View reviewed changes
ports/rtabmap/link-keywords.patch
# Issue that qhull dependency uses optimized and debug keywords,
# which are converted to \$<\$<NOT:\$<CONFIG:DEBUG>> and \$<\$<CONFIG:DEBUG>
# in RTABMap_coreTargets.cmake (not sure why?!).
-list(REMOVE_ITEM PCL_LIBRARIES "debug" "optimized")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This caused harmful mixing of release libs and debug libs.

@dg0yt dg0yt marked this pull request as ready for review March 31, 2024 16:17
@Cheney-W Cheney-W added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 1, 2024
@Cheney-W Cheney-W added requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function info:reviewed Pull Request changes follow basic guidelines and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels Apr 1, 2024
@Cheney-W
Copy link
Contributor

Cheney-W commented Apr 1, 2024

In local testing of the rtabmap feature, the failure occurred due to the inability to install liblzma, which is a known issue.

BillyONeal
BillyONeal reviewed Apr 1, 2024
View reviewed changes
ports/rtabmap/portfile.cmake
-DWITH_PYTHON_THREADING=OFF
-DWITH_PDAL=OFF
-DRTABMAP_QT_VERSION=6
"-DRTABMAP_RES_TOOL=${CURRENT_HOST_INSTALLED_DIR}/tools/rtabmap-res-tool/rtabmap-res_tool${VCPKG_TARGET_EXECUTABLE_SUFFIX}"
Copy link
Member

Choose a reason for hiding this comment

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

Review, no change requested. This is a sort of the options list. These are the actual changes.

  • -DRTABMAP_QT_VERSION=6 added
  • "-DRTABMAP_RES_TOOL=... added
  • -DWITH_ORB_OCTREE=ON gained a comment, # GPLv3
  • -DWITH_ORB_SLAM=OFF added

Copy link
Member

Choose a reason for hiding this comment

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

That last one doesn't appear to be an actual change as it's the default, but this list tries to list all options https://github.com/introlab/rtabmap/blob/11adbdcc9f4edcf047e2f0c147ea6f888d780074/CMakeLists.txt#L214

BillyONeal
BillyONeal approved these changes Apr 1, 2024
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for the fixes!

ports/rtabmap/portfile.cmake
-DWITH_PYTHON_THREADING=OFF
-DWITH_PDAL=OFF
-DRTABMAP_QT_VERSION=6
"-DRTABMAP_RES_TOOL=${CURRENT_HOST_INSTALLED_DIR}/tools/rtabmap-res-tool/rtabmap-res_tool${VCPKG_TARGET_EXECUTABLE_SUFFIX}"
Copy link
Member

Choose a reason for hiding this comment

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

That last one doesn't appear to be an actual change as it's the default, but this list tries to list all options https://github.com/introlab/rtabmap/blob/11adbdcc9f4edcf047e2f0c147ea6f888d780074/CMakeLists.txt#L214

@BillyONeal BillyONeal merged commit c5b5b4e into microsoft:master Apr 1, 2024
16 checks passed
@dg0yt dg0yt deleted the rtabmap branch April 2, 2024 05:47
@dg0yt dg0yt mentioned this pull request Apr 5, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes

Assignees

@Cheney-W Cheney-W

Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dg0yt @Neumann-A @Cheney-W @BillyONeal