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 v4.1.1 #5169

Merged
merged 50 commits into from
Aug 12, 2019
Merged

[OpenCV] Update to v4.1.1 #5169

merged 50 commits into from
Aug 12, 2019

Conversation

cenit
Copy link
Contributor

@cenit cenit commented Jan 16, 2019

Any feedback is welcome!

fixes: #2065
fixes: #2749
fixes: #2799
fixes: #3095
fixes: #3418
fixes: #4160
fixes: #4601
fixes: #4725
fixes: #4758
fixes: #5101
fixes: #5557
fixes: #5876
fixes: #6728
fixes: #6994

@luzpaz luzpaz mentioned this pull request Jan 18, 2019
3 tasks
@cenit cenit mentioned this pull request Jan 22, 2019
3 tasks
@cenit cenit mentioned this pull request Jan 30, 2019
5 tasks
@Codiferous Codiferous added the wip label Feb 8, 2019
@cenit
Copy link
Contributor Author

cenit commented Feb 10, 2019

Waiting on my other open PRs before asking this merge...

@cenit cenit changed the title [WIP] [OpenCV 4] Add a port of this new release [OpenCV 4] Update to v4, keep v3 as another port Feb 27, 2019
@cenit
Copy link
Contributor Author

cenit commented Feb 27, 2019

Thanks to #5417 finally completed, I think that we can start discussing this PR @Codiferous. Please tell me if you see any regression, so that I can start fixing bugs and problems. It could be possible that some ports must be made dependent on opencv3 instead of opencv, so that they keep working as before without big fixups, waiting for the upstream projects to be updated.

@cenit
Copy link
Contributor Author

cenit commented Feb 27, 2019

btw, in parallel of the opencv3 rename, what's your opinion on #3360? Shall I close it and forget it? I think it could be interesting in some edge cases... and not dangerous in any way, am i right?

@Rastaban
Copy link
Contributor

In the PR test this is causing failures in zxing-cpp on x86-windows and x64-windows, which has a dependancy on openvc.

@cenit
Copy link
Contributor Author

cenit commented Feb 28, 2019

@Rastaban thanks for noticing. I added a patch and zxing-cpp is building ok locally. Is it fixed here? I see a red cross anyway

@cenit cenit changed the title [OpenCV 4] Update to v4, keep v3 as another port [OpenCV] Update to v4, keep v3 as another port Feb 28, 2019
@Rastaban
Copy link
Contributor

Rastaban commented Mar 5, 2019

Here are the test results. The left results column is the latest commit to this PR, the right results column is test results from master: Blank space means the port did not exist and Skip typically means somthing it has a dependancy on failed so it was not built.

Processing arm-uwp                        412 vs 411
    ade                                                         Pass vs 
    opencv3                                                     Skip vs 
Processing arm64-windows                  467 vs 466
    ade                                                         Pass vs 
    opencv3                                                     Skip vs 
Processing x64-linux                      603 vs 605
    ade                                                         Fail vs 
    opencv                                                      Skip vs Pass
    opencv3                                                     Fail vs 
    zxing-cpp                                                   Skip vs Pass
Processing x64-osx                        594 vs 596
    ade                                                         Fail vs 
    opencv                                                      Skip vs Pass
    opencv3                                                     Fail vs 
    zxing-cpp                                                   Skip vs Pass
Processing x64-uwp                        438 vs 439
    ade                                                         Pass vs 
    opencv                                                      Skip vs Pass
    opencv3                                                     Fail vs 
    zxing-cpp                                                   Skip vs Pass
Processing x64-windows                    858 vs 857
    ade                                                         Pass vs 
    opencv3                                                     Fail vs 
Processing x64-windows-static             772 vs 773
    ade                                                         Pass vs 
    opencv                                                      Skip vs Pass
    opencv3                                                     Fail vs 
    zxing-cpp                                                   Skip vs Pass
Processing x86-windows                    838 vs 837
    ade                                                         Pass vs 
    opencv3                                                     Fail vs 

Here are all of the failures logs:
failureLogs.zip

As a sidenote: We are working on getting the CI system out from behind the wall so you can see the results and poke around yourself. I'm not sure when that will happen though. Until then let us know if there are any logs that would be useful and we should be able to get them to you.

@cenit
Copy link
Contributor Author

cenit commented Mar 6, 2019

