Comments inline.

On Tuesday 21 October 2014 06:28 PM, Maximiliano Curia wrote:
¡Hola Rahul!

The review process involves checking and fixing the packaging, and checking
upstream code for possible errors/incompatibilities with the way things are
done in the distribution. It takes time from both of us.
Totally understand and appreciate this. I didn't think that a package in Ubuntu mainstream would need so much review.

My ultra motive to offer you to review the package is to have more members
engaged in the team, not to push things that are not up to the quality expected
in Debian.
Agreed. But it would be great if we can have this in Debian Jessie. Is it still possible?
There are a couple of fixes in the upstream git, last commit is 2014-05-08,
you might want to include those.
Done.
To be under the kde team umbrella the package should be something like:
Maintainer: Debian/Kubuntu Qt/KDE Maintainers <debian-qt-...@lists.debian.org>
or:
Maintainer: Debian KDE Extras Team <pkg-kde-ext...@lists.alioth.debian.org>
or:
Maintainer: Debian Krap Maintainers <debian-qt-...@lists.debian.org>

The field: XSBC-Original-Maintainer is not considered valid in Debian
packages.

Add add yourself to the Uploaders list.
Done.

In the debian/copyright file:
Source: <url://example.com>
Please update the template to point to the upstream git repository.
Done.

Also in the debian/copyright file, the debian/* path is licensed under a more
restrictive license than the upstream code (GPL, and LGPL respectively), this
kind of licensing could block patches in the debian package from ever be
applied upstream and should be avoided. I pinged Rohan about this.
Ok.

In Debian the pam modules are named libpam-$module, please rename the binary
package.
Done.

The description provides almost no information, please extend it. Consider
using the kwalletmanager description, and adding a paragraph about the pam
module (ala libpam-gnome-keyring).
Done. I also added a README file describing the prerequisites and necessary configuration.

It's a good idea to set the build dependencies versions to (at least) the ones
listed in the CMakeLists.txt, in this case cmake (>= 2.8.8) and
libgcrypt11-dev (>= 1.5.0).
Done.

In the code I don't see any obvious errors, but I'm not an expert in pam
modules, some comments though:
In kwallet_hash, after the call to error = gcry_kdf_derive(..) it's not
checking in error returned something.

In prompt_for_password, the memset in the lines:
     struct pam_response *response = NULL;
     memset (&response, 0, sizeof(response));
is redundant.
I have not reviewed the upstream code (not sure if I'll be able to understand it also). Also, I prefer to leave upstream code unchanged unless it breaks something or has some security or performance issues.
Also, the normal review process is done via mentors.debian.net, where you
could upload the package and send a RFS, I prefer using a git repository where
I can see the changes made, and afterwards integrate the changes in a repository
for the package, either one is fine, or even an uri where I can fetch the
package source (I don't care about the binary file).
You can get the source at *https://github.com/amaramrahul/pam-kwallet*

In any case, I would prefer not to have the packages as attachments, specially
in bugs and the team mailing lists, so, unless you can't publish the files
somewhere else, please avoid sending them like so. And if you really have to
send the files as attachments, please send them via direct mail, without
copies.
Point noted.

Thanks,

Looking forward to your response.

Thanks,
Rahul.
-- 
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-kde-talk

Reply via email to