Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 753597 - Review Request: yazpp - C++ API for YAZ
Summary: Review Request: yazpp - C++ API for YAZ
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 753596 (view as bug list)
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2011-11-13 17:25 UTC by Ryan H. Lewis (rhl)
Modified: 2012-10-07 07:17 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-10-07 07:17:48 UTC


Attachments (Terms of Use)

Description Ryan H. Lewis (rhl) 2011-11-13 17:25:56 UTC
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.

Comment 1 Ryan H. Lewis (rhl) 2011-11-13 17:40:08 UTC
*** Bug 753596 has been marked as a duplicate of this bug. ***

Comment 2 Ryan H. Lewis (rhl) 2011-11-13 17:47:50 UTC
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.

Comment 3 Martin Gieseking 2011-11-20 10:33:04 UTC
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.

Comment 4 Ryan H. Lewis (rhl) 2011-12-23 22:51:43 UTC
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

Comment 5 Martin Gieseking 2012-01-07 12:28:58 UTC
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.

Comment 6 Martin Gieseking 2012-09-14 11:02:17 UTC
Ryan, what's the status of this package? Are you still interested in it? If not, please close the ticket as NOTABUG.

Comment 7 Martin Gieseking 2012-10-06 08:35:32 UTC
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_not_responding

Comment 8 Ryan H. Lewis (rhl) 2012-10-06 16:34:38 UTC
I have lost interested, and free time.

Comment 9 Martin Gieseking 2012-10-07 07:17:48 UTC
OK, thanks for the feedback. Closing the ticket.


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