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

[opencv] update to 4.9 #38928

Merged
merged 230 commits into from
Nov 5, 2024
Merged

[opencv] update to 4.9 #38928

merged 230 commits into from
Nov 5, 2024

Conversation

cenit
Copy link
Contributor

@cenit cenit commented May 24, 2024

Fixes #41418
Fixes #41250
Fixes #41094
Fixes #41038
Fixes #40680
Fixes #40568
Fixes #40393
Fixes #40406
Fixes #39884
Fixes #39835
Fixes #39651
Fixes #39454
Fixes #39224
Fixes #38321
Fixes #36771
Fixes #36093
Fixes #36009
Fixes #35937
Fixes #34559
Fixes #34279
Fixes #33742
Fixes #32228
Fixes #17850
Fixes #13120
Fixes #4937

Depending on #39703 (need to remove ffmpeg from ci.baseline here after that PR acceptance)

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label May 24, 2024
ports/opencv3/vcpkg.json Outdated Show resolved Hide resolved
ports/opencv4/vcpkg.json Outdated Show resolved Hide resolved
@JavierMatosD
Copy link
Contributor

JavierMatosD commented Nov 4, 2024

any news?

We are experimenting with a new internal review process. We were using vcpkg-team-review to reflect something else, but that is no longer the case. My apologies for the confusion.

This PR lgtm, but I'd prefer that @Neumann-A's review comments be addressed. I'm placing the PR in draft for now.
Please mark 'ready for review' when you have either addressed Neumann-A's feedback or responded. I will not look at this PR if it is in Draft. Marking it 'ready for review' lets me know that it is ready to be looked at again.

JavierMatosD
JavierMatosD previously approved these changes Nov 4, 2024
Copy link
Contributor

@JavierMatosD JavierMatosD left a comment

Choose a reason for hiding this comment

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

Just need a response to Neumann-A's review

@JavierMatosD JavierMatosD marked this pull request as draft November 4, 2024 19:00
@cenit
Copy link
Contributor Author

cenit commented Nov 4, 2024

I tried to answer all points and I applied a modification for what concerns the python3[extensions] clause.
Let's wait for CI, then if @Neumann-A is fine I will make the PR ready for review again

@cenit
Copy link
Contributor Author

cenit commented Nov 4, 2024

assuming the approval as a generic go for all open points, i resolved all of them and marked pr as ready

@JavierMatosD JavierMatosD removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Nov 4, 2024
@cenit cenit marked this pull request as ready for review November 4, 2024 21:53
@vicroms vicroms merged commit ca1501f into microsoft:master Nov 5, 2024
16 checks passed
@AdrianEddy
Copy link

@cenit I'd like to express my gratitude for you for putting in all the work on this PR and sticking with it for all the needed changes and fixes. I was watching it regularly and seeing you going through so many reference updates, issues with dependencies and extremely long CI runs and I just wanted to say thank you. You did a great job for all of us!

@theoractice
Copy link

theoractice commented Nov 6, 2024

@cenit thank you very much!
BTW I just found this update installs all opencv headers into opencv4 directory on windows. Needs a little fix?
I am using the latest vcpkg.exe.

@cenit cenit deleted the dev/cenit/opencv49 branch November 6, 2024 12:31
@dg0yt
Copy link
Contributor

dg0yt commented Nov 7, 2024

I'm happy to see opencv 4.9 merged. However:

  • I see downstream regressions in some PRs which involve opencv4 at least indirectly due to changes to ci baseline, e.g. [itk] Revise, test, fix #41525.
  • I have a direct fail for opencv4[core,tesseract]:x64-android (during analysis of the other failures).
  • I stumble again over the updated patches with no relevant changes. They really make it harder to analyse what has changed and what hasn't.

@cenit
Copy link
Contributor Author

cenit commented Nov 7, 2024

I'm happy to see opencv 4.9 merged. However:

  • I see downstream regressions in some PRs which involve opencv4 at least indirectly due to changes to ci baseline, e.g. [itk] Revise, test, fix #41525.
  • I have a direct fail for opencv4[core,tesseract]:x64-android (during analysis of the other failures).
  • I stumble again over the updated patches with no relevant changes. They really make it harder to analyse what has changed and what hasn't.

openvino is a dependency of opencv, so if opencv can be a dependency of openvino (i don't know it very well) that might create a circular dependency that vcpkg has many times failed to serve properly (it would require building base opencv, then openvino, then again opencv with openvino). Creating openvino forcing it without opencv might be the best solution if we cannot solve the circular problem above.

opencv4 has no tesseract feature. Which fail do you notice? I tried many combinations and all were working. I found that with opencv 4.10 I had to change something in opencv4[core,contrib], which has a dependency on tesseract. Is it what you meant? I didn't find this problem with OpenCV 4.9 when i tested it in the same test matrix i run...

which patches have no relevant changes here? i will try my best to help you find any regression that might have been lost, even before the #41985 gets interesting

@dg0yt
Copy link
Contributor

dg0yt commented Nov 7, 2024

Creating openvino forcing it without opencv might be the best solution if we cannot solve the circular problem above.

It is a requirement. Implemented in #42017.

opencv4 has no tesseract feature. ... opencv4[core,contrib], which has a dependency on tesseract.

Yes. #42015.

which patches have no relevant changes here?

To clarify: no relevant changes wrt to the patch for 4.8. Such as ports/opencv4/0006-fix-uwp.patch.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 7, 2024

FTR I really don't expect every port and feature configuration to work immediately after a major update, given the complexity fo the ports.

@cenit
Copy link
Contributor Author

cenit commented Nov 7, 2024

About #42015, strange that i didn't notice it in the test matrix and it appeared for the 4.10... sorry for that. It's not a config set we use very much, thanks for the PR

About the relevant change...

image

this is the relevant one: the new line to which apply the fix. Some configs are very strict about patch application, if the line does not match exactly they fail, and I prefer to be compliant with those requirements.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 7, 2024

FTR the openvino failure is quite interesting:

CMake Error at /vcpkg/downloads/tools/cmake-3.30.1-linux/cmake-3.30.1-linux-x86_64/share/cmake-3.30/Modules/FindOpenSSL.cmake:210 (pkg_check_modules):
  Unknown CMake command "pkg_check_modules".
Call Stack (most recent call first):
  /mnt/vcpkg-ci/installed/x64-android/share/openssl/vcpkg-cmake-wrapper.cmake:37 (_find_package)
  /vcpkg/scripts/buildsystems/vcpkg.cmake:813 (include)
  /vcpkg/downloads/tools/cmake-3.30.1-linux/cmake-3.30.1-linux-x86_64/share/cmake-3.30/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
  /mnt/vcpkg-ci/installed/x64-android/share/curl/CURLConfig.cmake:60 (find_dependency)
  /mnt/vcpkg-ci/installed/x64-android/share/curl/vcpkg-cmake-wrapper.cmake:11 (_find_package)
  /vcpkg/scripts/buildsystems/vcpkg.cmake:813 (include)
  /vcpkg/downloads/tools/cmake-3.30.1-linux/cmake-3.30.1-linux-x86_64/share/cmake-3.30/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
  /mnt/vcpkg-ci/installed/x64-android/share/tesseract/TesseractConfig.cmake:24 (find_dependency)
  /vcpkg/scripts/buildsystems/vcpkg.cmake:859 (_find_package)
  /vcpkg/downloads/tools/cmake-3.30.1-linux/cmake-3.30.1-linux-x86_64/share/cmake-3.30/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
  /mnt/vcpkg-ci/installed/x64-android/share/opencv4/OpenCVModules.cmake:41 (find_dependency)
  /mnt/vcpkg-ci/installed/x64-android/share/opencv4/OpenCVConfig.cmake:139 (include)
  /mnt/vcpkg-ci/installed/x64-android/share/opencv/vcpkg-cmake-wrapper.cmake:3 (_find_package)
  /vcpkg/scripts/buildsystems/vcpkg.cmake:813 (include)
  cmake/extra_modules.cmake:7 (find_package)
  cmake/extra_modules.cmake:177 (ov_generate_dev_package_config)
  CMakeLists.txt:165 (include)

The only scenario where this should happen is with CMAKE_DISABLE_FIND_PACKAGE_PkgConfig=ON,
I didn't want to debug this any further. Just quickly fixing the baseline regression.

@cenit
Copy link
Contributor Author

cenit commented Nov 7, 2024

i was constantly getting this regression with openvino on android in this pr if you remember. I wrote also about it along the thread. Then it disappeared

@dg0yt
Copy link
Contributor

dg0yt commented Nov 7, 2024

Hm, I find the old posts now, but I have no memories. That's remarkable, given https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9709. But that was triggered from changes in CURL.

So with the next CMake version in vcpkg, this particular error will no longer occur...

@sandye51
Copy link

sandye51 commented Nov 9, 2024

I'm happy to see opencv 4.9 merged. However:

  • I see downstream regressions in some PRs which involve opencv4 at least indirectly due to changes to ci baseline, e.g. [itk] Revise, test, fix #41525.
  • I have a direct fail for opencv4[core,tesseract]:x64-android (during analysis of the other failures).
  • I stumble again over the updated patches with no relevant changes. They really make it harder to analyse what has changed and what hasn't.

openvino is a dependency of opencv, so if opencv can be a dependency of openvino (i don't know it very well) that might create a circular dependency that vcpkg has many times failed to serve properly (it would require building base opencv, then openvino, then again opencv with openvino). Creating openvino forcing it without opencv might be the best solution if we cannot solve the circular problem above.

opencv4 has no tesseract feature. Which fail do you notice? I tried many combinations and all were working. I found that with opencv 4.10 I had to change something in opencv4[core,contrib], which has a dependency on tesseract. Is it what you meant? I didn't find this problem with OpenCV 4.9 when i tested it in the same test matrix i run...

which patches have no relevant changes here? i will try my best to help you find any regression that might have been lost, even before the #41985 gets interesting

OpenCV is used only in testing / sampler by OpenVINO, so it's not a produce level dependency.
As tests / samples are turned off in vcpkg, disabling OpenCV in OpenVINO's cmake is the right decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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