I pushed some patches here and there, on the OpenCV dependencies.
I can now build successfully (locally)
on macOS/linux: ./vcpkg install opencv[eigen,jpeg,opengl,openmp,png,tiff,webp,nonfree,contrib,eigen,ffmpeg,gdcm,ipp,jasper,jpeg,openexr,opengl,openmp,png,sfm,tbb,tiff,webp]
on windows: .\vcpkg.exe install opencv[eigen,jpeg,opengl,openmp,png,tiff,webp,nonfree,contrib,eigen,ffmpeg,gdcm,ipp,jasper,jpeg,openexr,opengl,openmp,png,sfm,tbb,tiff,webp,cuda,ovis]

ovis is a little bit problematic to fix on non-windows, there is a strange ninja error I've never seen related to a non-expanded variable inside FLAGS.
and for cuda I don't have any linux system to test it

@cenit
Copy link
Contributor Author

cenit commented Mar 6, 2019

it was a really tedious work. There are so many broken libraries... 😢 😭

@cenit
Copy link
Contributor Author

cenit commented Mar 6, 2019

again, this PR has become a huge PR with many different packages involved. First time something similar was closed and rebuilt in many different parts, copying my work and without even cherry picking my commits (no trace left of it, at the beginning I accepted it but for me it is important to have trace of the work done), last time another admin here merged all of them (luckily picking my commits). I hope that now is ok in this way, also to have a positive feedback by the CI

@cenit
Copy link
Contributor Author

cenit commented Mar 6, 2019

@Rastaban can you please give me an update of failures/pass, since the green mark is still a mirage?

@cenit
Copy link
Contributor Author

cenit commented Mar 7, 2019

i also tested builds on windows with static library linkage (but dynamic crt) and with the latest commit now also this config is fully working (at least here, at least for me)

@cenit
Copy link
Contributor Author

cenit commented Mar 7, 2019

I also added extension for opencv3 from #5123 (a little bit different because it does not work otherwise in static builds...), from #4802 and from #3635, so that in case this PR is accepted there's no regression in functionalities for the backward-compatible opencv3 port

@cenit
Copy link
Contributor Author

cenit commented Mar 7, 2019

While waiting for the logs, in order to understand what to fix next, I also have a problem to submit: due to the official FindLibLZMA.cmake not supporting debug/release configs, OpenCV does not propagate correctly this dependency inside its module file (it is built correctly, but to consume it I have to manually fix a cmake module). Let me explain it better:

Inside the file OpenCVModules.cmake, which is installed in vcpkg/installed/x64-windows-static/share/opencv, we can find a string which is similar to

C:/Users/username/vcpkg/installed/x64-windows-static/debug/lib/lzma.lib;

it must be changed with

\$<\$<NOT:\$<CONFIG:DEBUG>>:C:/Users/username/vcpkg/installed/x64-windows-static/lib/lzma.lib>;\$<\$<CONFIG:DEBUG>:C:/Users/username/vcpkg/installed/x64-windows-static/debug/lib/lzma.lib>;

Shall we produce a regex in the portfile to accomplish this? Is anyone able to help me? (regex are not really part of my strength, at least for now :D )
Maybe @ras0219-msft can you help me?

@cenit cenit changed the title [OpenCV] Update to v4, keep v3 as another port [OpenCV] Update to v4, keep v3 as another port. Fix many dependencies along the way Mar 7, 2019
@cenit
Copy link
Contributor Author

cenit commented Mar 12, 2019

@Rastaban Sorry for the ping, but do you have any news on the CI logs?

@cenit
Copy link
Contributor Author

cenit commented Mar 12, 2019

I was already managing "nonfree" in opencv3/4, so the latest merge is only to resolve conflicts, but nothing should have been changed from before.

@Cheney-W
Copy link
Contributor

Here are the latest test results:

Processing x64-linux                      623 vs 618 
     opencv                        **regression**                Fail vs Pass
Processing x64-windows                    876 vs 888
     gdcm2                         **regression**                Fail vs Pass
     libwebp                       **regression**                Fail vs Pass
Processing x86-windows                    855 vs 867
     gdcm2                         **regression**                Fail vs Pass
     libmupdf                      **regression**                Fail vs Pass
     libwebp                       **regression**                Fail vs Pass

And the log as below:
failureLogs.zip

@cenit
Copy link
Contributor Author

cenit commented Mar 15, 2019

