OpenCV 2.x #189
OpenCV 2.x #189
Conversation
Haven't fully verified the Windows build yet as I changed the DLL outpath to LIBRARY_BIN.
|
@jakirkham I need to only run Python 2.7 builds - how do I ensure that happens? |
| - osx_rpath.patch # [osx] | ||
|
|
||
| build: | ||
| number: 0 |
There was a problem hiding this comment.
Adding the following would ensure Python 2.7 builds only.
skip: true # [not py27]
Thanks @jakirkham!
|
You're no doubt more familiar with what is going on with Windows. Though you may still find this handy. |
| number: 0 | ||
| skip: true # [not py27] | ||
| features: | ||
| - vc9 # [win and py27] |
There was a problem hiding this comment.
I think this isn't needed if we have made python a build and runtime dependency as python tracks this feature.
There was a problem hiding this comment.
Noted - honestly, I feel like a dinosaur when it comes to the latest conda build stuff 😛 I haven't quite caught up with all the features stuff.
Remove features since we already rely on Python which means we don't require the features.
|
@jakirkham Very useful comments - thanks! I'm still learning the new way of doing things but I thought that 1) OpenCV is a useful package to have and 2) This way I can learn how to do things properly before I bring in other recipes I have. |
Glad to help. 😄
👍 Absolutely! Having it in here will definitely make this easier for me to use and likely others.
Definitely, I hope we are helping with this. 😉 |
| build: | ||
| - cmake | ||
| - numpy x.x | ||
| - python 2.x |
There was a problem hiding this comment.
Should drop these constraints on Python here. This skip above will correctly handle this in the system here.
|
|
||
| about: | ||
| home: http://opencv.org/ | ||
| license: BSD |
There was a problem hiding this comment.
Need a summary field. Just a short description.
There was a problem hiding this comment.
Also, we like to specify license version just to clarify anything. So, I believe OpenCV is BSD 3-Clause. Though you could say BSD New or BSD Simplified. Whatever will allow for an easy Google to clarify. 😄
| @@ -0,0 +1,38 @@ | |||
| package: | |||
| name: opencv | |||
| version: 2.4.11 | |||
There was a problem hiding this comment.
Would be nice to set this with jinja templates so it is easy to update. Here is a simple example.
|
Added some style comments above to give you a better feel for how these look. We have a linter on Travis (the Linux job), which will point out some simple issues like these, as well. Like having maintainers or specifying a summary. Though we are continually working on it to help improve this feedback. Thoughts on this are, of course, welcome. |
| -DBUILD_PNG=1 \ | ||
| -DBUILD_OPENEXR=1 \ | ||
| -DBUILD_JASPER=1 \ | ||
| -DBUILD_JPEG=1 \ |
There was a problem hiding this comment.
I see some of these libraries here also ship with conda (e.g libpng, zlib, libtiff, libjpeg and others). We should definitely include these as build and runtime requirements. For any of these that are not here (e.g. OpenEXR), we might want to disable for now, but add a note that we need to add a recipe for them.
There was a problem hiding this comment.
Hmm. If my GIT history is correct this is actually problematic. OpenCV is notoriously poor at properly supporting Python (particularly VS 2008) and if you try and link against external libraries then the OpenCV CMake setup is not able to correctly rectify the correct headers with the correct libraries. For example, I had a comment in my original repo that I turned to building everything because OpenCV was using the shipped Windows LIBPNG headers but linking against conda LIBPNG, causing a segfault at run time.
|
So, added a bunch of style comments. Also, I think I missed this before (sorry about that), but the |
| -DWITH_FFMPEG=0 \ | ||
| -DCMAKE_INSTALL_PREFIX=$PREFIX | ||
| make -j${CPU_COUNT} | ||
| make install |
There was a problem hiding this comment.
Could we run some sort of make check, make test, or ctest command here? Whatever happens to be available and does a decent job of testing the library.
There was a problem hiding this comment.
Will have to look into that as I've never run the tests before!
Edit: Looks like its kind of a pain - Need to download the test data and then run a Python script (OPENCV_TEST_DATA_PATH=/path/to/above/zip $PYTHON modules/ts/misc/run.py -a build) from the recipe root. OpenCV does not use ctest/make test/make check.
|
@jakirkham As you can see - the builds are currently failing because there are no Eigen or TBB packages on main/conda-forge yet. How is it best to go about rectifying this? Pause this recipe and get the Eigen and TBB recipes in? |
|
@jakirkham Thanks for adding the Eigen recipe. The SHA1 is wrong at the moment - but then we will see if it passes because the tests fail on my laptop! If Windows fails, it's probably because of not setting the generator correctly - but we can easily rectify that by stealing from one of the other recipes. I'm not near a Windows PC right now but I can try things out tomorrow if necessary. |
Yeah, copypasta. Since fixed.
Almost sure I have messed up Windows. Will work on that thanks to your suggestions. |
Still need to test on Windows.
Windows builds seem to work
Also, steal tests from Homebrew rather than running the OpenCV tests because there is far too much data to download for the tests. Finally, remove TBB for now to get the recipe in
|
Hi! This is the friendly conda-forge-admin automated user. I just wanted to let you know that I linted all conda-recipes in your PR ( |
| {% for each_opencv_lib in opencv_libs %} | ||
| - test -f $PREFIX/lib/libopencv_{{ each_opencv_lib }}.dylib # [osx] | ||
| - test -f $PREFIX/lib/libopencv_{{ each_opencv_lib }}.so # [linux] | ||
| {% endfor %} |
There was a problem hiding this comment.
As another test, can we verify that FindOpenCV.cmake got installed correctly? Ran into an issue with the exist (defaults) OpenCV install where this was not the case. Would be nice if we could have this out of the gate with our version.
There was a problem hiding this comment.
Apparently there is no FindOpenCV.cmake - this functionality is provided by $PREFIX/share/OpenCV/OpenCVConfig.cmake - at least that is what I gather from here. This file is currently installed, but it installs to this share folder. Do we want it there? There doesn't seem to be a standard place to put this stuff? I've also got other packages locally that has installed to places such as $PREFIX/lib, $PREFIX/PACKAGE/lib and $PREFIX/cmake. @jakirkham
|
Could we please add |
|
|
||
| :TRIMSUB | ||
| set tempvar=%* | ||
| GOTO :eof |
Also, use Jasper from the feedstocks on Unix
But this seems to fail on my Mac because $PYTHON points to _build rather than _test !!??
|
Hi! This is the friendly conda-forge-admin automated user. I just wanted to let you know that I linted all conda-recipes in your PR ( |
1 similar comment
|
Hi! This is the friendly conda-forge-admin automated user. I just wanted to let you know that I linted all conda-recipes in your PR ( |
| - libpng # [unix] | ||
|
|
||
| test: | ||
| imports: |
There was a problem hiding this comment.
Add the following...
requirements:
- python
|
@jakirkham Now that Eigen is in - is it possible to re-start the circleci build? |
|
Absolutely! 😄 Have restarted Circle and Travis. As we don't have AppVeyor builds yet, haven't restarted that yet. |
|
Nice. They are now passing. 😄 Let me go back through this again and we'll see where we stand. |
| -DWITH_CUDA=0 \ | ||
| -DWITH_OPENCL=0 \ | ||
| -DWITH_OPENNI=0 \ | ||
| -DWITH_FFMPEG=0 \ |
There was a problem hiding this comment.
Let's not worry about this for the first iteration. If that's ok.
|
Basically happy with this. One minor nit. Also, thinking one last time about testing, but if we can't figure something out it is not a big deal. Otherwise, I know Windows hasn't been tested as we are waiting for Eigen. Though I would be happy merging if the nit is fixed and we think one last time about testing. |
|
Hi! This is the friendly conda-forge-admin automated user. I just wanted to let you know that I linted all conda-recipes in your PR ( |
|
Ok, not waiting for AppVeyor as we know the issue is Eigen is missing. If it turns out there are more issues for Windows after that missing dependency is resolved, we can handle them in the feedstock. Thanks again for putting this together @patricksnape. |
Haven't fully verified the Windows build yet as I changed the
DLL outpath to LIBRARY_BIN.
This is taken from the Menpo builds which means it may not build as I'm not sure if TBB and Eigen are available globally yet.