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

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



On 03/05/16 03:08, lumin wrote:
- 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.

What about cross-building?

- 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.

What upstream does is not always aligned with what Debian recommends
though. I was just wondering how the caffe and proto binaries were
related. Plus, it makes lintian ouput comments about unstripped static
libs.

- 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.

Here is why I would consider splitting them **now**. A significant portion of your d/rules consist in detecting whether CUDA is available
and optionally enable compilation of the CUDA version. This could be
avoided by having a caffe or caffe-cpu source package, which potentially
builds on all arch, and a caffe-cuda source package with the CUDA Build-Depends, for which you would not need to manually specify the target
architectures.

Benefits from this include:
1) The caffe[-cpu] source package and related binaries can live in main,
instead of contrib at the moment.
2) The caffe-cuda source package does not need the manual architecture
restriction stuff.
3) Last but not least, the resulting source packages become much
simpler to maintain in the long term, which is good from a
**team-maintenance perspective**.

As an FYI, the version of CUDA you are asking is available for ppc64el
and no longer for i386. With your solution, you (now) have to modify
this manually in d/{control,rules}. With separate source packages, you would not have to.

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)
```

You should only require
export DEB_BUILD_MAINT_OPTIONS = hardening=+all
export DEB_CFLAGS_MAINT_APPEND  = -Wall -pedantic
export DEB_CPPFLAGS_MAINT_APPEND = -Wall -pedantic
export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed

I have added DEB_CPPFLAGS_MAINT_APPEND, since CMake does not always
automatically transfer the CFLAGS to CPPFLAGS. Unless, the caffe
build system is set-up for doing so.

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...

You would have to look for override lines of the form:

set(CFLAGS ...) which should be replaced by set(CFLAGS $(CFLAGS) ...)

An upstream classic unfortunately.

Ghis


Reply to: