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 and opencv4: eliminate conflict, add new variants, bring parity between both #9555

Merged

Conversation

mascguy
Copy link
Member

@mascguy mascguy commented Dec 29, 2020

Description

Goals:

  • Eliminate conflict between opencv and opencv4.
  • Ensure parity and consistency between them, as much as possible.
  • Upgrade opencv4 to v4.5.0.

Detailed Changes:

  • opencv: eliminate conflict with opencv4; add variants debug, nonfree, tests
  • opencv4: eliminate conflict with opencv; switch to shared libs; rename variant opencv_contrib to contrib, to match opencv; add variants debug, java, python35..39, qt4, qt5, tbb, tests, vtk; add patch for dylibs
  • gerbil: bump revision to rebuild against updated opencv4; update for opencv4 library move
  • py-pytorch: bump revision to rebuild against updated opencv4; update for opencv4 library move

Fixed Issues:

Dependencies:

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.6.8 10K549
Xcode 3.2.2 10M2148

macOS 10.8.5 12F2560
Xcode 5.1.1 5B1008

macOS 10.12.6 16G2136
Xcode 9.2 9C40b

macOS 10.13.6 17G14019
Xcode 10.1 10B61

macOS 10.14.6 18G103
Xcode 11.3.1 11C505

macOS 10.15.6 19G2021
Xcode 12.0 12A7209

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • ensured the port does not create any faulty symbolic links?

@macportsbot
Copy link

Notifying maintainers:
@stromnov for port opencv4.
@neverpanic for port gerbil.

@neverpanic
Copy link
Member

Fine for me from gerbil PoV.

@mascguy mascguy changed the title opencv and opencv4: eliminate conflicts, and bring parity between both opencv and opencv4: eliminate conflicts, add new variants, bring parity between both Dec 31, 2020
@mascguy mascguy changed the title opencv and opencv4: eliminate conflicts, add new variants, bring parity between both opencv and opencv4: eliminate conflict, add new variants, bring parity between both Dec 31, 2020
@kencu
Copy link
Contributor

kencu commented Jan 1, 2021

IMHO we would leave ffmpeg enabled by default, and skip the variant being added. Everyone will want that, and it's always been like thst, so why change it? Less variants = MUCH better, for 100 reasons. Ideally MP would have NO variants, but we need some for x11 vs quartz, etc. Think, always, simpler is better, more consistent for all users is better, variants are evil.

There are a bunch of whitespace things (periods removed, etc) that are minor.

what exactly was the the reason opencv and opencv4 conflicted, and how was that resolved? I didn't immediately see that.

@mascguy
Copy link
Member Author

mascguy commented Jan 1, 2021

IMHO we would leave ffmpeg enabled by default, and skip the variant being added. Everyone will want that, and it's always been like thst, so why change it? Less variants = MUCH better, for 100 reasons. Ideally MP would have NO variants, but we need some for x11 vs quartz, etc. Think, always, simpler is better, more consistent for all users is better, variants are evil.

Agreed, +ffmpeg should be the default; update committed. As for the why, one of my goals was to provide more granularity, and flexibility. Indeed, one of the open issues brought up the ffmpeg case specifically.

what exactly was the the reason opencv and opencv4 conflicted, and how was that resolved? I didn't immediately see that.

It was primarily related to items added to ${prefix}/bin. But there were also other potential conflicts too, when enabling variant 'contrib'.

The conflict was eliminated by ensuring opencv4's binaries are installed into ${prefix}/libexec/opencv4/bin, and then soft-linking to them (with a prefix of opencv4_) in ${prefix}/bin.

@kencu
Copy link
Contributor

kencu commented Jan 1, 2021

so - granularity is bad, in and of itself. variants are bad. more variants are worse. They propogate down through all the dependencies, so when you have this:

sudo port install opencv

and that kicks in a +ffmpeg variant, that +ffmpeg variant then gets sent down the pipe to every dependency in the chain.

Many ports that have no real need for ffmpeg might then be forced to build with that variant, which makes people build everything from source who might have otherwise just installed a binary from the buildbot.

And then people go down build paths that are not often actually ever tested (who on earth in macports ever even builds any of the variants, much less all of them, before committing an update --? -- answer: nobody does).

So (IMHO) make the portfile simpler again, dump the ffmpeg variant, and just leave ffmpeg always enabled for opencv unless there is an actual, important use case whereby somebody might really need to install opencv WITHOUT ffmpeg. And as I think such a use case does not exist, having an added variant just increases MacPorts already excessive confusion.

Variants are evil. IMO we should ideally have no variants, but as that is not possible the ones we do need to have should be absolutely minimal. Adding variants does not improve a port, it detracts from it.

@mascguy
Copy link
Member Author

mascguy commented Jan 1, 2021

So (IMHO) make the portfile simpler again, dump the ffmpeg variant, and just leave ffmpeg always enabled for opencv unless there is an actual, important use case whereby somebody might really need to install opencv WITHOUT ffmpeg. And as I think such a use case does not exist, having an added variant just increases MacPorts already excessive confusion.

Fair enough, change committed.

@mascguy
Copy link
Member Author

mascguy commented Jan 2, 2021

@stromnov Andrew, any thoughts relative to this pull request? Good, bad, ...?

@stromnov
Copy link
Member

stromnov commented Jan 2, 2021

@mascguy Looks good, but I'm a little worried about compatibility in the dependent ports.

@mascguy
Copy link
Member Author

mascguy commented Jan 3, 2021

@mascguy Looks good, but I'm a little worried about compatibility in the dependent ports.

That's certainly a reasonable concern, and one that I had too.

My testing approach was as follows:

  • opencv: Files and such haven't moved, so less risk here. But I rebuilt some dependencies, and ran basic sanity testing.
  • opencv4: Fundamental changes were made, like the location of libraries, and switch from static to dynamic. So I rebuilt the dependent ports (python, py-pytorch, and gerbil), and also ran tests against them. I also verified that the new opencv4 dylibs were being loaded by processes, and exercised functionality. Everything looks good.

While it's not practical to cover all functionality, I'm confident that things are working properly overall.

What do you think?

@mascguy mascguy force-pushed the mascguy-opencv-opencv4-gerbil-squashed branch from 9a88c08 to 80590e2 Compare January 8, 2021 17:34
@mascguy
Copy link
Member Author

mascguy commented Jan 8, 2021

Squashed all changes down into a single commit, and ensured all fixed tickets referenced properly for autoclose.

@mascguy
Copy link
Member Author

mascguy commented Jan 8, 2021

@stromnov Andrew, thoughts regarding my testing/validation approach?

@mascguy mascguy force-pushed the mascguy-opencv-opencv4-gerbil-squashed branch 4 times, most recently from 46ecd8c to 24a9f6c Compare January 8, 2021 19:21
@mascguy
Copy link
Member Author

mascguy commented Jan 8, 2021

@kencu Ken, I've fixed the indentation issues. Any final thoughts?

…s; update gerbil and py-pytorch for opencv4 changes

opencv: eliminate conflict with opencv4; add variants debug, nonfree, tests
opencv4: eliminate conflict with opencv; switch to shared libs; rename variant opencv_contrib to contrib, to match opencv; add variants debug, java, python35..39, qt4, qt5, tbb, tests, vtk
gerbil: update for opencv4 library move
py-pytorch: update for opencv4 library move

Fixes: https://trac.macports.org/ticket/61912
Fixes: https://trac.macports.org/ticket/61801
Fixes: https://trac.macports.org/ticket/60118
Fixes: https://trac.macports.org/ticket/58845
Fixes: https://trac.macports.org/ticket/57640
Fixes: https://trac.macports.org/ticket/51734
Fixes: https://trac.macports.org/ticket/48218
Fixes: https://trac.macports.org/ticket/32528
@mascguy mascguy force-pushed the mascguy-opencv-opencv4-gerbil-squashed branch from 24a9f6c to a606de1 Compare January 8, 2021 21:18
@mascguy
Copy link
Member Author

mascguy commented Jan 9, 2021

Folks, if anyone has any final objections, please let me ASAP. Thanks!

@kencu
Copy link
Contributor

kencu commented Jan 9, 2021

It's huge PR, with many changes, and many ports will break if opencv breaks. We'll have to have some time to test this, on every system.

Please don't make any more changes while we do, as then we'll have to start all over again and test everything again :> This could take a week or two to get it built on all the systems.

