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] New port #14299

Merged
merged 25 commits into from
Nov 4, 2020
Merged

[rtabmap] New port #14299

merged 25 commits into from
Nov 4, 2020

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented Oct 29, 2020 •
edited
Loading

Describe the pull request

@seanyen seanyen marked this pull request as ready for review October 30, 2020 00:58
@seanyen
Copy link
Contributor Author

seanyen commented Oct 30, 2020

This is ready for review and merge. Thanks!

PhoebeHui
PhoebeHui requested changes Oct 30, 2020
View reviewed changes
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.

@seanyen, thanks for your contribution!

@PhoebeHui PhoebeHui added category:new-port The issue is requesting a new library to be added; consider making a PR! requires:author-response labels Oct 30, 2020
@seanyen
Copy link
Contributor Author

seanyen commented Oct 30, 2020

@PhoebeHui Thanks for the review. I addressed the feedback in the latest commit.

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Oct 30, 2020
BillyONeal
BillyONeal previously requested changes Oct 30, 2020
View reviewed changes
ports/rtabmap/001_opencv.patch Outdated
)
ENDIF(GTSAM_FOUND)

+IF(NOT VTK_FOUND)
Copy link
Member

Choose a reason for hiding this comment

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

It seems like the port should declare a dependency on vtk instead? It would be bad if:

vcpkg install vtk
vcpkg install rtabmap

and

vcpkg install rtabmap
vcpkg install vtk

produce different output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this VTK_FOUND patch and added vtk as a dependency. Waiting for the CI results.

@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Oct 30, 2020
@seanyen
Copy link
Contributor Author

seanyen commented Oct 31, 2020 •
edited
Loading

Connecting #11601. Adding VTK as a dependency run into the same issue reported there. It cannot find pcl/visualization/*.h even when the relative feature (pcl[vtk]) is enabled.

So instead of dealing with VTK related things, I used WITH_QT=OFF to disable all the visualization tools builds and reworked the condition when DISABLE_VTK gets defined. Hope that can address the concern where vcpkg install vtk; vcpkg install rtabmap vs vcpkg install rtabmap; vcpkg install vtk might produce different outputs.

BillyONeal
BillyONeal reviewed Nov 2, 2020
View reviewed changes
ports/rtabmap/vcpkg.json Outdated
"version-string": "0.20.3",
"description": "Real-Time Appearance-Based Mapping",
"homepage": "https://introlab.github.io/rtabmap/",
"supports": "!linux & !osx & !static",
Copy link
Member

@BillyONeal BillyONeal Nov 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should this be supports: windows and then call vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY ?

see https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/vcpkg_check_linkage.md#notes

(Note that this will automatically make vcpkg_configure_cmake and friends do the right thing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By doing that, it caused the static windows CI build failure on this error:

-- Note: rtabmap only supports dynamic library linkage. Building dynamic library.
CMake Error at scripts/cmake/vcpkg_check_linkage.cmake:42 (message):
  Refusing to build unexpected dynamic library against the static CRT.  If
  this is desired, please configure your triplet to directly request this
  configuration.

Maybe I should do supports: windows & !static plus vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY)?

@BillyONeal BillyONeal dismissed their stale review November 2, 2020 05:43

The bits I was waiting for are fixed.

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Nov 3, 2020
@BillyONeal BillyONeal merged commit 9549cd9 into microsoft:master Nov 4, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal left review comments

@PhoebeHui PhoebeHui PhoebeHui approved these changes

Assignees

@PhoebeHui PhoebeHui

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[New Port Request] RTAB-MAP (or RTABMAP)
4 participants
@seanyen @BillyONeal @PhoebeHui @NancyLi1013