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

Updated OpenCV compatibility for libcxxwrap_julia_jll to version 0.11 #8820

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

Moeasy64
Copy link
Contributor

Good morning,

As we are working with our Professor @marcoxa, I am opening this pull request in order to fix the incompatibility problem for Julia 1.10 of OpenCV package. I'm also going to open a PR on OpenCV.jl to update the Project.toml with the CxxWrap version 0.14. I hope this will fix the incompatibility.

Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

You also need to change the version number of the package itself:

version = v"4.6.0"

which basically means upgrade to a newer version

O/OpenCV/build_tarballs.jl Outdated Show resolved Hide resolved
O/OpenCV/build_tarballs.jl Outdated Show resolved Hide resolved
@marcoxa
Copy link

marcoxa commented May 31, 2024

Hi

my students are doing a great job tracking down these issues.

Let me say that all of this should be ... easier.

@marcoxa
Copy link

marcoxa commented May 31, 2024

You also need to change the version number of the package itself:

version = v"4.6.0"

which basically means upgrade to a newer version

We are not the maintainers of the OpenCV Julia binding; it should be up to them to decide on a new version number.

@giordano
Copy link
Member

it should be up to them to decide on a new version number.

It looks like there have been already four releases after 4.6.0: https://opencv.org/releases/

@marcoxa
Copy link

marcoxa commented May 31, 2024

it should be up to them to decide on a new version number.

It looks like there have been already four releases after 4.6.0: https://opencv.org/releases/

Good point. But this is the OpenCV release(s).

The bigger issue here is how to bring the Julia binding of OpenCV up to speed with Julia latest incarnations.

@barche
Copy link
Contributor

barche commented May 31, 2024

It looks like the actual Julia wrappers are in opencv_contrib, which like OpenCV itself has a tag for version 4.9.0. Unfortunately the code in the Julia module is 3 years old, so it may need some adaptation.

The reason the OpenCV version needs to be incremented is because the compat requirements of this JLL will change by moving to a newer libcxxwrap_julia. The most elegant way to do that is by moving to a more recent OpenCV upstream version. There are workarounds by modifying the version number only here, but this is messy and best avoided.

@giordano
Copy link
Member

giordano commented May 31, 2024

The main reason for me to ask to change the version number was the change of the compat bound for julia, which appears unjustified. Without that, we may not need to change the version number of the package, but someone still needs to fix the wrapper somehow.

@marcoxa
Copy link

marcoxa commented May 31, 2024

The main reason for me to ask to change the version number was the change of the compat bound for julia, which appears unjustified. Without that, we may not need to change the version number of the package, but someone still needs to fix the wrapper somehow.

That "someone still needs to fix the wrapper somehow" seems reasonable.

Now: where should this wrapper be "fixed"? In the OpenCV main repository? Meaning a PR to the OpenCV community? And what about the libcxxwrap library? How to coordinate these changes?

@barche
Copy link
Contributor

barche commented May 31, 2024

Now: where should this wrapper be "fixed"? In the OpenCV main repository? Meaning a PR to the OpenCV community? And what about the libcxxwrap library? How to coordinate these changes?

The wrapper lives here: https://github.com/opencv/opencv_contrib/tree/4.x/modules/julia

Unfortunately, it is a little more complicated than I anticipated, since the wrapper is actually auto-generated using a Python script, so modifying that would probably be the best way forward. Looking at the git commits in that directory it seems the wrappers were added as part of a GSOC project.

@marcoxa
Copy link

marcoxa commented Jun 1, 2024 via email

@barche
Copy link
Contributor

barche commented Jun 2, 2024

Getting this to work for Julia 1.10 may actually be easier than I initially thought, all that is needed is to modify bundled/patches/opencv-julia.patch to replace

@wrapmodule(OpenCV_jll.libopencv_julia_path, :cv_wrap)

with

@wrapmodule(OpenCV_jll.get_libopencv_julia_path, :cv_wrap)

The Julia 1.11 and 1.12 errors can probably also be fixed by adding a patch to opencv_contrib/modules/julia/gen/cpp_files/jlcxx/array.hpp, backporting some of the changes that were made in libcxxwrap_julia.

@likanzhan
Copy link

@barche Is there an update to this problem? And where this code should be changed? Thanks.

@giordano
Copy link
Member

And where this code should be changed? Thanks.

#8820 (comment)

@ViralBShah
Copy link
Member

ViralBShah commented Oct 13, 2024

@marcoxa The easiest thing for us to do is to apply the patches we need for opencv_contrib in Yggrdasil and build opencv 4.10 first. We can then submit them upstream to opencv_contrib so that we don't need to keep carrying them.

It seems that once we can get this over the hump, future updates should hopefully be straightforward since a lot of the machinery on the Julia side has stabilized a fair bit (fingers crossed).

Note that some of the bundled patches against opencv 4.6 will need to be reviewed and updated (or removed if those changes have been incorporated upstream).

@barche
Copy link
Contributor

barche commented Oct 20, 2024

I added some changes that will hopefully make this build against the latest libcxxwrap-julia

@ViralBShah ViralBShah dismissed giordano’s stale review October 22, 2024 01:38

This is addressed. Not sure what other option I have to clear this and hence dismissing.

@ViralBShah
Copy link
Member

@barche That is an insane patch - thank you! I hope going forward it won't need this much work. Should the opencv-contrib patches be upstreamed?

@barche
Copy link
Contributor

barche commented Oct 22, 2024

Before upstreaming the opencv_contrib changes I'd like to see what can be changed in libcxxwrap-julia, since some of the changes (e.g. the container_has_less_than_operator specializations) are only needed because of a poor design in libcxxwrap-julia, that causes also problems elsewhere (such as JuliaInterop/CxxWrap.jl#455)

Also, before merging this PR the binaries (downloadable from buildkite) should be tested with OpenCV.jl

@ViralBShah ViralBShah closed this Oct 22, 2024
@ViralBShah ViralBShah reopened this Oct 22, 2024
@ViralBShah
Copy link
Member

@marcoxa @Moeasy64 Would it be possible for you to test the binaries from this PR? In the build pipeline in the CI, each of the jobs has artifacts that can be tested.

@barche
Copy link
Contributor

barche commented Oct 22, 2024

The binaries are not working because the OpenCV build system adds the flags -fvisibility=hidden -fvisibility-inlines-hidden to the compilation. I don't immediately see where, so clues welcome :)

@barche
Copy link
Contributor

barche commented Oct 23, 2024

Provided all builds still pass the binaries should work after the build is complete. At least the OpenCV tests start here in Julia 1.11 locally, with no output shown yet at the time of writing this.

So if all builds pass this is good to merge for me.

@barche
Copy link
Contributor

barche commented Oct 24, 2024

Going over the PR one more time before hitting the big green button I see this still uses Qt5Base. Any idea if this is needed? It will conflict with the Qt6Base requirement of GR (and thus Plots).

@barche
Copy link
Contributor

barche commented Oct 24, 2024

I went ahead and tried directly by just changing to Qt6Base, let's hope this recent OpenCV just picks it up. It's best to do this change now, since updating the Qt dep would require a JLL version change otherwise.

@barche barche merged commit 6a38954 into JuliaPackaging:master Oct 24, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants