Bug#823140: RFS: caffe/1.0.0~rc3-1 -- a deep learning framework [ITP]
Thank you for this careful and thorough review!
On Tue, 2016-05-17 at 14:50 +0100, Ghislain Vaillant wrote:
> Dear all,
>
> On 16/05/16 15:50, Gianfranco Costamagna wrote:
> > Hi Lumin
> >
> >
> >> Done. Updated package has been uploaded to mentors:
> >> https://mentors.debian.net/package/caffe
> > 1) did you try to enable the Debug build too?
> > without CMAKE_BUILD_TYPE=RelWithDebInfo you won't be able to get automatic debug packages I think
>
> I don't think it is true (anymore?), since Debian injects its own "None"
> build configuration + DEB_*_MAINT_FLAGS.
Actually the current packaging files can successfully yield
*-dbgsym*.deb packages.
> > 2) why the python3 support is disabled? note: this will require a trip in new queue, so if possible
> > I would prefer a python3-only build
>
> Same here. Since the build system leaves you the choice, please consider
> packaging the Python 3 version. Python 2 has an expiration date anyway.
As said in Debian's python policy I'm aware of it. Let me
clarify this point here. one of the build dependencies of
python-caffe-* is python*-protobuf, and python3-protobuf
is not possible for current protobuf version in Debian.
In a word, build dependencies not satisfied for python3-caffe-*.
> > 3) all the "debian/tmp" strings in install files should/can be removed I guess
>
> Indeed, they should probably be removed.
Will do this.
> > (I didn't review all the packaging, just something that might/should be fixed.
> >
> > I'll wait for Ghislain to finish his review ;)
>
> * I am really not a fan of the templated configuration of the dh_install
> files. Worst case, do it programmatically in d/rules rather than
> declaratively with templates + a call to yet another custom script.
> AFAIC, this creates a lot of overhead for no obvious benefits from a
> team-maintenance point-of-view.
I'll fix this. Removing those template generation matter indeed
avoids a python3 script.
> * Thinking long-term solution, one should just bite the bullet and make
> the build system multi-arch aware. It's just one module in CMake
> (GnuInstallDirs) and substituting hardcoded DESTINATION paths with the
> appropriate CMake variables. I am sure upstream would accept such patch,
> and all distributions could benefit from it. I have done it multiple
> times and never encountered resistance upstream. Plus the significant
> simplification of the debian files...
Will patch it.
> * On a different note, caffe seems to support building the documentation
> from source with doxygen. Please consider packaging it in a separate
> binary package (caffe-doc). The content of examples/* and models/*
> might also fit in this -doc package.
Yeah but I'm thinking about the latex(pdf) document generation,
I don't know should I add Build-Indep on texlive or is there
a "core" package to complete doxygen latex compiling.
I'll build caffe-doc package from "caffe" source, and also recommend
"caffe-doc" package in packages from "caffe-contrib".
> * You should consider adding a packaging testsuite. You could start with
> a script running part or all the examples shipped in the -doc package,
> which would verify that the packaged software run as intended.
Caffe has complete gtest testing routings. According to my
experience of using Caffe, it should be working
if it passed the dh_auto_test.
Adding more test is easy, and I think some more testsuite
from me will benifit other maintainers since they can learn
from it how to test this software by hand.
I'll do this.
> * You should consider adding some upstream metadata as described in [1].
> I am sure the Caffe guys would appreciate that we appropriately forward
> the citation information displayed on their README.
I didn't know Debian has such a cool thing. Will do this.
> * What is the purpose of keeping unapplied patches in d/patches?
Uh I forgot to remove them ... Will fix it.
> * Missing uversionmangling in d/watch for appropriate tracking of
> release candidate releases.
Since uscan(1) describes uversionmangle I think I can manage it.
> * Consider using libblas-dev | libblas.so as Build-Depends instead of
> libopenblas-dev. Not everyone is using openblas as its prefered blas
> implementation and upstream does not force to use this specific vendor
> (do they?), so no reason to force it here either.
No, I don't agree using libblas-dev as build-dep. Caffe only supports
3 BLAS backends: atlas, openblas and Intel MKL. Besides, openblas surpasses
basic BLAS by a large margin on performance, we should not switch
build-dep from openblas/atlas back to basic blas for a computational
intensive application, which would be awkward.
There is a discussion thread about which BLAS to use for the debian package.
Some caffe user believes that, it doesn't matter what BLAS the
maintainer choose, any high-performance one will do.
So I chose openblas after an investigation.
see
https://github.com/BVLC/caffe/issues/2601
> * For the future, seems like additional caffe-tools and octave-caffe
> binary packages could be created from the content generated by the
> matlab/ and tools/ leaves of the source tree. I guess that's what is
> acknowledged in debian/TODO.
ELFs under tools/ are shipped in package "caffe-{cpu,cuda}". I prefer this
scheme rather than "caffe-cpu(metapackage) + caffe-tools(elfs)".
BTW I'd like to create a metapackage "caffe-cpu-all".
For octave-caffe, I haven't find the way to pass the build with octave
yet. I'd like to postpone this binary package to future uploads.
OpenCL support from caffe upstream is still *experimental*, but
it will come one day, hence it was added to TODO. caffe-doc
and octave-caffe are as said above.
> [1] https://wiki.debian.org/UpstreamMetadata
>
> Ghis
Thank you Ghislain Vaillant, the Caffe package quality
will be significantly improved at next time mentors
upload! Thank you Gianfranco Costamagna for long-time
attention!
Reply to: