[bug#30801] [PATCH 0/1] Add opencv

["Björn Höfling" <bjoern.hoefling@bjoernhoefling.de>]

[-- Attachment #1: Type: text/plain, Size: 7699 bytes --]

Hi Ludo,

thanks for reviewing!

On Thu, 15 Mar 2018 22:04:54 +0100
ludo@gnu.org (Ludovic Courtès) wrote:

> Hi Björn,
> 
> Björn Höfling <bjoern.hoefling@bjoernhoefling.de> skribis:
> 
> > The test suite consists of an extra package, weighting 465MB
> > compressed. It runs very well. I think the size is worth it. It
> > consists of proprietary things (i.e. lena.jpg). As far as I
> > understand, that is OK, if it doesn't get in the final src/bin
> > store output. Right?  
> 
> As a rule of thumb, there should not be non-free stuff in the
> derivation graph.
> 
> If there’s non-free software, that’s not OK, even if it doesn’t show
> up in the output.
> 
> If it’s “just” data (pictures) that are non-free, that’s OK per the
> FSDG:
> <https://www.gnu.org/distros/free-system-distribution-guidelines.html#non-functional-data>.
> If we could replace it with a free variant, I think we should clearly
> encourage it, but lena.jpg is hardly replaceable in this context (I’d
> hope it weren’t around for what it tells about CS, but that’s another
> story…).

What is being downloaded is in the store of cause, but it is only
images and videos, no code. And it is not linked against.

The suite is 400 MB in size and replacing Lena with Linus or even
better pictures and videos of trees would need a fundamental idea of
the algorithms behind OpenCV, with I don't have. So for now I just leave
it as is, though I agree with your fundamental ideas :-)

 
> > CPU-optimization: I hope I have done everything right. Reading the
> > article from Guix HPC a second time helped a lot. So now it should
> > be compiled with SSE2/NEON being the minimum required instruction
> > set, and dispatches to other ISAs where available.  
> 
> Great.
> 
> > Size: Currently I load a bunch of dependencies in and have one
> > big  package as output. guix size is 1.1 GiB. I slightly have the
> > feeling someone could ask to split it in several outputs. Though
> > having one big output was the easiest thing first and I don't know
> > how one would handle inter-dependencies between the different
> > outputs.  
> 
> 1.1G is a bit too much IMO, indeed.  :-)
> 
> How much is OpenCV itself?  If OpenCV itself is big, it would be nice
> to look at what’s taking up space in there with ‘du’.  For instance,
> if there are .a files, we might want to not build them and keep only
> shared objects.
> 
> Splitting in separate outputs may or may not make sense.  If there are
> examples or large pieces of HTML doc, introducing a “doc” and/or an
> “examples” output may help.  Likewise, if there are binaries that
> depend on more stuff that the library itself, introducing a “lib”
> output might be a good idea.
> 
> Could you take a look?

I don't know which of the store items was my latest one, I'm currently
recompiling but that takes 1-2 hours to finish.

Most are 50MB, one is 265MB, I suppose that was the latest with all
dependencies and all (free) OpenCV modules. So the 1.1GB was
definitively the convex hull including all dependencies.

The whole thing is more or less only "lib".

I forgot to mention that I ignored the "doc" package: It built one when
I added doxygen as a dependency, but it did not install it. Now that I
know how to manually copy/install files, I could give that another try.
But that would add (to a :doc output, of cause), not substract.

The "bin" part is only 7 MB, so not worth mentioning. There are some
examples which I did not include, because they are not very interesting
in the compiled version, but more for understanding the programming
part from the source code.

OK, I looked into some other packages and tried to split it into pieces
(one piece for now). What I do is just add a phase after install and
copy things manually (copied from the git package definition):

