Bug 661810 - Review Request: libcacard - Common Access Card emulation library
Summary: Review Request: libcacard - Common Access Card emulation library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-12-09 17:24 UTC by Alon Levy
Modified: 2014-08-04 22:08 UTC (History)
4 users (show)

Fixed In Version: libcacard-0.1.0-4.fc14
Clone Of:
Environment:
Last Closed: 2010-12-30 20:21:52 UTC
Type: ---
Embargoed:
hdegoede: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Alon Levy 2010-12-09 17:24:58 UTC
Spec URL: http://people.freedesktop.org/~alon/libcacard.spec
SRPM URL: http://people.freedesktop.org/~alon/libcacard-0.1.0-2.fc14.src.rpm
Description:
This library emulates CAC, or Common Access Cards, a specification used for java smart cards used for authentication, carrying personal information, encryption of and signature of documents. The library relies on NSS for it's backend, and provides an API for clients to instantiate cards and applets on the cards, and provide certificates for the CAC applet. It was written for usage by SPICE and Qemu to let a vm access a remote or host physical or emulated card.

Comment 1 Hans de Goede 2010-12-09 18:54:35 UTC
I'll review this and I'll sponsor Alon. Alon note you forgot to set the FE-NEEDSPONSOR blocker bug on this bug. No need to do that now as I'll sponsor you.

Comment 2 Hans de Goede 2010-12-09 19:01:58 UTC
Some initial comments / things to fix (which make doing a full review a bit hard):

You've the following defattr line in the spec file (multiple times):
%defattr(-,root,root.-)
That should be:
%defattr(-,root,root,-)

The:
%doc COPYING README

Line should be part of the %files for the main package, iow make the main
%files look like this:

%files
%defattr(-,root,root,-)
%doc COPYING README
%{_libdir}/libcacard.so.*

And remove the %doc line further below

In %files devel I see:
%{_libdir}/libcacard.*a

In Fedora we do not ship .la nor .a files, please remove those (see
/etc/rpmdevtools/spectemplate-minimal.spec )

Comment 3 Alon Levy 2010-12-12 09:48:36 UTC
Addressed all the issues, fixed package is revision 3:

Spec URL: http://people.freedesktop.org/~alon/libcacard.spec
SRPM URL: http://people.freedesktop.org/~alon/libcacard-0.1.0-3.fc14.src.rpm

Comment 4 Hans de Goede 2010-12-12 10:08:49 UTC
Thanks, full review below:

Good:
----
- rpmlint says:
libcacard.src: W: no-cleaning-of-buildroot %clean
libcacard.src: W: no-buildroot-tag
libcacard.src: W: no-%clean-section
libcacard-devel.x86_64: W: no-documentation
libcacard-tools.x86_64: W: no-documentation
libcacard-tools.x86_64: W: no-manual-page-for-binary vscclient
These can all be ignored
- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2+) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files
- post/postun ldconfig ok

Needs work:
-----------
- rpmlint says:
libcacard.src:59: W: macro-in-%changelog %doc
libcacard.src:59: W: macro-in-%changelog %files
libcacard-devel.x86_64: W: no-dependency-on libcacard/libcacard-libs/liblibcacard
These are bad, see below
- devel should require base package n-v-r, iow it needs a
Requires:       %{name} = %{version}-%{release}
line below its Summary. You should add the same to the the -tools subpackage
to force the -tools and main package version to stay in sync
- macros in changelog, when you refer to a macro like %doc in the changelog
use %%doc instead
- Your rpm Group tag is wrong and although mostly unused in Fedora, I
believe it should still be right. For the main package it should be:
Group:          System Environment/Libraries
and then for -devel
Group:          Development/Libraries

Notes:
------
- The source files in the tarbal miss a LGPLv2+ copyright / license header,
  please add those in the next upstream release
- A hint for your next package, it is a good idea to start out with one of the
  templates under /etc/rpmdevtools

Comment 5 Alon Levy 2010-12-12 13:06:37 UTC
Addressed all the issues, except the upstream LGPLv2+ copyright, will do that for next upstream release.

fixed package is revision 4:

Spec URL: http://people.freedesktop.org/~alon/libcacard.spec
SRPM URL: http://people.freedesktop.org/~alon/libcacard-0.1.0-4.fc14.src.rpm

Comment 6 Hans de Goede 2010-12-12 14:09:40 UTC
Looks good now, approved!

I'll go and add you to the Packagers group in FAS and sponsor you.

Comment 7 Alon Levy 2010-12-14 15:07:44 UTC
New Package SCM Request
=======================
Package Name: libcacard
Short Description: Common Access Card (CAC) Emulation
Owners: alon
Branches: f14
InitialCC: jwrdegoede

Comment 8 Jason Tibbitts 2010-12-14 15:30:53 UTC
Git done (by process-git-requests).

Comment 9 Fedora Update System 2010-12-15 10:45:13 UTC
libcacard-0.1.0-4.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/libcacard-0.1.0-4.fc14

Comment 10 Fedora Update System 2010-12-17 08:24:49 UTC
libcacard-0.1.0-4.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update libcacard'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/libcacard-0.1.0-4.fc14

Comment 11 Fedora Update System 2010-12-30 20:21:47 UTC
libcacard-0.1.0-4.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.