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

opencv3/opencv4: add/update python subports; refine file layouts for consistency #9823

Merged
merged 6 commits into from
Jan 28, 2021

Conversation

mascguy
Copy link
Member

@mascguy mascguy commented Jan 27, 2021

Description

  • opencv4: Refine python subports, which were just added via merged PR 6261. Also re-add variants 'gdal' and 'opencl', which were inadvertently removed.
  • opencv3: Migrate python variants to subports, for consistency with opencv4.
  • Add notes to both opencv3 and opencv4, pointing out change to users.
  • gerbil: Update for opencv4 file layout change.
  • py-pytorch: Update for opencv4 file layout change; add ccache support.
  • py-imutils: Update for renamed opencv4 python subport.

Note to Maintainers:

  • opencv3: Since this port is brand-new as of a week ago, the switch from python variants to subports is unlikely to impact any users.
  • opencv4: Python binding support is still quite new, having been added within the past two weeks. So again, user impact is minimal.
  • Nonetheless, notes have been added to both, mentioning the change.
Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.8.5 12F2560
Xcode 5.1.1 5B1008

macOS 10.10.5 14F2511
Xcode 6.4 6E35b

macOS 10.11.6 15G22010
Xcode 7.3.1 7D1014

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

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?

@macportsbot
Copy link

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

@mascguy
Copy link
Member Author

mascguy commented Jan 27, 2021

Maintainers: Since port 'py-pytorch' is included, there's a good change the CI jobs may timeout and fail. Hopefully not, but having just gone through this with another PR... there's a good chance that will happen.

@kencu
Copy link
Contributor

kencu commented Jan 27, 2021

see the cmake PG options that are meant to be used instead of setting the args directly, such as

default cmake.install_prefix {${prefix}}

@mascguy
Copy link
Member Author

mascguy commented Jan 27, 2021

see the cmake PG options that are meant to be used instead of setting the args directly, such as
[...]

Unfortunately that only covers a few of the umpteen CMake flags needed for these ports. But that type of cleanup will be looked at when opencv3 and opencv4 are finally merged together.

@mascguy
Copy link
Member Author

mascguy commented Jan 27, 2021

Added use of cmake portgroup, to cleanup tracking ticket:

opencv/opencv4: eliminate portfile duplication, via subports

@essandess
Copy link
Contributor

Would you also please add a commit for py-imutils that changes this dependency to port:py${python.version}-opencv4?

port:py${python.version}-opencv \

@mascguy
Copy link
Member Author

mascguy commented Jan 27, 2021

Would you also please add a commit for py-imutils that changes this dependency to port:py${python.version}-opencv4?

Ah, that might help too... :-)

Done.

@mascguy
Copy link
Member Author

mascguy commented Jan 27, 2021

Alas, the last CI build failed due to timeout, for port py-pytorch... as expected. No surprise though.

@mascguy
Copy link
Member Author

mascguy commented Jan 27, 2021

Ken, are you okay with deferring cleanup via use of portgroup cmake? As much as I'd love to tackle that ASAP, I'm hesitant to make any more changes to this PR...

@kencu
Copy link
Contributor

kencu commented Jan 27, 2021

Well, here's the deal. The cmake 1.1. PortGroup adds a whole bunch of:

configure.pre_args-append -DCMAKE_THINGAMABOB=MacPorts-default-thingamabob

When you also add:

configure.args-append -DCMAKE_THINGAMABOB=my-preferred-thingamabob

the cmake 1.1 PortGroup has no clue you did that, so it adds it's own things as well.

Now -- the cmake 1.1 PortGroup puts it's thingamabobs in configure.pre_args, and you put yours in configure.args, so if we're lucky, yours come after, and trash the earlier ones added by the PortGroup.

But this is just plain ugly, right?

SO I would prefer if you take 5 minutes, and for args that the cmake PortGroup controls with those options I linked to, just set them that way instead, which is just better, cleaner, shorter, nicer, etc.

If there is also some additional arg you really want to set that there is no cmake PortGroup option for, then add those by hand in the configure.args, and you won't be competing with the cmake portgroup.

@mascguy
Copy link
Member Author

mascguy commented Jan 27, 2021

Ken, cmake-related changes completed. While it only eliminated a few of the configure arguments, it also gave me the opportunity to clean up path copy-paste... which was long-overdue anyway.

Take a look, and let me know what you think.

@mascguy mascguy force-pushed the mascguy-opencvX-python-subports-new branch from a8dfbbd to 5f1d975 Compare January 27, 2021 23:50
@kencu kencu merged commit 3d74dc8 into macports:master Jan 28, 2021
@mascguy mascguy deleted the mascguy-opencvX-python-subports-new branch January 28, 2021 03:05
@mascguy
Copy link
Member Author

mascguy commented Jan 28, 2021

Thanks Ken!

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

Successfully merging this pull request may close these issues.

6 participants