+       (add-after 'install 'split
+         (lambda* (#:key outputs #:allow-other-keys)
+           (let* ((outputs-out (assoc-ref outputs "out"))
+                  (outputs-feature2d (assoc-ref outputs "feature2d"))
+                  (outputs-face  (assoc-ref outputs "face"))
+                  (outputs-core (assoc-ref outputs "core"))
+                  (libface     (string-append outputs-out "/lib/libopencv_face.so"))
+                  (libface*    (string-append outputs-face "/lib/libopencv_face.so"))
+                  (libface3.4     (string-append outputs-out "/lib/libopencv_face.so.3.4"))
+                  (libface3.4*    (string-append outputs-face "/lib/libopencv_face.so.3.4"))
+                  (libface3.4.1     (string-append outputs-out "/lib/libopencv_face.so.3.4.1"))
+                  (libface3.4.1*    (string-append outputs-face "/lib/libopencv_face.so.3.4.1")))
+
+                  
+                  (mkdir-p (string-append outputs-face "/lib"))
+                  
+                  (for-each (lambda (old new)
+                              (copy-file old new)
+                              (delete-file old)
+                              (chmod new #o555))
+                            (list libface libface3.4 libface3.4.1)
+                            (list libface* libface3.4* libface3.4.1*))
+                  )
+           #t ; TODO: Implement it.
+           ))
+       )))

The problem here is that this doesn't correct the RPATHS and I will get
problems with dependent library parts (The python modules in that case):

phase `strip' succeeded after 0.5 seconds
starting phase `validate-runpath'
validating RUNPATH of 46 binaries in "/gnu/store/f7aqk1my2bdprhgp3gxmnl9227gxf43m-opencv-3.4.1/lib"...
/gnu/store/f7aqk1my2bdprhgp3gxmnl9227gxf43m-opencv-3.4.1/lib/python3.6/site-packages/cv2.cpython-36m-x86_64-linux-gnu.so: error: depends on 'libopencv_face.so.3.4', which cannot be found in RUNPATH ("/gnu/store/f7aqk1my2bdprhgp3gxmnl9227gxf43m-opencv-3.4.1/lib" "/gnu/store/124ymrzp0dwx6qfh4r4r4763sa5k48sv-libsm-1.2.2/lib" "/gnu/store/dbdjmralkrzqn6b093hp69bjljvfr7zm-libice-1.0.9/lib" "/gnu/store/g7sak8qzk7lk06ggn38xpfv5mb8da6kk-libxt-1.1.5/lib" "/gnu/store/n6acaivs0jwiwpidjr551dhdni5kgpcr-glibc-2.26.105-g0890d5379c/lib" "/gnu/store/xfjba1kww8ngdc6nxldd8ly93nh13ayy-gcc-5.5.0-lib/lib" "/gnu/store/xfjba1kww8ngdc6nxldd8ly93nh13ayy-gcc-5.5.0-lib/lib/gcc/x86_64-unknown-linux-gnu/5.5.0/../../..")
validating RUNPATH of 7 binaries in "/gnu/store/f7aqk1my2bdprhgp3gxmnl9227gxf43m-opencv-3.4.1/bin"...
validating RUNPATH of 3 binaries in "/gnu/store/1356brvn9cjilcp2zizm9gx3vigmz74i-opencv-3.4.1-face/lib"...
phase `validate-runpath' failed after 0.3 seconds

So, somehow I have to solve dependencies between the different outputs.
I have no idea how to do that. For a :doc output, that doesn't matter,
but how can I tell Guix the dependency hierachy of different outputs?

 
> Nitpick:
> 
> > +    (synopsis "Computer Vision Library")  
> 
> No need to capitalize.

OK.

 
> > +    (description "OpenCV (Open Source Computer Vision) is a
> > library aimed at  
> 
> I’d remove the parenthetic part.

OK.

> 
> > +real-time computer vision, including several hundred computer
> > +vision algorithms.")  
> 
> Bonus points if you can expound a little bit, without just listing
> those algorithms though.  ;-)

OK, you got me. I will add more lines when the technical problems are
solved.

 
> Apart from that the patch looks very polished and that’s a pleasure to
> review!


Thanks. It took quite some rounds until that point. But good to know
you enjoyed the result :-)

Björn

PS: Happy Easter and this is not the xxxx-04-01 fool.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]