The SRPM URL for ccid should have been: SRPM URL:
http://directory.fedora.redhat.com/built/rpm_review/rrelyea/ccid-1.0.1-1.src.rpm

Since it is already in Extras, what is the purpose of bringing it into Core?

They are needed for the smart card login support for RHEL5.

It being needed for RHEL has nothing to do with whether it should be in Fedora
Core or not, though.... Is there a plan to add smart card login support to
Fedora also, or just to RHEL?

Yes. You will see smart card login support in fedora before you see it in RHEL.

This still doesn't mean it has to be in Core.  Since it is already in extras,
lets skip to the step of getting approval for core by the technical lead.

Bill?

By the way, if you're interested in maintaining these packages in Extras, that
would be fine with me too.  Some comments/notes anyway:

> SRPM URL: [...] pcsc-lite-1.3.1-1.src.rpm

Ick, this would not upgrade the version currently in Extras.  Please bump the
release tag.

> This package is moving from Extras to Core. Changes from the extra version 
> include:

These should be mentioned in the package's %changelog, otherwise it'll look like
I'm responsible of the changes (some of which I consider regressions):

> moved libpcsclite.so from devel to libs.

Why?

> make %config %config(noreplace) to make rpmlint happy (oh and it's the right 
> thing to do;).

Partially disagreed.  The FE package has /etc/reader.conf marked as %ghost
%config.  Actually the right thing to do would be to remove the %config
altogether; this file is not something that anyone should go and modify, it's
deleted and recreated from the contents of /etc/reader.conf.d/*.conf on every
pcscd restart.  One could also argue that a better place for the generated file
would be somewhere in /var instead of /etc/reader.conf.

Additionally, I see the new package marks /etc/reader.conf.d/README a
%config(noreplace) file, which doesn't make sense to me.

One other thing: the build dependency on graphviz was removed, but the HAVE_DOT
= yes modification for doxygen's config is still in, that should probably be
removed.

> Generic USB CCID (Chip/Smart Card Interface Devices) driver.
> This package is also moving from extras to core. The only change to this file 
> was removing the %(_dist) macro.

Same problem here wrt. upgrading from the current FE version, please bump the
release tag.

(In reply to comment #6)
> This still doesn't mean it has to be in Core.  Since it is already in extras,
> lets skip to the step of getting approval for core by the technical lead.

Well, if you want smart card login support, I suspect that means some portion of
pam/gdm/etc is going to grow a direct dependency. If that's the case, it's fine
with me. Login support generally seems like a Core feature

Curious why .so moved from devel->lib.

Why regenerate the config file on startup? Aside from requiring write access
(/var/run/pcsc?), it seems wasteful. Can't it just read /etc/reader.conf.d/
directly?

Reading /etc/reader.conf.d/*.conf directly would be better, yes.  The current
implementation was submitted to me in PM (see Jul 31 2004 %changelog entry) and
later sent to and adopted by upstream almost as-is.  I bet upstream wouldn't
mind a patch that makes pcscd take care of things itself.

I'll update the release number.

The 'noreplace' was just blindly following the Fedora package standards. I have
no problem reverting it. Obviously the README is not part of the actual config.
The whole /etc/reader.conf.d is really only used for serial readers (and other
non-hot pluggable readers), which are getting more an more rare. I have no
problem reverting that.

Is /usr/sbin/update-reader.conf really run every time you start pcscd, or just
when an appropriate reader package installs itself?

The move of libpcsclite.so is for some pkcs #11 modules which dlopen libpcsclite.so.

One other question Ville, why does pcsc-lite depend on ccid. pcsc-lite isn't
useful without some reader, but it does start without one, and one can envision
systems which have some other reader. It's not a big deal since most modern
readers are ccid readers, so ccid support should be part of the base anyway. I
didn't want to change that at this point without knowing the reason for it.

So, they're dlopening the unversioned .so symlink, and hoping it's the same ABI
they're built against? That seems fragile.

Yes, /usr/sbin/update-reader.conf is being run on every pcscd start, see start()
in /etc/init.d/pcscd.

pcsc-lite does not depend on ccid specifically.  It depends on pcsc-ifd-handler,
which is a "virtual" dependency, provided by packages that contain suitable
drivers.  In addition to ccid, in Extras it is provided by pcsc-lite-openct and
(should be provided) by ctapi-cyberjack-pcsc, as well as number of other, mainly
proprietary, driver packages elsewhere.

A conf file in /usr/sbin just seems a bit um... not cool.  Is there any way to
put this somewhere else?  /etc/sysconfig/ ? /etc/ ?  /usr/share/ ?  Somewhere
that isn't /usr/sbin...

/usr/sbin/update-reader.conf is not a config file, it's a shell script which
creates /etc/reader.conf from  the contents of /etc/reader.conf.d/*.conf.  Yes,
I did nag to upstream about the potentially confusing name of the script...

I've uploaded new packages:

Spec URL: http://directory.fedora.redhat.com/built/rpm_review/rrelyea/pcsc-lite.spec
SRPM URL:
http://directory.fedora.redhat.com/built/rpm_review/rrelyea/pcsc-lite-1.3.1-2.src.rpm

Spec URL: http://directory.fedora.redhat.com/built/rpm_review/rrelyea/ccid.spec
SRPM URL:
http://directory.fedora.redhat.com/built/rpm_review/rrelyea/ccid-1.0.1-2.src.rpm

Changes include:
- reverting the changes to the reader.conf.
- reverting the shared library change. pcsc-lite is supposed to be a pretty
stable API, but I'll leave the versioning in until I can verify with upstream
whether or not binary API continuity is part of the package requirements.
- incremented the version number with the appropriate comment.
- removed the sed command which turned on HAVE_DOT in the doxygen config file.

I guess I should have said, please review these updates?
There are several packages that depend on these that are waiting in the pipeline.

bob

pcsc-lite compared to the latest FE package:
- libpcsclite.pc has moved from -devel to -libs.  That looks like a bug to me in
  the first place, and especially so because -devel still has a dependency on
  pkgconfig, and -libs not.
- %{_sysconfdir}/reader.conf.d/README is %ghost %config; I think both of those
  should be dropped and the file included without any special %foo's.  Or maybe
  make it %doc.
- %{_sysconfdir}/reader.conf is no longer %ghost; I mentioned removing %config
  in comment 7, but was %ghost removed for some specific reason too?

ccid compared to the latest FE package:
- Careful, macros are expanded in %changelog too.  Escape it like %%{?dist}
  (with two '%'s) or just say "disttag".

Thanks for the quick review Ville.
I've now updated the packages (and removed some bottlenecks so I can turn them
around faster).

here's the diff between the CORE proposed packages and the current EXTRAS to aid
the review the only question I have is the %doc for reader.conf.d/README
correct, or does the %doc do some magic I'm not aware of:

--- /usr/src/redhat/SPECS/pcsc-lite.spec        2006-04-22 13:32:58.000000000 -0700
+++ ./pcsc-lite.spec    2006-06-08 09:02:14.253045000 -0700
@@ -1,6 +1,6 @@
 Name:           pcsc-lite
 Version:        1.3.1
-Release:        1%{?dist}
+Release:        2
 Summary:        PC/SC Lite smart card framework and applications

 Group:          System Environment/Daemons
@@ -13,7 +13,6 @@

 BuildRequires:  libusb-devel >= 0.1.7
 BuildRequires:  doxygen
-BuildRequires:  graphviz
 Requires(post): /sbin/chkconfig
 Requires(preun): /sbin/chkconfig
 Requires:       pcsc-ifd-handler
@@ -57,8 +56,6 @@
 %setup -q
 %patch0 -p0
 %patch1 -p1
-sed -i -e 's/^\(HAVE_DOT\s*=\s*\)/\1 YES/' doc/doxygen.conf.in
-

 %build
 %configure \
@@ -114,7 +111,7 @@
 %defattr(-,root,root,-)
 %doc AUTHORS ChangeLog* COPYING DRIVERS HELP README SECURITY TODO
 %dir %{_sysconfdir}/reader.conf.d/
-%{_sysconfdir}/reader.conf.d/README
+%doc %{_sysconfdir}/reader.conf.d/README
 %ghost %config %{_sysconfdir}/reader.conf
 %{_initrddir}/pcscd
 %{_bindir}/formaticc
@@ -142,6 +139,11 @@


 %changelog
+* Mon Jun 5 2006 Bob Relyea <rrelyea> - 1.3.1-2
+- Move to Fedora Core.
+- Remove dependency on graphviz.
+- Removed %%{_dist}
+
 * Sat Apr 22 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.3.1-1
 - 1.3.1.

3c3
< Release:        2
---
> Release:        1%{?dist}
15a16
> BuildRequires:  graphviz
58a60,61
> sed -i -e 's/^\(HAVE_DOT\s*=\s*\)/\1 YES/' doc/doxygen.conf.in
>
114c117
< %doc %{_sysconfdir}/reader.conf.d/README
---
> %{_sysconfdir}/reader.conf.d/README
142,146d144
< * Mon Jun 5 2006 Bob Relyea <rrelyea> - 1.3.1-2
< - Move to Fedora Core.
< - Remove dependency on graphviz.
< - Removed %%{_dist}
<


--------------------------------
--- /usr/src/redhat/SPECS/ccid.spec     2006-04-22 13:33:57.000000000 -0700
+++ ./ccid.spec 2006-06-08 09:06:19.497820000 -0700
@@ -2,7 +2,7 @@

 Name:           ccid
 Version:        1.0.1
-Release:        1%{?dist}
+Release:        2
 Summary:        Generic USB CCID smart card reader driver

 Group:          System Environment/Libraries
@@ -61,6 +61,9 @@


 %changelog
+* Mon Jun  5 2006 Bob Relyea <rrelyea> - 1.0.1-2
+- Move to Fedora Core, removed %%{_dist}.
+
 * Sat Apr 22 2006 Ville Skyttä <ville.skytta at iki.fi> - 1.0.1-1
 - 1.0.1.

Lost review comments:
https://www.redhat.com/archives/fedora-package-review/2006-June/thread.html

In particular:
https://www.redhat.com/archives/fedora-package-review/2006-June/msg01096.html

Here's the full view

Smart Card Login Packages:

Base:
pam_pkcs11      (default)
coolkey         (default) depends on pcsc-lite-libs ifd-egate
ccid            (default) depends on pcsc-lite
ifd-egate       (default) depends on pcsc-lite
pcsc-lite-libs  (default)
pcsc-lite       (default)

Developement Libraries
pcsc-lite-devel depends on pcsc-lite-libs
coolkey-devel   depends on coolkey

Added to dist-fc6, waiting for build into rawhide.

Built into rawhide.