[Bug 469955] New: Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

Summary: Review Request: fprintd - D-Bus service for Fingerprint reader access

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Summary: Review Request: fprintd - D-Bus service for
                    Fingerprint reader access
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail], [e-mail]
   Estimated Hours: 0.0
    Classification: Fedora

Not that this requires libfprint >= 0.1, builds available at:
http://koji.fedoraproject.org/koji/taskinfo?taskID=917627

Spec URL: http://hadess.fedorapeople.org/fprintd/fprintd.spec
SRPM URL: http://hadess.fedorapeople.org/fprintd/fprintd-0.1-1.fc10.src.rpm
Description: D-Bus service to access fingerprint readers.

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Till Maas  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Till Maas   2008-11-06 08:38:02 EDT ---
Some issues I noticed at my first check:

- Why do you need a Conflict with pam_fprint, are Obsolutes/Provides not better
fitted here?
https://fedoraproject.org/wiki/Packaging/Conflicts

- Source0 is not a URL and it is not explained how to create the source
https://fedoraproject.org/wiki/Packaging/SourceURL

- Is it intentional that the config files in /etc are not marked as %config?

Btw. the release string of the linked libfprint seems to be wrong, it should be
0.1.pre.fc10 instead of 1.pre.fc10

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

--- Comment #2 from Bastien Nocera   2008-11-18 14:03:33 EDT ---
(In reply to comment #1)
> Some issues I noticed at my first check:
> Why do you need a Conflict with pam_fprint, are Obsolutes/Provides not better
>
> fitted here?
> https://fedoraproject.org/wiki/Packaging/Conflicts

It's not stable/tested enough to replace pam_fprint. So I'm currently only
conflicting with it.
> Source0 is not a URL and it is not explained how to create the source
>
> https://fedoraproject.org/wiki/Packaging/SourceURL

True, will fix.
> Is it intentional that the config files in /etc are not marked as %config?

Yes and no. %{_sysconfdir}/fprintd.conf should be marked as %config, but it
doesn't actually do anything useful yet, so it's better to just have it
replaced for now. I should add a comment about it.
> Btw. the release string of the linked libfprint seems to be wrong, it should be
> 0.1.pre.fc10 instead of 1.pre.fc10

That should be fixed, I updated libfprint in F11:
http://koji.fedoraproject.org/koji/taskinfo?taskID=938201

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

--- Comment #3 from Bastien Nocera   2008-11-23 09:32:44 EDT ---
All fixed.

http://hadess.fedorapeople.org/fprintd/fprintd.spec
http://hadess.fedorapeople.org/fprintd/fprintd-0.1-2.gitaf42ec70f3.fc10.src....

Koji scratch build worked:
http://koji.fedoraproject.org/koji/taskinfo?taskID=946393

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Matthias Clasen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #4 from Matthias Clasen   2008-11-25 11:15:53 EDT ---
Here is what rpmlint says on the rpms:

fprintd.i386: W: non-conffile-in-etc /etc/fprintd.conf

You said you wanted to make this conf ?

fprintd.i386: W: non-conffile-in-etc
/etc/dbus-1/system.d/net.reactivated.Fprint.conf

This is ignorable

fprintd-devel.i386: W: summary-not-capitalized fprintd development
documentation

I always use a more-or-less standardized summary of "Development files for
%{name}" for -devel packages.

fprintd-devel.i386: W: invalid-license GFDLv1.1+

No + there, I think.

printd-pam.i386: W: no-documentation

Thats sad, but ignorable.

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

--- Comment #5 from Matthias Clasen   2008-11-25 11:29:47 EDT ---
Taking a first look at the spec file:

Requires:       %{name} = %{version}

Not sure if theres a policy about this, but I always do
Requires: %{name} = %{version}-%{release}
to avoid surprises

Going to do a formal review in a bit.

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

--- Comment #6 from Bastien Nocera   2008-11-25 11:36:42 EDT ---
(In reply to comment #4)
> Here is what rpmlint says on the rpms:
>
> fprintd.i386: W: non-conffile-in-etc /etc/fprintd.conf
>
> You said you wanted to make this conf ?

No, I added a comment about it:
# This file should be marked as config when it does something useful
> fprintd.i386: W: non-conffile-in-etc
> /etc/dbus-1/system.d/net.reactivated.Fprint.conf
>
> This is ignorable
>
> fprintd-devel.i386: W: summary-not-capitalized fprintd development
> documentation
>
> I always use a more-or-less standardized summary of "Development files for
> %{name}" for -devel packages.

OK, will change.
> fprintd-devel.i386: W: invalid-license GFDLv1.1+
>
> No + there, I think.

But the docs say:
"Permission is granted to copy, distribute and/or modify this document under
the terms of the GNU Free Documentation License, Version 1.1 or any later
version  [...]"
> printd-pam.i386: W: no-documentation
>
> Thats sad, but ignorable.

I'll write a little something to go upstream and in the package.

(In reply to comment #5)
> Taking a first look at the spec file:
>
> Requires:       %{name} = %{version}
>
> Not sure if theres a policy about this, but I always do
> Requires: %{name} = %{version}-%{release}
> to avoid surprises

Yes, will fix.

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Matthias Clasen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|[e-mail]    |[e-mail]

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

--- Comment #7 from Matthias Clasen   2008-11-26 01:46:25 EDT ---
two more informal comments on the spec before I run down the checklist:
- I don't think the explicit Requires: PolicyKit is needed, library deps 
  should take care of that
- The Conflicts: pam_fprint should probably have a comment explaining your
  rationale from comment #2

rpmlint run: see above

package name: ok
spec file name: ok
packaging guidelines: ok
license: ok
license field: not ok
  the license field says GPLv2+, and the source files say so too,
  but the license file is GPL3. What gives ?
license file: ok
spec file language: ok
spec file legible: ok
upstream sources: ok
buildable: ok
ExcludeArch: ok
BuildRequires: ok
locale handling: ok
shared libs: ok
relocatable: ok
directory ownership: not ok
  -devel should require gtk-doc, for /usr/share/gtk-doc/html
  -pam should require pam, for /lib/security
duplicate files: ok
permissions: not ok
  the %files sections for pam and devel need %defattr
%clean: ok
macro use: ok
permissible content: ok
large docs: ok
%doc content: ok
headers: ok
static libs: ok
pc files: ok
shared libs: ok
devel deps: ok
libtool archives: ok
gui apps: ok
file ownership: ok
%install: ok
utf8 filenames: ok

summary: some small fixes left

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Bastien Nocera  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |474573

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Bastien Nocera  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|474573                      |

--- Comment #8 from Bastien Nocera   2008-12-04 10:09:49 EDT ---
Fixed Require: %{name} = %{version}-%{release}
Changed summary for devel files
Added README for pam

(In reply to comment #7)
> two more informal comments on the spec before I run down the checklist:
> I don't think the explicit Requires: PolicyKit is needed, library deps 
> should take care of that

Removed
> The Conflicts: pam_fprint should probably have a comment explaining your
> rationale from comment #2

I removed the Conflicts. The configuration is different, so I'll add some
release notes.
> rpmlint run: see above
>
> package name: ok
> spec file name: ok
> packaging guidelines: ok
> license: ok
> license field: not ok
>   the license field says GPLv2+, and the source files say so too,
>   but the license file is GPL3. What gives ?

Fixed, it's supposed to be a GPLv2 COPYING file.
> license file: ok
> spec file language: ok
> spec file legible: ok
> upstream sources: ok
> buildable: ok
> ExcludeArch: ok
> BuildRequires: ok
> locale handling: ok
> shared libs: ok
> relocatable: ok
> directory ownership: not ok
>   -devel should require gtk-doc, for /usr/share/gtk-doc/html

Added.
> -pam should require pam, for /lib/security

Already dragged in through library deps (PAM libs are in the pam package)
> duplicate files: ok
> permissions: not ok
>   the %files sections for pam and devel need %defattr

Done.
> %clean: ok
> macro use: ok
> permissible content: ok
> large docs: ok
> %doc content: ok
> headers: ok
> static libs: ok
> pc files: ok
> shared libs: ok
> devel deps: ok
> libtool archives: ok
> gui apps: ok
> file ownership: ok
> %install: ok
> utf8 filenames: ok
>
> summary: some small fixes left

All done!

http://hadess.fedorapeople.org/fprintd/fprintd.spec
http://hadess.fedorapeople.org/fprintd/fprintd-0.1-3.git43fe72a2aa.fc10.src....

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=976118

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Bastien Nocera  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |474573

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Matthias Clasen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-review+

--- Comment #9 from Matthias Clasen   2008-12-04 12:18:55 EDT ---
Looks fine now. Approved.

Might be nice to add

/usr/share/dbus/interfaces/net.reactivated.Fprint.xml

at some point

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Bastien Nocera  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

--- Comment #10 from Bastien Nocera   2008-12-04 12:38:27 EDT ---
New Package CVS Request
=======================
Package Name: fprintd
Short Description: D-Bus service for Fingerprint reader access
Owners: hadess
Branches: F-11
InitialCC:

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Kevin Fenzi  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
               Flag|fedora-cvs?                 |fedora-cvs+

--- Comment #11 from Kevin Fenzi   2008-12-05 00:10:42 EDT ---
cvs done.

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Bastien Nocera  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |NEXTRELEASE
               Flag|fedora-cvs+                 |fedora-cvs?

--- Comment #12 from Bastien Nocera   2008-12-05 05:09:45 EDT ---
Build in rawhide, thanks all.

[Bug 469955] Review Request: fprintd - D-Bus service for Fingerprint reader access

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=469955

Kevin Fenzi  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-cvs?                 |fedora-cvs+