-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[rtabmap] update to 0.21.0 #30254
[rtabmap] update to 0.21.0 #30254
Conversation
I would need help from vcpkg maintainers, The CI is failing without any description of the error, and I cannot reproduce it on my computer. Here is the output on my side:
|
You can click through from checks to the "failure logs" artifacts on GH actions. This is the error:
AFAICS the project uses qt5_wrap_ui, so Note that the cmake config error log has two warnings related to policies affecting autouic - also on your system. |
The project uses Qt6 and I compared the logs on my computer and the one on Azure, see attachments: It seems they are not using the same compiler, maybe it is the issue why I cannot reproduce on my side. Here the logs when it fails on Azure:
and corresponding lines on my computer (they are completely different):
On CI, I am not sure why it is using EDIT: cmake config: |
I may have hit an old version, but is it significant? The current version is using |
Oh sorry, yeah the project indeed uses |
ports/rtabmap/vcpkg.json
Outdated
"description": "Build RTAB-Map with GUI support (Qt)", | ||
"dependencies": [ | ||
{ | ||
"name": "pcl", | ||
"default-features": false, | ||
"features": [ | ||
"qt" | ||
] | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To build with Qt support, there should be an explicit dependency on qtbase
and the other qt ports which are needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked thx! actually no... :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't expect it to fix the issue. Still waiting for --trace-expand
.
FTR I would also add "default-features": false
for qtbase
so that users can control which features they want from the default set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated qtbase without defautl features. Here is the --trace-expand:
vcpkg install rtabmap --x-cmake-args=--trace-expand > output.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it from the failing CI, i.e. add it to OPTIONS
of vcpkg_cmake_configure
.
…i2, realsense2, k4w2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port-version
should be 0
.
see https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/contributing/maintainer-guide.md#versioning
Reconverted to Draft, as this issue #30254 (comment) is still there on CI. As I cannot reproduce the error on my computer, it is quite difficult to debug this. I'll check back later when I will have some spare time. |
Now we have a big log file.
but also
I still don't see how CMake could determine that compiling AboutDialog.cpp depends the qt6_wrap_ui results, and the 32 jobs of vcpkg ci could make a difference with regard to the 16 jobs on your machine. |
@dg0yt FYI Because OpenCV dependency imported target explicitly sets:
the trick is to explicitly add this for downstream packages depending on Qt and OpenCV:
... well, for packages not using yet Ready to be merged! |
The features have been test successfully locally. |
vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/share/unofficial-rtabmap/unofficial-rtabmap-config.cmake" | ||
[[# Generated by CMake]] | ||
[[# Generated by CMake | ||
include(CMakeFindDependencyMacro) | ||
find_dependency(OpenCV) | ||
find_dependency(PCL) | ||
find_dependency(ZLIB) | ||
find_dependency(VTK) | ||
find_dependency(OpenMP)]] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we need to patch these transitive dependencies anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something missing on downstream packages? rtabmap now uses targets, and provide already those checks: https://github.com/introlab/rtabmap/blob/6dca8ab0f366b2628cafc332ec5ef10869e120b3/RTABMapConfig.cmake.in#L3-L47
./vcpkg x-add-version --all
and committing the result.Fixes #30293