I can't reproduce any libmupdf problem locally. I wonder what's the problem because I can clearly see the openjpeg.lib in the command line at linking stage and those missing symbols are exported by that lib.

I have merged with master in the meantime, during the weekend I should have more free time to check remaining issues: I hope to see this PR closed soon, because it is becoming too huge to maintain

@cenit
Copy link
Contributor Author

cenit commented Mar 15, 2019

is it because I didn't update the CONTROL files that maybe some ports are not rebuilt?

@cenit
Copy link
Contributor Author

cenit commented Mar 16, 2019

#5625 might be the proper fix for the problem described in #5169 (comment). Which is not a problem discoverable with CI at the moment, due to absence of any test on built libraries, am I right?

@cenit
Copy link
Contributor Author

cenit commented Mar 19, 2019

I imported also a patch for libmupdf which should fix also problems for x86 builds.
Can I have an update at the end of what's still broken and what not? I doubt the green mark will be here at the end of the CI tests... 😢
To be honest, let me repeat myself. I think this PR is exploding, I cannot fix all the packages here only because my dependencies were broken from the beginning and they were a dependency for other broken packages...

@vicroms
Copy link
Member

vicroms commented Mar 19, 2019

While waiting for the logs, in order to understand what to fix next, I also have a problem to submit: due to the official FindLibLZMA.cmake not supporting debug/release configs, OpenCV does not propagate correctly this dependency inside its module file (it is built correctly, but to consume it I have to manually fix a cmake module). Let me explain it better:
Inside the file OpenCVModules.cmake, which is installed in vcpkg/installed/x64-windows-static/share/opencv, we can find a string which is similar to
C:/Users/username/vcpkg/installed/x64-windows-static/debug/lib/lzma.lib;

it must be changed with
$&lt;$&lt;NOT:$CONFIG:DEBUG>:C:/Users/username/vcpkg/installed/x64-windows-static/lib/lzma.lib>;$<$CONFIG:DEBUG:C:/Users/username/vcpkg/installed/x64-windows-static/debug/lib/lzma.lib>;

Shall we produce a regex in the portfile to accomplish this? Is anyone able to help me? (regex are not really part of my strength, at least for now :D )
Maybe @ras0219-msft can you help me?

I have fixed a similar problem in the past by doing a STRING(REGEX REPLACE ..) in the portfile.
If this is still an issue I can take a look at it.

@cenit
Copy link
Contributor Author

cenit commented Aug 2, 2019

Thanks for merging. I was doing the same thing :)

@cenit
Copy link
Contributor Author

cenit commented Aug 2, 2019

@vicroms vcpkg_configure_cmake does not contain the "WITH_HALIDE" new feature

edit: OTOH, is it now possible to remove all the other WITH_xxx because vcpkg_check_features takes care of them?

@vicroms
Copy link
Member

vicroms commented Aug 2, 2019

Oops, forgot to add it.

vcpkg_check_features() does not pass the WITH_xxx values to vcpkg_configure_cmake() (but it would be really could if it did), so we still have to pass them manually.

@vicroms
Copy link
Member

vicroms commented Aug 5, 2019

Thanks for investigating that ffmpeg patch!

I'll bring this issue to our team's offline discussion.

There is a new problem we should keep an eye on, for future vcpkg development: a staging area. This is not a problem now, since the feature works and I still don’t know what we are losing.

ffmpeg would like a basic opencv lib to enable some features (the patch was there to make it possible, but the feature is not even exposed). In turn, opencv would like ffmpeg ready to enable some other features. This would require a staging area, so that if i’d like to install opencv[ffmpeg] this would trigger:

1. build `opencv[core]` in a staging area
2. build ffmpeg pointing to `opencv[core]` 
3. build `opencv[ffmpeg]`

I'm hoping to merge this PR soon, are there any issues that you are aware that should be addressed first?

@cenit
Copy link
Contributor Author

cenit commented Aug 5, 2019

Not any that I am aware. Please proceed with merging!
I am also on vacation this week (after such a long time without you'd never imagine 🤣), which could mean two different things:
a) I have more time to help in case of big problems
b) or I might just turn off pc for a while...

Finally, what about versioning in the library filenames? Shall we keep it or remove? Since there can only be one version in Vcpkg installed foldertree, even if anyone uses overlays, I'd push for unversioned library file names. This would ease work for devs on projects that use explicit link inside VS solutions, because starting from now I'd hope for more aggressive updates for opencv 😄

