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 migrations #16640

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
3 participants
@fxcoudert
Copy link
Member

fxcoudert commented Aug 10, 2017

Starting again on opencv migrations.

Open issues that need to be dealt with:

(other issues are old, closed, CUDA-related or do not provide enough information to act upon)

@fxcoudert fxcoudert requested a review from ilovezfs Aug 10, 2017

@fxcoudert

This comment has been minimized.

Copy link
Member Author

fxcoudert commented Aug 10, 2017

poke @iMichka

Formula/opencv.rb Outdated
@@ -0,0 +1,158 @@
class Opencv3 < Formula

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

class should be opencv

This comment has been minimized.

Copy link
@iMichka

iMichka Aug 10, 2017

Member

And the other one misses AT2

Formula/opencv.rb Outdated
class Opencv3 < Formula
desc "Open source computer vision library, version 3"
homepage "http://opencv.org/"
revision 1

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

not needed since it's an upgrade 2.4.13.2 -> 3.2.0

Formula/opencv@2.rb Outdated
@@ -0,0 +1,98 @@
class Opencv < Formula

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

class needs to be opencvat2

@fxcoudert fxcoudert force-pushed the fxcoudert:opencv_again branch Aug 10, 2017

Formula/opencv.rb Outdated
depends_on :java => :optional
depends_on :python3 => :optional

with_python = build.with?("python") || build.with?("python3")

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

numpy only has "without" options now so no options should be needed

Formula/opencv@2.rb Outdated
homepage "http://opencv.org/"
url "https://github.com/opencv/opencv/archive/2.4.13.2.tar.gz"
sha256 "4b00c110e6c54943cbbb7cf0d35c5bc148133ab2095ee4aaa0ac0a4f67c58080"
revision 1

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

this is technically a new formula

@iMichka

This comment has been minimized.

Copy link
Member

iMichka commented Aug 10, 2017

I think the python3 thing is solved by this PR that got abandoned: Homebrew/brew#2499

It tells you to setup your path and stuff to make it work.
But I never really tested all this myself, but once we have a clean build here I can give it a shot locally.

Formula/opencv.rb Outdated
sha256 "8a067e3e026195ea3ee5cda836f25231abb95b82b7aa25f0d585dc27b06c3630"
end

def arg_switch(opt)

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

this is not idiomatic in core

Formula/opencv@2.rb Outdated
depends_on :java => :optional
depends_on :ant if build.with? "java"

def arg_switch(opt)

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

this is not idiomatic in core

Formula/opencv.rb Outdated

option :cxx11

depends_on :ant => :build if build.with? "java"

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

the java option doesn't exist yet up here. it has to be below depends_on :java => :optional

@iMichka

This comment has been minimized.

Copy link
Member

iMichka commented Aug 10, 2017

Do we want to consider the update to version 3.3.0 or are we doing this in a separate step? Homebrew/homebrew-science#6078

It removes a bunch of code, which is nice.

Formula/opencv@2.rb Outdated
depends_on "ffmpeg" => :optional
depends_on "tbb" => :optional
depends_on :java => :optional
depends_on :ant if build.with? "java"

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

should be marked as => :build

Formula/opencv.rb Outdated
option "with-contrib", 'Build "extra" contributed modules'
option "with-java", "Build with Java support"
option "with-tbb", "Enable parallel code in OpenCV using Intel TBB"
option "without-numpy", "Use a numpy you've installed yourself instead of a Homebrew-packaged numpy"

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

I don't think we need this

Formula/opencv.rb Outdated
option "without-numpy", "Use a numpy you've installed yourself instead of a Homebrew-packaged numpy"
option "without-python", "Build without Python support"

option :cxx11

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

can we just make this the default?

Formula/opencv.rb Outdated
depends_on "ffmpeg" => :optional
depends_on "tbb" => :optional
depends_on :java => :optional
depends_on :python3 => :optional

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

can we build with support for both?

This comment has been minimized.

Copy link
@iMichka

iMichka Aug 10, 2017

Member

There is only one PYTHONPATH; and opencv relies on it. Supporting Python 2 and 3 together broke people's build when using Python 3. This is why there is a check for that somewhere in the formula. But I would be willing to remove it and see what happens.

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

If we can't support both, I think we need to just pick one. This formula shouldn't be built from source as a matter of course.

This comment has been minimized.

Copy link
@iMichka

iMichka Aug 10, 2017

Member

Let's try to re-enable both and see what happens :)

Formula/opencv.rb Outdated

