Reg. packaging pam-kwallet for Debian

[Rahul Amaram]

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-kde at lists.debian.org>
> or:
> Maintainer: Debian KDE Extras Team <pkg-kde-extras at lists.alioth.debian.org>
> or:
> Maintainer: Debian Krap Maintainers <debian-qt-kde at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pkg-kde-talk/attachments/20141023/1d5bbe08/attachment.html>