@vicroms
Copy link
Member

vicroms commented Aug 7, 2019

I am also on vacation this week

Enjoy your vacations!

Finally, what about versioning in the library filenames? Shall we keep it or remove?

We think it's OK to remove the version from the library filenames.

@cenit
Copy link
Contributor Author

cenit commented Aug 8, 2019

Since it wouldn't be bad to find OpenCV 4 when I will be back at work, I did the removal of versioning from windows library names

@vicroms
Copy link
Member

vicroms commented Aug 9, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vicroms vicroms merged commit 4fb5152 into microsoft:master Aug 12, 2019
@vicroms
Copy link
Member

vicroms commented Aug 12, 2019

🎉🎉🎉🎉🎉

Finally after months of work this PR has been merged!

Thanks to everyone that contributed with their feedback and specially to @cenit for having the willpower and patience to see this through! 😄

The plan in the future is to handle updates to OpenCV much faster, and I think all the improvements made to the port in this PR will enable us to achieve that goal.

@cenit
Copy link
Contributor Author

cenit commented Aug 12, 2019

Thank you too, very much

@cenit
Copy link
Contributor Author

cenit commented Aug 12, 2019

It has been an extremely long journey, with so many parallel PRs necessary to make this one see the light! Thanks to everybody. It has been difficult, but very satisfying

@cenit
Copy link
Contributor Author

cenit commented Aug 12, 2019

@vicroms not sure we handled properly the meta-package thing you recently pushed.
This is the message I have when trying to update

Warning: could not reinstall feature opencv[eigen]:x64-linux
Warning: could not reinstall feature opencv[flann]:x64-linux
Warning: could not reinstall feature opencv[jpeg]:x64-linux
Warning: could not reinstall feature opencv[opengl]:x64-linux
Warning: could not reinstall feature opencv[png]:x64-linux
Warning: could not reinstall feature opencv[tiff]:x64-linux
Warning: could not reinstall feature opencv[eigen]:x64-windows
Warning: could not reinstall feature opencv[flann]:x64-windows
Warning: could not reinstall feature opencv[ipp]:x64-windows
Warning: could not reinstall feature opencv[jpeg]:x64-windows
Warning: could not reinstall feature opencv[opengl]:x64-windows
Warning: could not reinstall feature opencv[png]:x64-windows
Warning: could not reinstall feature opencv[tiff]:x64-windows
Warning: could not reinstall feature opencv[webp]:x64-windows

not the nicest welcome to the latest OpenCV release. Without proper exposure in the metapackage, many features are hidden behind the opencv4 package...

@cenit
Copy link
Contributor Author

cenit commented Aug 12, 2019

I will open soon another PR, trying to fix these things and also update OpenCV 3 to a proper status. Having OpenCV3 still around in a broken state is something I cannot stand, sorry

@vicroms
Copy link
Member

vicroms commented Aug 13, 2019

@vicroms not sure we handled properly the meta-package thing you recently pushed.
This is the message I have when trying to update

Warning: could not reinstall feature opencv[eigen]:x64-linux
Warning: could not reinstall feature opencv[flann]:x64-linux
Warning: could not reinstall feature opencv[jpeg]:x64-linux
Warning: could not reinstall feature opencv[opengl]:x64-linux
Warning: could not reinstall feature opencv[png]:x64-linux
Warning: could not reinstall feature opencv[tiff]:x64-linux
Warning: could not reinstall feature opencv[eigen]:x64-windows
Warning: could not reinstall feature opencv[flann]:x64-windows
Warning: could not reinstall feature opencv[ipp]:x64-windows
Warning: could not reinstall feature opencv[jpeg]:x64-windows
Warning: could not reinstall feature opencv[opengl]:x64-windows
Warning: could not reinstall feature opencv[png]:x64-windows
Warning: could not reinstall feature opencv[tiff]:x64-windows
Warning: could not reinstall feature opencv[webp]:x64-windows

not the nicest welcome to the latest OpenCV release. Without proper exposure in the metapackage, many features are hidden behind the opencv4 package...

That was a HUGE oversight on my part, the right thing to do is for the meta-package to expose all the features in opencv4.

@cenit
Copy link
Contributor Author

cenit commented Aug 13, 2019

Thanks for having fixed this quickly in #7659

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