if build.with? "python3"
# Reset PYTHONPATH, workaround for https://github.com/Homebrew/homebrew-science/pull/4885
ENV["PYTHONPATH"] = ""

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

I'd probably just put ENV.delete("PYTHONPATH") at the beginning of install and leave it at that.

Formula/opencv.rb Outdated
args << "-DENABLE_AVX2=ON" if Hardware::CPU.avx2?
end

inreplace buildpath/"3rdparty/ippicv/downloader.cmake",

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

what's going on here? has this been reported?

This comment has been minimized.

Copy link
@iMichka

iMichka Aug 10, 2017

Member

opencv 3.3.0 fixes this. See PR in homebrew science.

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

We should upgrade to that as part of the same PR.

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

If 3.3.0 goes in science first that's OK.

Formula/opencv.rb Outdated
"${OPENCV_ICV_PLATFORM}"
resource("icv-macosx").stage buildpath/"3rdparty/ippicv/downloads/macosx"

mkdir "macbuild" do

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

build

Formula/opencv.rb Outdated

mkdir "macbuild" do
system "cmake", "..", *args
system "make"

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

does just make install fail?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Aug 10, 2017

we should add a formula rename for opencv3 to opencv

Formula/opencv.rb Outdated
@@ -0,0 +1,157 @@
class Opencv < Formula
desc "Open source computer vision library, version 3"
homepage "http://opencv.org/"

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

actually does need revision 1 if we're going to do the formula rename from opencv3

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

We should upgrade to 3.3.0 as part of the same PR, so it won't be needed.

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

(If 3.3.0 goes in science first that's OK, and then we can revision bump here.)

Formula/opencv.rb Outdated
end
end

head do

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

I think we have enough problems with these formulae not to need head.

This comment has been minimized.

Copy link
@iMichka

iMichka Aug 10, 2017

Member

There are some websites/blog posts out there recommending to build opencv with --head. Never understood why. But it allowed me to close tens issues just based on the fact that it was head. So +1 on removing it.

Formula/opencv.rb Outdated
end
end

option "with-contrib", 'Build "extra" contributed modules'

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

feels out of scope since quality control is going to be low

Formula/opencv@2.rb Outdated
args << "-DENABLE_AVX=ON" if Hardware::CPU.avx?
end

mkdir "macbuild" do

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

build

Formula/opencv@2.rb Outdated

mkdir "macbuild" do
system "cmake", "..", *args
system "make"

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

does just make install fail

Formula/opencv.rb Outdated
pythons = build.with?("python3") ? ["with-python3"] : []
depends_on "numpy" => [:recommended] + pythons if with_python

# Dependencies use fortran, which leads to spurious messages about gcc

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

is this still a thing?

Formula/opencv.rb Outdated
# Dependencies use fortran, which leads to spurious messages about gcc
cxxstdlib_check :skip

resource "icv-macosx" do

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

can we do without this? It says third party

Formula/opencv.rb Outdated
depends_on "libtiff"
depends_on "openexr"
depends_on :python => :recommended unless MacOS.version > :snow_leopard
depends_on "ffmpeg" => :optional

This comment has been minimized.

Copy link
@ilovezfs

ilovezfs Aug 10, 2017

Contributor

any reason not to make this default on?

@fxcoudert fxcoudert force-pushed the fxcoudert:opencv_again branch 5 times, most recently Aug 13, 2017

@fxcoudert

This comment has been minimized.

Copy link
Member Author

fxcoudert commented Aug 13, 2017

@BrewTestBot test this please

@fxcoudert

This comment has been minimized.

Copy link
Member Author

fxcoudert commented Aug 13, 2017

@iMichka how do you feel about the current state? are we ready to merge?

@iMichka

This comment has been minimized.

Copy link
Member

iMichka commented Aug 13, 2017

I'll build it locally and give it a quick try, I want to see what happens with the python2/python3 import problems reported in science. I have some time now, so I'll report back to you in 1 or 2 hours.

Is there a way to not have the following error:

W: 107: col 45: Unused block argument - python. You can omit all the arguments if you don't care about them.

Can you set the variable to something like unused or _ to prevent the audit to fail (don't know if that works in the ruby world but this is how I do it in Python).

@iMichka

This comment has been minimized.

Copy link
Member

iMichka commented Aug 13, 2017

Also we need a PR in science to remove opencv/opencv3. You closed Homebrew/homebrew-science#5996, so either it needs to be re-opened or a new one needs to be created.

@iMichka

This comment has been minimized.

Copy link
Member

iMichka commented Aug 13, 2017

Ok so I built the formula as is. import cv2 works in python3. It is weird that this is called cv2 as the formula is now opencv (3.3.0). But that is probably upstream's decision.

The formula does not build python2 bindings. This is probably due to this line:

depends_on :python => :recommended if MacOS.version <= :snow_leopard

Is this the wanted behaviour? There is an option to build without python2, but this would only be useful under Snow Leopard or older?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Aug 14, 2017

@iMichka that line isn't relevant. It should build the python2 bindings due to

  option "without-python", "Build without python2 support"

See the numpy formula and the scipy formula for other examples.

The formula probably needs to use Language::Python.each_python(build) do |python, version| for it to work properly.

The same should be used in the test block.

@fxcoudert

This comment has been minimized.

Copy link
Member Author

fxcoudert commented Aug 20, 2017

@iMichka this is actually working for me:

$ python2.7
Python 2.7.13 (default, Jul 18 2017, 09:17:00) 
[GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import site; site.addsitedir("/usr/local/lib/python2.7/site-packages")
>>> import cv2
>>> print(cv2.__version__)
3.3.0

Both python2 and python3 bindings get built.

@iMichka

This comment has been minimized.

Copy link
Member

iMichka commented Aug 20, 2017

[michkapopoff:~] % python
Python 2.7.13 (default, Jul 18 2017, 09:17:00) 
[GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import cv2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named cv2
>>> import site; site.addsitedir("/usr/local/lib/python2.7/site-packages")
>>> import cv2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named cv2

Python2 does not got built:
https://gist.github.com/a36bcf6c96622544bc3bc240dace386e

Only the python3 .so gets installed
Installing: /usr/local/Cellar/opencv/3.3.0/lib/python3.6/site-packages/cv2.cpython-36m-darwin.so

@iMichka

This comment has been minimized.

Copy link
Member

iMichka commented Aug 20, 2017

You can also see this the configuration section:

General configuration for OpenCV 3.3.0 =====================================

--   Python 2:
--     Interpreter:                 /usr/bin/python (ver 2.7.10)
--     Libraries:                   NO
--     numpy:                       NO (Python wrappers can not be generated)
--     packages path:               lib/python2.7/site-packages
-- 
--   Python 3:
--     Interpreter:                 /usr/local/opt/python3/bin/python3 (ver 3.6.2)
--     Libraries:                   /usr/local/opt/python3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/config-3.6m-darwin/libpython3.6.dylib (ver 3.6.2)
--     numpy:                       /usr/local/lib/python3.6/site-packages/numpy/core/include (ver 1.13.1)
--     packages path:               lib/python3.6/site-packages

It picks up the wrong Python 2 (system python).

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Aug 20, 2017

It should say

--   Python 2:
--     Interpreter:                 /usr/bin/python (ver 2.7.10)
--     Libraries:                   /System/Library/Frameworks/Python.framework/Versions/2.7/lib/libpython2.7.dylib (ver 2.7.10)
--     numpy:                       /usr/local/lib/python2.7/site-packages/numpy/core/include (ver 1.13.1)
--     packages path:               lib/python2.7/site-packages

@fxcoudert fxcoudert force-pushed the fxcoudert:opencv_again branch Aug 20, 2017

@iMichka

This comment has been minimized.

Copy link
Member

iMichka commented Aug 20, 2017

brew doctor told me that I had an unlinked numpy in my Cellar. Cleaning that up and removing all the other numpys installed with pip, the python2 bindings got built and installed. Imports in python 3 and 2 worked correctly. All good for me then.

EOS
system ENV.cxx, "test.cpp", "-I#{include}", "-L#{lib}", "-o", "test"
assert_equal version.to_s, shell_output("./test").strip
assert_match version.to_s, shell_output("python -c 'import cv2; print(cv2.__version__)'")

This comment has been minimized.

Copy link
@iMichka

iMichka Aug 20, 2017

Member

Can the opencv@2 test block be also run for both Python versions with Language::Python.each_python(build) do |python, _version|?

This comment has been minimized.

Copy link
@iMichka

iMichka Aug 20, 2017

Member

Err no please disregard that comment, it does not have python3 support anyway :)

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Aug 20, 2017

  * W: 108: col 45: Unused block argument - `python`. You can omit all the arguments if you don't care about them.

@fxcoudert fxcoudert force-pushed the fxcoudert:opencv_again branch to 461ed02 Aug 20, 2017

@iMichka
Copy link
Member

iMichka left a comment

+1 good to go :)

@ilovezfs ilovezfs closed this in 7acce76 Aug 20, 2017

@fxcoudert fxcoudert deleted the fxcoudert:opencv_again branch Aug 24, 2017

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.