[Bug 188369] Review Request: ctapi-cyberjack

[bugzilla at redhat.com]

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

Summary: Review Request: ctapi-cyberjack


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





------- Additional Comments From j.w.r.degoede at hhs.nl  2006-04-30 04:53 EST -------
Well thats a nice start:
[hans at shalem ~]$ rpmbuild -ba /usr/src/redhat/SPECS/ctapi-cyberjack.spec 
error: parse error in expression
error: /usr/src/redhat/SPECS/ctapi-cyberjack.spec:15: parseExpressionBoolean
returns -1
error: Package has no %description: ctapi-cyberjack

This is caused by the following lines in the spec:
%if %{dist} == "FC4"
        %define readers_dir    %{_libdir}/readers
%else
        %define readers_dir %(pkg-config libpcsclite --variable=usbdropdir)
%endif

I was already planning on adding a "MUST Fix" item for those, but not this soon
in the review. This is not the Fedora Extras way of doing things to avoid %if
filled specfiles we have one specfile per release in our CVS. Reviews should
always happen against the devel release, but in most cases (this one included)
the latest stable is fine too.

So for this review please target FC-5 and FC-5 only then once your package is
approved and imported (into the devel branch where all imports happen) you can
request CVS branches for FC-5 and FC-4 and then change the specfile for FC-4 .

It is ok to add a comment to the specfile submitted for review about nescesarry
changes for older releases. So you could change the above lines to:
# define this to %{_libdir}/readers when building on FC-4
%define readers_dir %(pkg-config libpcsclite --variable=usbdropdir)

Since it looks like I won't be able todo a full review right now after all
because of lack of time and because there is alot to fix in a first glance, here
is a quick and probably incomplete list of "MUST Fix" items:

MUST Fix
========

* Don't use release conditional %if constructs, instead target FC-5 with the
  spec submitted for review (see above).
* Don't use German in the specfile not even in comments
* Remove the unused "%define _realrelease    1"
* Replace "4%{dist}" with "4%{?dist}" watch the ?
* Have you tried replacing "make" with "make %{?_smp_mflags}" ?
  Ifso please add a comment that make breaks with %{?_smp_mflags} if not
  please add %{?_smp_mflags}.
* When linking libctapi-cyberjack.so it doesn't get passed an soname flag, the 
  ld -x --shared -lusb -o libctapi-cyberjack.so ctn_list.o cjctapi_beep.o ...
  command should get passed "-soname libctapi-cyberjack.so.0"
* When installing libctapi-cyberjack.so you install it as
  libctapi-cyberjack.so.%{version} this is not correct you should install it as
  libctapi-cyberjack.so.0 (matching the -soname above).
  When the ABI of the library changes (which it will probably never will) you
  change both the -soname and the install command to libctapi-cyberjack.so.1,
  when the ABI changes again to .2 etc.
* Don't use a .so.%{version} install for the ifd, this is a plugin which always
  gets dlopen-ed and as such should be installed as a plain .so, thus you also
  do not need to add -soname to the link command for the ifd only for the ctapi 
  lib.
* libcyberjack_ifd.so gets staticly linked against libctapi-cyberjack, please 
  make this dynamic (currently the link command includes:
  "../ctapi/libctapi-cyberjack.a" replace this with "-L../ctapi 
  -llibctapi-cyberjack").
* drop the "Requires:       %{name} = %{version}" for PC-SC subpackage,
  currently it is staticly linked and the dyn link which should be there will
  lead to an autodependency generated by rpm.
* Rename the "PC-SC" subpakcage to pcsc to match the way pcsc is written in
  other packages names (pcsc-lite, pcsc-tools, pcsc-perl)
* Don't create the docdir and install the docs there just name the docs
  as they are releative to the builddir afyter %doc, rpmbuild will copy
  them for you and create the docdir itself.

And some personal preferences of mine, not obligatory in anyway:
* Don't put a "," between BuildRequires whitespace alone is enough.
* Don't use %{buildroot} because %{buildroot}%{_datadir} is hard to read,
  instead use $RPM_BUILD_ROOT, $RPM_BUILD_ROOT%{_datadir} is IMHO much easier
  to read.


Phew, long list. Good luck fixing all these, most are trivial to fix though and
don't get scared away by this you picked a hard package to start with and as
package more software your reviews will go more and more smoothly. Please post a
new version addressing all the above MUST Fix items, then I'll take a second
stab at doing a Full review and hopefully approving your first package. Feel
free to ask questions about the above if there is anything you don't understand
or disagree with.


-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.