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@xxxxxx 2006-04-30 04:53 EST ------- Well thats a nice start: [hans@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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review