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: nobody@fedoraproject.org ReportedBy: bnocera@redhat.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com 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.
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 opensource@till.name changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |opensource@till.name
--- Comment #1 from Till Maas opensource@till.name 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
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 bnocera@redhat.com 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
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
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 bnocera@redhat.com 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
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 mclasen@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mclasen@redhat.com
--- Comment #4 from Matthias Clasen mclasen@redhat.com 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.
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 mclasen@redhat.com 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.
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 bnocera@redhat.com 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.
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 mclasen@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |mclasen@redhat.com
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 mclasen@redhat.com 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
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 bnocera@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |474573
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 bnocera@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|474573 |
--- Comment #8 from Bastien Nocera bnocera@redhat.com 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
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 bnocera@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |474573
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 mclasen@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review+
--- Comment #9 from Matthias Clasen mclasen@redhat.com 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
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 bnocera@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #10 from Bastien Nocera bnocera@redhat.com 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:
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 kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flag|fedora-cvs? |fedora-cvs+
--- Comment #11 from Kevin Fenzi kevin@tummy.com 2008-12-05 00:10:42 EDT --- cvs done.
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 bnocera@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE Flag|fedora-cvs+ |fedora-cvs?
--- Comment #12 from Bastien Nocera bnocera@redhat.com 2008-12-05 05:09:45 EDT --- Build in rawhide, thanks all.
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 kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
package-review@lists.fedoraproject.org