The CI system seems just useless at present, unfortunately, and I can't tell what is and what is not building with that.

@mascguy
Copy link
Member Author

mascguy commented Jan 9, 2021

It's huge PR, with many changes, and many ports will break if opencv breaks. We'll have to have some time to test this, on every system.

Please don't make any more changes while we do, as then we'll have to start all over again and test everything again :> This could take a week or two to get it built on all the systems.

Sounds good, I'm ready to call it a day on this one.

In terms of the testing, is it all automated? And is there any way for me to monitor/view the progress?

@mascguy
Copy link
Member Author

mascguy commented Jan 15, 2021

While I'm not trying to rush this - there's no question that it needs to be well-vetted - eliminating the conflict between these two ports will also be a huge win for our users.

How are the builds proceeding?

@kencu
Copy link
Contributor

kencu commented Jan 15, 2021

while (DAY_JOB > 12h/DAY) (
  pause_opencv_testing();
)

@mascguy
Copy link
Member Author

mascguy commented Jan 15, 2021

while (DAY_JOB > 12h/DAY) (
  pause_opencv_testing();
)

LOL, fair enough... no rush.

@mascguy
Copy link
Member Author

mascguy commented Jan 15, 2021

And let me know if there's anything I can do to help!

@kencu
Copy link
Contributor

kencu commented Jan 16, 2021

10.14:

$ port -v installed | grep opencv
  opencv @3.4.13_1 (active) platform='darwin 18' archs='x86_64' date='2021-01-15T23:39:28-0800'
  opencv4 @4.5.0_0 (active) platform='darwin 18' archs='x86_64' date='2021-01-15T23:45:44-0800'

@mascguy
Copy link
Member Author

mascguy commented Jan 16, 2021

In terms of my testing strategy, I ran through so many port installs across my four daily drivers, that I lost count. (Those are macOS 10.12 through 10.15, all physical installs.)

To elaborate a bit more, I went through the following iterations:

  • Install of these two ports "stock," with no variants selected;
  • Installs with the most popular variants selected (all enabled together), except for contrib. Note that the Python variant can't match between them, as only one can install opencv bindings at one time [for a given Python release]:
    • opencv: +eigen +gdal +java +nonfree +opencl +python39 +qt5 +tbb +vtk
    • opencv4: +eigen +gdal +java +nonfree +opencl +python38 +qt5 +tbb +vtk
  • Installs with everything in the former, along with +contrib.

Of note, through all of that testing, I did encounter build failures when enabling contrib, for opencv. However, that turned out to be from the 3.4.13 upgrade, prior to this PR.

In any case, all of the above was done numerous times, across macOS 10.12 through 10.15.

As part of that, I also installed a few key dependents on these ports, in particular py-pytorch (py39-pytorch for opencv, and py38-pytorch for opencv4) and gerbil (for opencv4). And then ran tests, with both a simple python program, as well as some gerbil runs.

Once I was confident with all of that (many days later), I added macOS 10.6 and 10.8 to the mix, via two Parallels VMs.

So while I certainly haven't covered every conceivable testing case, I really spent mucho time on this one. But it'll be worth it in the long run, as having opencv ports that can coexist will be a huge boon for users!

Thoughts?

@mascguy
Copy link
Member Author

mascguy commented Jan 16, 2021

Oh, in terms of outstanding issues, there's just one found during my testing: opencv4 is failing to build on 10.6. But that's not a new issue, as it also exists with the existing opencv4 port. I'm not sure how feasible it is to fix.

@kencu
Copy link
Contributor

kencu commented Jan 16, 2021

OK. Enough testing it seems then.

@kencu kencu merged commit ca9ffa6 into macports:master Jan 16, 2021
@mascguy
Copy link
Member Author

mascguy commented Jan 16, 2021

Beautiful! Thanks as always Ken!

@mascguy
Copy link
Member Author

mascguy commented Jan 17, 2021

I've been monitoring the build progress of opencv, opencv4, py-torch, and gerbil... and everything looks good. Still waiting for the 10.14 buildbot to catch up on its backlog, but otherwise, everything has built successfully where expected.

Not surprising, but still great to see!

@mascguy mascguy deleted the mascguy-opencv-opencv4-gerbil-squashed branch January 18, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 participants