Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: yazpp - C++ API for YAZ
https://bugzilla.redhat.com/show_bug.cgi?id=753597
Summary: Review Request: yazpp - C++ API for YAZ Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: me@ryanlewis.net QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org Classification: Fedora Story Points: --- Type: ---
Spec URL: http://fedorapeople.org/~rhl/yazpp/yazpp.spec SRPM URL: http://fedorapeople.org/~rhl/yazpp/yazpp-1.2.7-2.fc16.src.rpm Description: YAZ++ is an application programming interface (API) to YAZ which supports the development of Z39.50/SRW/SRU client and server applications using C++. Like YAZ, it supports Z39.50-2003 (version 3) as well as SRW/SRU version 1.1 in both the client and server roles. YAZ++ includes an implementation of the ZOOM C++ binding and a generic client/server API based on the Observer/Observable design pattern.
Hey guys, just packaged this up. It works against the latest rawhide version of yaz, and is the latest version of yazpp.
I would appreciate a review.
$ rpmlint -v yazpp.spec yazpp.spec: I: checking-url http://ftp.indexdata.dk/pub/yazpp/yazpp-1.2.7.tar.gz (timeout 10 seconds) 0 packages and 1 specfiles checked; 0 errors, 0 warnings.
$ rpmlint -v yazpp-1.2.7-2.fc16.src.rpm yazpp.src: I: checking yazpp.src: I: checking-url http://www.indexdata.dk/yazplusplus/ (timeout 10 seconds) yazpp.src: I: checking-url http://ftp.indexdata.dk/pub/yazpp/yazpp-1.2.7.tar.gz (timeout 10 seconds) 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
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=753597
--- Comment #1 from Ryan H. Lewis (rhl) me@ryanlewis.net 2011-11-13 12:40:08 EST --- *** Bug 753596 has been marked as a duplicate of this bug. ***
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=753597
--- Comment #2 from Ryan H. Lewis (rhl) me@ryanlewis.net 2011-11-13 12:47:50 EST --- I forgot to include the scratch build this of course succesfully builds in rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=3511373
Since this package requires yaz v4.2.8 or later it will currently only build in rawhide.
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=753597
Martin Gieseking martin.gieseking@uos.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.gieseking@uos.de
--- Comment #3 from Martin Gieseking martin.gieseking@uos.de 2011-11-20 05:33:04 EST --- Ryan, here are a couple of initial comments:
- please always check your rpms (source AND binary packages) with rpmlint:
$ rpmlint ./yazpp*.rpm yazpp.x86_64: E: devel-dependency libyaz-devel yazpp.x86_64: E: library-without-ldconfig-postin /usr/lib64/libyazpp.so.4.0.0 yazpp.x86_64: E: library-without-ldconfig-postun /usr/lib64/libyazpp.so.4.0.0 yazpp.x86_64: E: library-without-ldconfig-postin /usr/lib64/libzoompp.so.4.0.0 yazpp.x86_64: E: library-without-ldconfig-postun /usr/lib64/libzoompp.so.4.0.0 4 packages and 0 specfiles checked; 5 errors, 0 warnings.
- the Summary should be a bit more verbose ;)
- change the Group of the base package to "System Environment/Libraries"
- if you want to build the package for EPEL < 6 as well, adapt the BuildRoot field according to https://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag
If you targeting Fedora and/or EPEL 6 only, you can drop the BuildRoot tag, the %clean section, the initial cleaning of the buildroot in %install, and all %defattr lines
- Don't mix $RPM_BUILD_ROOT and %{buildroot}. Choose one variant and stick with it.
- Why does the base package require libyaz-devel? I think you can drop it.
- replace "%package -n yazpp-devel" with "%package devel", and "%description -n yazpp-devel" with "%description devel" Same for %files: replace "%files -n yazpp" with %files, and "%files -n yazpp-devel" with "%files devel"
- if you want to keep the %defattr lines, change them to %defattr(-,root,root,-)
- .la files must not be packaged in Fedora. Remove it in %install and drop it from %files
- Static libraries must go to a separate -static package. However, Fedora usually doesn't ship static libraries. If there's no good reason for providing it, just add --disable-static to %configure, and remove the .a file from %files.
- add "%post -p /sbin/ldconfig" and "%postun -p /sbin/ldconfig" between %clean and %file section: https://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries
- remove the echo line from the spec
- the devel package must require the base package this way: https://fedoraproject.org/wiki/Packaging/Guidelines#Requiring_Base_Package
- Please be more specific in %files, especially if you add only few files. This helps to avoid adding unwanted files and to get a quick overview of what's actually put into the package. For example, instead of %{_libdir}/*.so.* it's better to write: %{_libdir}/libyazpp.so.* %{_libdir}/libzoompp.so.*
- add file ChangeLog to the %docs
- The tarball contains a Doxyfile, so it's probably a good idea to build the doxygen documentation and add it to the %docs. As there is quite a lot of documentation available, I suggest to put it in a -doc subpackage.
- yazzpp provides some command-line tools, too (yaz-my-client, yaz-my-server, zclient, zlint). Maybe you should add them to a -utils subpackage.
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=753597
--- Comment #4 from Ryan H. Lewis (rhl) me@ryanlewis.net 2011-12-23 17:51:43 EST --- Hi,
sorry for the long reply time. I have addressed every issue,
all files located: http://fedorapeople.org/~rhl/yazpp
Here is the rpmlint output
spec: (i'm using this build root just to set a variable for make dox) $ rpmlint -v yazpp.spec yazpp.spec:47: W: rpm-buildroot-usage %build make %{?_smp_mflags} DESTDIR=${RPM_BUILD_ROOT} all dox yazpp.spec: I: checking-url http://ftp.indexdata.dk/pub/yazpp/yazpp-1.2.7.tar.gz (timeout 10 seconds) 0 packages and 1 specfiles checked; 0 errors, 1 warnings.
srpm: (same as above) $ rpmlint -v yazpp-1.2.7-3.fc16.src.rpm yazpp.src: I: checking yazpp.src: I: checking-url http://www.indexdata.dk/yazplusplus/ (timeout 10 seconds) yazpp.src:47: W: rpm-buildroot-usage %build make %{?_smp_mflags} DESTDIR=${RPM_BUILD_ROOT} all dox yazpp.src: I: checking-url http://ftp.indexdata.dk/pub/yazpp/yazpp-1.2.7.tar.gz (timeout 10 seconds) 1 packages and 0 specfiles checked; 0 errors, 1 warnings.
rpms: $ rpmlint -v *.rpm yazpp.x86_64: I: checking yazpp.x86_64: I: checking-url http://www.indexdata.dk/yazplusplus/ (timeout 10 seconds) yazpp-debuginfo.x86_64: I: checking yazpp-debuginfo.x86_64: I: checking-url http://www.indexdata.dk/yazplusplus/ (timeout 10 seconds) yazpp-devel.x86_64: I: checking yazpp-devel.x86_64: I: checking-url http://www.indexdata.dk/yazplusplus/ (timeout 10 seconds) yazpp-docs.x86_64: I: checking yazpp-docs.x86_64: I: checking-url http://www.indexdata.dk/yazplusplus/ (timeout 10 seconds) yazpp-utils.x86_64: I: checking yazpp-utils.x86_64: I: checking-url http://www.indexdata.dk/yazplusplus/ (timeout 10 seconds) yazpp-utils.x86_64: W: no-manual-page-for-binary zclient yazpp-utils.x86_64: W: no-manual-page-for-binary zlint 5 packages and 0 specfiles checked; 0 errors, 2 warnings.
here is the scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3603893
and I notified upstream as well as asked them to look at fixing the documentation: http://lists.indexdata.dk/pipermail/yazlist/2011-December/003366.html
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=753597
Martin Gieseking martin.gieseking@uos.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |martin.gieseking@uos.de Flag| |fedora-review?
--- Comment #5 from Martin Gieseking martin.gieseking@uos.de 2012-01-07 07:28:58 EST --- The package looks almost fine. There are just a few minor things that need some attention:
- Drop DESTDIR=$RPM_BUILD_ROOT from the make statement in %build. As far as I see, it's not necessary to build the package properly.
- Drop "%post devel -p /sbin/ldconfig" and "%postun devel -p /sbin/ldconfig". ldconfig doesn't need to be called for symlinks.
- You can remove "rm -rf ${RPM_BUILD_ROOT}" from the %install section. The %clean section is redundant as well.
- I recommend not to package the .3 man pages generated by doxygen. Instead, add the html variant. Since you create a -docs subpackage, move the doxygen docs there: + drop the whole %doc line from %files devel + add "%doc dox/html/" to %files doc
- add the following lines to the %install section: mkdir tmp mv ${RPM_BUILD_ROOT}/%{_docdir}/yazpp/ tmp and the line in %files docs should look like this: %doc tmp/* dox/html/ This way you ensure that all the doc files go to the same (and correct) directory.
$ rpmlint yazpp-* yazpp.src:48: W: rpm-buildroot-usage %build make %{?_smp_mflags} DESTDIR=${RPM_BUILD_ROOT} all dox yazpp-utils.x86_64: W: no-manual-page-for-binary zclient yazpp-utils.x86_64: W: no-manual-page-for-binary zlint 6 packages and 0 specfiles checked; 0 errors, 3 warnings.
--------------------------------- key:
[+] OK [.] OK, not applicable [X] needs work ---------------------------------
[+] MUST: The package must be named according to the Package Naming Guidelines. [+] MUST: The spec file name must match the base package %{name}. [+] MUST: The package must meet the Packaging Guidelines. [+] MUST: The package must be licensed with a Fedora approved license. [+] MUST: The License field in the package spec file must match the actual license. [+] MUST: The file containing the text of the license(s) for the package must be included in %doc. [+] MUST: The spec file must be written in American English. [+] MUST: The spec file for the package MUST be legible. [+] MUST: The sources used to build the package must match the upstream source. $ md5sum yazpp-1.2.7.tar.gz* 587f778f34b9b16de47ec26a2a3d1927 yazpp-1.2.7.tar.gz 587f778f34b9b16de47ec26a2a3d1927 yazpp-1.2.7.tar.gz.upstream
[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [.] MUST: If the package does not successfully compile, build or work on an architecture, ... [+] MUST: All build dependencies must be listed in BuildRequires. [+] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied. [.] MUST: The spec file MUST handle locales properly. [X] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun. - drop the calls of ldconfig for the devel package
[+] MUST: Packages must NOT bundle copies of system libraries. [.] MUST: If the package is designed to be relocatable, ... [X] MUST: A package must own all directories that it creates. - %{docdir}/yazpp/ is unowned
[X] MUST: A Fedora package must not list a file more than once in %files. - doc/yazpp-config.1* is listed twice
[+] MUST: Permissions on files must be set properly. [+] MUST: Each package must consistently use macros. [+] MUST: The package must contain code, or permissable content. [+] MUST: Large documentation files must go in a -doc subpackage. [+] MUST: Files in %doc must not affect the runtime of the application. [+] MUST: Header files must be in a -devel package. [.] MUST: Static libraries must be in a -static package. [+] MUST: If a package contains library files with a suffix, then library files that end in .so (without suffix) must go in a -devel package. [+] MUST: devel packages must require the base package using a fully versioned dependency. [+] MUST: Packages must NOT contain any .la libtool archives. [.] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [+] MUST: Packages must not own files or directories already owned by other packages. [+] MUST: All filenames in rpm packages must be valid UTF-8.
EPEL <= 5 only: [X] MUST: The spec file must contain a valid BuildRoot field. [+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}. [+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}. [.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
[.] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [+] SHOULD: The reviewer should test that the package builds in mock. [+] SHOULD: The package should compile and build into binary rpms on all supported architectures. [+] SHOULD: The reviewer should test that the package functions as described. [+] SHOULD: If scriptlets are used, those scriptlets must be sane. [+] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg. [.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [+] SHOULD: Your package should contain man pages for binaries/scripts.
https://bugzilla.redhat.com/show_bug.cgi?id=753597
--- Comment #6 from Martin Gieseking martin.gieseking@uos.de --- Ryan, what's the status of this package? Are you still interested in it? If not, please close the ticket as NOTABUG.
https://bugzilla.redhat.com/show_bug.cgi?id=753597
--- Comment #7 from Martin Gieseking martin.gieseking@uos.de --- Any news here? I'll close the ticket if there's no status update within a week. https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews#Submitter_...
https://bugzilla.redhat.com/show_bug.cgi?id=753597
--- Comment #8 from Ryan H. Lewis (rhl) me@ryanlewis.net --- I have lost interested, and free time.
https://bugzilla.redhat.com/show_bug.cgi?id=753597
Martin Gieseking martin.gieseking@uos.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Blocks| |201449 (FE-DEADREVIEW) Resolution|--- |NOTABUG Flags|fedora-review? | Last Closed| |2012-10-07 03:17:48
--- Comment #9 from Martin Gieseking martin.gieseking@uos.de --- OK, thanks for the feedback. Closing the ticket.
package-review@lists.fedoraproject.org