[aravis] add new port (+add to OpenCV 4 as optional feature) #42351
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dg0yt
reviewed
Nov 25, 2024
ports/opencv4/vcpkg.json
Outdated
| "aravis": { | ||
| "description": "aravis", | ||
| "dependencies": [ | ||
| "aravis" |
There was a problem hiding this comment.
Enable opt-out from usb.
Suggested change
| "aravis" | |
| { | |
| "name": "aravis", | |
| "default-features": false | |
| } |
There was a problem hiding this comment.
then it might be better to leave usb away as a default feature from aravis? i now went with that option to stay aligned with aravis (though i think they make it dependent on the availability of the library by default since they set it to auto)
There was a problem hiding this comment.
vcpkg will install aravis' default feature by default. This change enables the user to explicitly request installation of aravis without default features.
Comment on lines
1
to
5
| include(FindPkgConfig) | ||
| pkg_search_module(Aravis REQUIRED aravis-0.8) | ||
| if (Aravis_FOUND) | ||
| set(Aravis_LIBS "aravis-0.8") | ||
| endif() |
There was a problem hiding this comment.
This wrapper is pointless and not working correctly.
The code should be added to the consuming projects instead.
There was a problem hiding this comment.
without this wrapper i can't do find_package(Aravis REQUIRED) - i didn't find any documentation on how to resolve this (even vcpkg-cmake-wrapper.cmake is undocumented beyond the fact that it may exist). how else would you solve this?
ports/aravis/usage
Outdated
Comment on lines
1
to
4
| The aravis package provides CMake targets: | ||
|
|
||
| find_package(aravis REQUIRED) | ||
| target_link_libraries(main PRIVATE ${Aravis_LIBRARIES}) |
There was a problem hiding this comment.
Don't add a usage file here. The tool generates the desired information heristically for pkg-config.
There was a problem hiding this comment.
ok, i've removed it.
but then it just says:
aravis provides pkg-config modules:
# Camera control and image acquisition library
aravis-0.8
isn't this less helpful than having a clear example on what to do (as is e.g. the case with the usage i had provided)?
rursprung
force-pushed
the
add-aravis
branch
4 times, most recently
from
ed919f4 to
16cbe1d
Compare
rursprung
changed the title
rursprung
force-pushed
the
add-aravis
branch
2 times, most recently
from
51a354c to
69fffe6
Compare
|
@dg0yt: thanks a lot for your super fast review! i now have one last CI failure in the android CI builds which i don't know how to solve: based on labstreaminglayer/liblsl-Android#4 (unrelated library, just found the error description there) this is caused when using a too-old Android NDK as these methods were implemented more recently (Android NDK 24, see also the source code). is there a way to force the CI to build with a newer android SDK version for this port (though the log file does talk about also, i just got this result on windows in the CI: i guess i do need to add a patch for the build after all to skip generating these bins (at least until it's done in upstream)? |
WangWeiLin-MV
added
category:port-feature
|
The Android problem is not the NDK (vcpkg CI has r27c) but the API level (vcpkg uses 21 as default). |
|
For the executables in |
rursprung
force-pushed
the
add-aravis
branch
from
69fffe6 to
bb39fe6
Compare
|
would you mind adding aravis feature to the vcpkg-ci-opencv port, so that it is tested in CI? |
|
it seems that OpenCV doesn't find Aravis on Windows with their
i've added it (as a dedicated entry due to having to add |
|
Unfortunately, the CI failure logs weren't uploaded. |
here's the relevant excerpt from aravis itself was installed successfully (and i can find it in |
|
i am trying to reproduce it locally on my computer. If I will find any solution, I will ask your permission to push to your vcpkg fork |
|
ok I have a patch. Can I push to your fork? please add me as a contributor to https://github.com/rursprung/vcpkg
or add this patch |
|
then it complains about missing to better speed up the work, please let me be a contributor to your fork so that i can quickly add everything that i find |
|
thank you for looking into this! i've added you as a collaborator. is flagging the dependencies as |
|
detect_aravis.cmake should be called only if aravis is required. And in that case those findings are required, not optional, so that's the rationale adopted also in other opencv patches |
This patch worked for me: |
|
added the patch so that you know the current state of art. We will need to work on the aravis_libraries list and append missing glib ones to fully fix the link errors |
|
drastically simplified (does not even need a new patch anymore and it builds here) |
|
@rursprung please try also on your side the windows libraries. If it works, mark the PR as ready. Aravis is great addition (i remember how much difficult it was even trying to make it work 5 years ago on windows, now the windows ecosystem is improved so much that the PR is quite straightforward) |
dg0yt
reviewed
Nov 28, 2024
Comment on lines
+28
to
+29
| vcpkg_find_acquire_program(PKGCONFIG) | ||
| set(ENV{PKG_CONFIG} "${PKGCONFIG}") |
There was a problem hiding this comment.
Note that this may also change behavior for other find modules, in unexpected/untested ways. (Mostly on Windows.)
And in most other CMake ports, we pass it as PKG_CONFIG_EXECUTABLE cmake option. (Nitpick.)
thank you very much for all your work here, @cenit! here are my test results:
one thing i'm not sure is whether we should keep it as-is with |
|
@rursprung can you check what's failing on x64-linux? i had no time to verify it, but this PR is very interesting and it's a pity if it becomes stale |
|
@cenit: based on the build logs this is a false reject: see line 3875 in the logs. i think re-triggering it is enough. what's your opinion/input on my previous question regarding the default features? and his review feedback regarding your OpenCV patch (which i can't judge, sorry)? |
this cannot be the error. llvm cannot be cached but it never made the pipeline look as failing about pkgconfig, i think that opencv feature self-evaluation mechanism should be good enough to verify that features are working as expected and as before. In any case, enabling pkgconfig is good and should have been done before to enable future additional features (i copy/pasted the most common method i found in other ports) for default features, no opinions. I usually deal with GigE so any outcome is fine for me without having further investigated the matter |
i had another look and there were a bunch more failures with the exact same pattern + this one (which is also a false reject): => i still believe that with a re-run this will be fixed
ok, then let's leave it as-is. do you want to reply to the review feedback and mark it as done?
then i'll make my live easier and set it as a default feature 🙂 |
Co-Authored-By: Stefano Sinigardi <[email protected]>
|
based on the latest build i see no direct relation between this failure and aravis. i sadly really don't have time to dig into this right now, sorry! 😢 |
|
Assuming that opencv imports aravis via pkg-config, the rtabmap error is an indication that the aravis pc files forwards the glib linking requirement only as |
thanks @dg0yt! i'm back to being completely out of my waters again (never worked with pkg-config before) 😅 i had a look at the generated this is using (the file is the same for debug & release builds) if i see it correctly the file is being generated here as part of the meson build: https://github.com/AravisProject/aravis/blob/a33a3f4ff8bca72b27a193b39debed0327c3a551/src/meson.build#L312-L318 |
|
I changed this to draft because the build is broken; please feel free to mark 'Ready for review' when it's working and you're happy with it. |
|
Ah, I forgot to add a note. The linker line has everything which is need. However, the linux toolchain has what CMake calls a "traditional linker": The order of static libraries matters. But here CMake doesn't provide link libs correctly. We have seen this before, and we don't know the exact triggers :-( Unfortunately, I can't provide more help ATM. |
|
i have the same error in my opencv PR |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
please note:
aravisfeature introduced in this PR) and it worked finearv-camera-test-0.8,arv-fake-gv-camera-0.8,arv-test-0.8andarv-tool-0.8) which cannot be disabled at the moment (except with a patch to their build system), i've reported this to upstream: add meson option to build only the library AravisProject/aravis#962. i think they currently don't hurt and it's better if this gets fixed in upstream rather than having a patch here.resolves #37072
resolves #3411
checklist:
find_packagecalls are REQUIRED, are satisfied byvcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.vcpkg.jsonmatches what upstream says.vcpkg.jsonmatches what upstream says../vcpkg x-add-version --alland committing the result.