[Pkg-ofed-devel] Infiniband Kernel Module Initializer and port config tool

[Ana Guerrero Lopez]

Hi Talat,

Sorry for taking so much to reply but I wanted to take the time to reply
thoroughly as there were plenty of things to point to. Also, before I
continue, thank you for packaging this for Debian, something like this
was definitively missing.

On Sun, Jan 17, 2016 at 01:26:07PM +0200, Talat mellanox wrote:
> Hi Ana and OFED and Debian Developement and Discussion team,
> 
> 
> Till now, in Ubuntu and Debian Operation systems, rdma and Infiniband modules
> loaded manually by the user, compared with other operation systems there is
> a service that control them (load, unload, status, autoload ..  ).
> 
> This services is required and needed by Infiniband and RoCE users, in order to
> load the Infiniband kernel modules/ configure ports in VPI mode, the service
> already integrated in other operation system named rdma.
> 
> We prepare the package that contain this tools in Debian format – attached .
> 
> What do you think about adding this package to debian under the umbrella of
> “OFED and Debian Development and Discussion team”

> Could you please review and share it with the relevant reviewer ?

So here goes a first review.

I'm making my comments in two groups, one related to Debian processes
about things that should be done independently of the package and another
about the packaging itself (as in the files in debian/).

# Processes

* Code source: the source should be published somewhere. E.g. github
or some Mellanox's *public* git repository. The URL later goes in the
"Homepage" field in debian/control and in debian/copyright as well.
I know this is packaged in RedHat already but in Debian we must specify
clearly the origin.
Some files have Copyright lines from RedHat, are they the original upstream?
I see you distributed the same content that rdma-7.2_4.1_rc6-1.el7.src.rpm
plus a new file rdma-initramfs-tools-hook that has no authorship or licensing.
Is this file from Mellanox?

* License: this point is related with the point above. There is no doubt
this is meant to be licensed under the GPL but it's not properly documented.
It's missing a file with the full license. Only the files rdma.ifdown-ib and
rdma.ifup-ib say they're under the GPL v2 only, and the spec says GPLv2+,
this is usually interpreted as GPL version 2 or any later version, but it 
would be nice to have something more explicit.

* Git: please, use pkg-ofed's git for hosting the packaging. This is
independent to where you publish your source code. Notice we use some branches
specially for integration with git-buildpackage.

The repositories are at:
http://anonscm.debian.org/gitweb/?a=project_list;pf=pkg-ofed
Alioth's git help:
https://wiki.debian.org/Alioth/Git

I can create a empty git repository for you if needed.

* Before packaging something for Debian, you should send an ITP. More
information at:
 https://wiki.debian.org/ITP
 https://www.debian.org/devel/wnpp/

* Package name: Finally something that maybe is a bit nitpicking. I'm not
sure that "rdma" is a good name for your package. I'm aware this is the RedHat
name and this is certainly something subjective, but it makes more sense to me
something like rdma-scripts or rdma-modules-scripts. I would interpret 'rdma'
like a meta-package that installs everything related to rdma, which isn't the 
case. I would welcome more opinions on this from other lurkers in the list :)


# Packaging

* debian/changelog:
It closes bug #676403 ( https://bugs.debian.org/676403 ) that's unrelated
to this package or OFED. It should close the bug of the ITP mentioned earlier.

* debian/compat:
It says 8, these days we're in debhelper compatibility level 9.

* debian/control:
To work, your package needs a set of packages installed. Maybe some of them
should be in Depends, Recommends or Suggests. Please take a look at this.
Basically, you should make sure your package shouldn't fail if no other ofed 
packages are installed.

The long description is too short, it should be quite more longer and
explain clearly what this package does.

* debian/copyright:
The source is missing, it can't point to a RedHat package, it must be
at the very least a git repository.

The copyright is incomplete, e.g. this is missing:
rdma.ifdown-ib:# Copyright (c) 1996-2013 Red Hat, Inc. all rights reserved.
rdma.ifup-ib:# Copyright (c) 1996-2013 Red Hat, Inc. all rights reserved

All the boilerplate from the template (those line starting with #) should be
removed.

Finally, see the licensing issue I mentioned above.

* debian/postinst  debian/postrm  debian/prerm

You won't need those files if you install the systemd unit using the dh
systemd helper.

And if you were to use those files, they're missing #DEBHELPER#
in the end of the file

* debian/rules

You define pversion but you don't use it later.

Have you tried installing the files using a makefile or a debian/install
file instead of doing it by hand in the debian/rules?

For installing the systemd unit there is also a special dh helper,
don't do it by hand.
Read: https://wiki.debian.org/Teams/pkg-systemd/Packaging

Why are you overriding the conffiles installation?
Files in /etc must be marked conffiles if they are included in a
package. Otherwise they should be created by maintainer scripts.
Refer to Debian Policy Manual section 10.7 (Configuration files) for details.

* debian/patches

It would be good if the patches have the authorship and a description of
why they're needed.

* files installed

Very important, your package shouldn't modify files installed by another
package in any way, it's forbidden by the Debian policy.

The package ships a udev rule and installs it under /etc/udev/rules.d,
which is reserved for user-installed files. The correct directory for
system rules is /lib/udev/rules.d.

The *.sh files the package installs shouldn't include the extension ".sh"

The scripts you're installing are also missing a manpage.

Finally, a read of the maintainer's guide:
https://www.debian.org/doc/manuals/maint-guide/
is strongly encouraged. It would help to understand better the mentioned
issues and have pointers to further documentation when needed.

Please, don't hurry up, and take time to go through this list before pushing a
new version of the package!

I didn't build your package or test it, so there will be probably more
issues.

What do you mean with the relevant reviever?


>     - The package is based on rdma package from other Operation system.

Yes, I have seen it's from RedHat.

>     - Currently only systemd is supported.

That's ok

>     - Do we need to support upstart as well ?

No.

>     - How do we publish the code git tree somewhere ?

You can use github. Your company seems to be there https://github.com/Mellanox ?
There is also http://git.openfabrics.org/
And for the packaging, debian's git.

>     - [very important AR] The conf file “/etc/modprobe.d/mlx4.conf”
> (that comes with
>      “kmod” package) is not needed anymore, and must be removed.
>       This conf file conflicts with “/lib/modprobe.d/libmlx4.conf”
> that will come with
>       “rdma” package, and prevents running a script (/sbin/mlx4-setup.sh) that
>        configures ConnectX-3 devices port types (IB vs. ETH).
> 

Which kmod package? Debian's kmod doesn't include a file /etc/modprobe.d/mlx4.conf
If your package conflicts with another package, it should have a Conflicts:
line.

I hope all this helps.
Ana