[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Bug#823140: RFS: caffe/1.0.0~rc3-1 -- a deep learning framework [ITP]



Hi mentors,

I've fixed the issues you pointed out. New packages
are rebuilt locally, and uploaded to mentors.
https://mentors.debian.net/package/caffe

Debomatic-amd64 is still building this updated package:
http://debomatic-amd64.debian.net/distribution#experimental/caffe/1.0.0~rc3-1/buildlog

Following is my comments to the remarks

On Mon, 2016-05-02 at 14:13 +0100, Ghislain Vaillant wrote:
> - d/changelog: should only contain the entry closing the initial release
> bug.

Removed the line not necessary.

> - d/control: Build-Depends on libboost-all-dev. Do you really need to
> pull the complete Boost stack for the build? Quick look I have had:
> 
> "find_package(Boost 1.46 REQUIRED COMPONENTS system thread filesystem)"
> 
> So you should only require libboost-{filesystem,system,thread}-dev.

Depends on libboost-{filesystem,system,thread,python}-dev, instead
of libboost-all-dev.

> - d/control: Build-Depends on nvidia-cuda-toolkit which automatically
> pulls nvidia-cuda-dev, so no need to specify nvidia-cuda-dev.

Removed nvidia-cuda-dev as build-deps.

> - d/*.install.in: no multi-arch install paths? why?

I have no plan to make multiarch support for this package,
because that makes no sense. In production environment
Caffe is a computational intensive and memory-consuming
application, and I believe no user will need multi-arch
support for this package.

Made no change related to multi-arch.

> - d/libcaffe-dev.install.in: what is the purpose of the additional
> libproto.a binary?

There is a protobuf definition file under Caffe's source,
  src/caffe/proto/caffe.proto
which is a very essential file as it defined
caffe's "protocol". And libproto.a is the compiled
binary version of that protocol.

Besides, the Official "install" target installs
that static lib, so it is recommended by upstream
to have such a lib. Hence it goes into the -dev packages.

> - d/rules + d/*.in: IMO, sounds like a very convoluted way of running 2
> separate builds (one for CPU one for CUDA) and moving the files in the
> right places. Another possibility could have been to have caffe-cpu and
> caffe-cuda as separate source packages, one in main and one in contrib.
> For each of them, the packaging would have been much more simple to
> maintain I suppose.

I've ever considered split them up into 2 source packages,
but building 2 set of binary from 1 source is my initial intent.
I may split them in the future.

> - lintian (from d-o-m):
> 
> I: caffe source: quilt-patch-missing-description fix-spelling-error
> 
> Might want to fix this if not done already. Also please mention whether
> the patch has been forwarded upstream.

Added header to this patch. 

> I: libcaffe-cpu-dev: unstripped-static-library 
> usr/lib/caffe/libproto.a(caffe.pb.cc.o)
> 
> Again not sure what this libproto.a is doing here.

As explained above, this file may be not useless.

> I: libcaffe-cpu1: hardening-no-fortify-functions 
> usr/lib/libcaffe.so.1.0.0-rc3
> 
> Are you using hardening in d/rules? If so, then the injected flags
> might be shadowed by the build system. Consider fixing this.

I've tried to inject these flags according to wiki:Hardening
```
# Hardening Caffe according to https://wiki.debian.org/Hardening
DPKG_EXPORT_BUILDFLAGS=1
export DEB_BUILD_MAINT_OPTIONS = hardening=+all
export DEB_CFLAGS_MAINT_APPEND  = -Wall -pedantic
export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed
include /usr/share/dpkg/buildflags.mk
CFLAGS+=$(CPPFLAGS)
CXXFLAGS+=$(CPPFLAGS)
```

But hardening-fortify-source-function is still unfixed.
```
$ hardening-check /usr/bin/caffe
/usr/bin/caffe:
 Position Independent Executable: yes
 Stack protected: yes
 Fortify Source functions: no, only unprotected functions found!
 Read-only relocations: yes
 Immediate binding: yes
```

I have no idea about it...

Thank you for reviewing!

> Good luck,
> Ghis



Reply to: