Bug 598511

This is an informal review. Formal review will follow.

MUST items:

OK: rpmlint must be run on every package. The output should be posted in the review.

Rpmlint has been carried out by me.
$ rpmlint libgtextutils*.rpm
libgtextutils.src: W: spelling-error Summary(en_US) Assaf -> Assad, Assam, Assay
libgtextutils.src: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
libgtextutils.x86_64: W: spelling-error Summary(en_US) Assaf -> Assad, Assam, Assay
libgtextutils.x86_64: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
libgtextutils-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 5 warnings.


OK: The package must be named according to the Package Naming Guidelines .
OK: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
OK: The package must meet the Packaging Guidelines .
OK: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
OK: The License field in the package spec file must match the actual license. 
OK: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.


OK: The spec file must be written in American English. 

Rpmlint says two spelling-error. However they are proper names.


OK: The spec file for the package MUST be legible. 
OK: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
OK (x86_64): The package MUST successfully compile and build into binary rpms on at least one primary architecture. 
OK (x86_64): If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. 


NG: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.

According to http://fedoraproject.org/wiki/Packaging/Guidelines
There is no need to include gcc and gcc-c++ BuildRequires.


NA: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
OK: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. 
OK: Packages must NOT bundle copies of system libraries.
NA: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. 


NG: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 

%{_includedir}/gtextutils and %{_includedir}/gtextutils/gtextutils directories should be owned by -devel subpackage by using %dir.


OK: A Fedora package must not list a file more than once in the spec file's %files listings. 
OK: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. 
OK: Each package must consistently use macros. 
OK: The package must contain code, or permissable content. 
NA: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). 
OK: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. 
OK: Header files must be in a -devel package. 
NA: Static libraries must be in a -static package. 
OK: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. 
OK: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} 
OK: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
NA: 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. 
OK: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
OK: All filenames in rpm packages must be valid UTF-8. 


SHOULD items:
NA: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. 
NA: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. 
OK: The reviewer should test that the package builds in mock. 
OK (x86_64), other not tested: The package should compile and build into binary rpms on all supported architectures. 
not tested: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
OK: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. 
NA: Usually, subpackages other than devel should require the base package using a fully versioned dependency. 
OK: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. 
NA: 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. 


NA: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.

But documents too few...

> %{_includedir}/gtextutils and %{_includedir}/gtextutils/gtextutils
> directories should be owned by -devel subpackage by using %dir.

They are included correctly:

$ rpmls -p libgtextutils-devel-0.6-1.fc13.x86_64.rpm|grep ^d
drwxr-xr-x  /usr/include/gtextutils
drwxr-xr-x  /usr/include/gtextutils/gtextutils

That is because the spec file does

  %{_includedir}/*

which includes the files %{_includedir}/*, and if that happens to be a directory, it includes the directory and the entire tree beyond it.


[As why the developers create two gtextutils/gtextutils header directories, so a simple '#include <gtextutils/foo>' is not enough without adjusting the search path for headers, is beyond me, btw.]

(In reply to comment #2)
> > %{_includedir}/gtextutils and %{_includedir}/gtextutils/gtextutils
> > directories should be owned by -devel subpackage by using %dir.
> 
> They are included correctly:

Thank you for pointing out.

In my very old days, I have many experiences that some empty directories are still exist after removing packages without "%dir".

Now I carried out install-and-remove test with this package and the directories are correctly removed.


Please ignore my review for %dir issue.

> In my very old days, I have many experiences that some empty
> directories are still exist after removing packages without "%dir".

Missing explicit %dir is unlikely to be the cause. Those symptoms sound like misplaced directory ownership and wrong inter-package dependencies. That is, the package that owns the directories is removed prior to other packages, which store files in the same directories. RPM can only delete a directory, if it's empty.

rpmlint output:
libgtextutils.src: W: spelling-error Summary(en_US) Assaf -> Assad, Assam, Assai
libgtextutils.src: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
libgtextutils.x86_64: W: spelling-error Summary(en_US) Assaf -> Assad, Assam, Assai
libgtextutils.x86_64: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
libgtextutils-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 5 warnings.

These are OK.

**

I recommend using just
 http://hannonlab.cshl.edu/fastx_toolkit/
as the URL.

**

MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK

MUST: The spec file for the package is legible and macros are used consistently. NEEDSWORK
- You are mixing %{optflags} and $RPM_BUILD_ROOT. This is not allowed.
http://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

- The %files section could use some clarity. Using wildcards where they are not needed is bad style, and packages might end up owning stuff they're not supposed to. Please change
 %{_includedir}/*
to 
 %{_includedir}/gtextutils/
and consider also using
 %{_libdir}/libgtextutils-*.so.*
 %{_libdir}/libgtextutils-*.so


MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. NEEDSWORK
- licensecheck reports:
 ./src/gtextutils/string_tokenize.h: AGPL (v3 or later) 
and the same for other files as well. Looking at the license text itself confirms that this is a "v3 or later" license. => License tag should be AGPLv3+.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
d6969aa0d31cc934e1fedf3fe3d0dc63  libgtextutils-0.6.tar.bz2
d6969aa0d31cc934e1fedf3fe3d0dc63  ../SOURCES/libgtextutils-0.6.tar.bz2

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- The compilation process adds some of its own flags:
" -Wextra -Wformat-nonliteral -Wformat-security -Wswitch-default -Wswitch-enum -Wunused-parameter -Wfloat-equal -Werror -DDEBUG -g -O1 -DDEBUG -g -O1 -version-info 0:0:0 -release 0.6"
- You must get rid of at least the "-g -O1". You'll need to patch configure to do this. I suggest removing lines 15093-15100 altogether, which read

if test "$debug" = "true"
then
  CFLAGS="${CFLAGS} -DDEBUG -g -O1"
  CXXFLAGS="${CFLAGS} -DDEBUG -g -O1"
else
  CFLAGS="${CFLAGS} -O3"
  CXXFLAGS="${CFLAGS} -O3"
fi


MUST: Packages containing shared library files must call ldconfig. OK
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Large documentation files must go in a -doc subpackage. OK
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
EPEL: Clean section exists. OK
EPEL: Buildroot cleaned before install. OK
EPEL: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK

Oh, so you have made release 2? Ugh.

Anyway, the issues still remain.

**

Overriding the compilation flags is, of course, the cleaner solution to making sure the right flags are used. However, setting CFLAGS is not enough, also CXXFLAGS needs to be set, so use

make %{?_smp_mflags} CFLAGS="%{optflags}" CXXFLAGS="%{optflags}"

(or, if you decide to go with the other macro syntax, use $RPM_OPT_FLAGS instead of %{optflags})

**

Some headers are installed, so you might want to consider saving their time stamps in %install with

 make install INSTALL="install -p"

Still one issue: by installing the package and running rpmlint on it, I get

libgtextutils.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libgtextutils-0.6.so.0.0.0 /lib64/libm.so.6

You can probably get rid of this with the help of
https://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency

New version which should address your concerns is at:

http://verdurin.org.uk/~verdurin/fedora/reviews/fastx_toolkit/libgtextutils-0.6-3.fc12.src.rpm

http://verdurin.org.uk/~verdurin/fedora/reviews/fastx_toolkit/libgtextutils.spec

Apologies for the mixed macros - I used to start a new .spec in Emacs and its default includes both styles, which I tended to forget to correct.  Now I use rpmdev-newspec -m, which doesn't have this problem.

The query I still have is over the licensing.  Rpmlint complains that the value you suggested - AGPLv3+ - is not valid.  The choice seems to be:  "AGPLv3" or "AGPLv3 with exceptions"

Whoops, I'm terribly sorry that this has slipped under my radar, as I've been quite busy at $DAYJOB. I'll try to look through this ASAP.

rpmlint now stands at

libgtextutils.src: W: spelling-error Summary(en_US) Assaf -> Assad, Assam, Assai
libgtextutils.src: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
libgtextutils.x86_64: W: spelling-error Summary(en_US) Assaf -> Assad, Assam, Assai
libgtextutils.x86_64: W: spelling-error %description -l en_US fastx -> fast, fasts, fast x
libgtextutils-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 5 warnings.

The shlib issue and the compilation flags have been fixed.

(In reply to comment #8)
> The query I still have is over the licensing.  Rpmlint complains that the value
> you suggested - AGPLv3+ - is not valid.  The choice seems to be:  "AGPLv3" or
> "AGPLv3 with exceptions"

Well, rpmlint is not the canonical source for these kinds of issues. It's a tool targeted at all distributions using RPM, so it doesn't handle Fedora specific stuff well. 

The Fedora Licensing page at http://fedoraproject.org/wiki/Licensing does as well have only AGPLv1, AGPLv3 and AGPLv3 with exceptions, but all other GNU licenses seem to have the + versions that indicate that also later versions are accepted.

The license of libgtextutils contains
   This program is free software: you can redistribute it and/or modify
   it under the terms of the GNU Affero General Public License as published by
   the Free Software Foundation, either version 3 of the License, or
   (at your option) any later version.

so I would really put AGPLv3+, although it makes rpmlint cry.

Please fix this issue before git import. The package has been

APPROVED


PS. One tiny nit-pick jumped to my eye: the empty line after %description is IMHO unnecessary; you don't have one in -devel.

For reference, I've put a new version with the licensing and formatting fixes at:

http://verdurin.fedorapeople.org/reviews/libgtextutils/libgtextutils.spec

http://verdurin.fedorapeople.org/reviews/libgtextutils/libgtextutils-0.6-4.fc14.src.rpm

New Package SCM Request
=======================
Package Name: libgtextutils
Short Description: Assaf Gordon text utilities
Owners: verdurin
Branches: f13 f14 el5 el6
InitialCC:

Git done (by process-git-requests).

Ping Adam, what's the status?

Forgot to build this before now - closing.  Thanks for the review.

libgtextutils-0.6-5.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/libgtextutils-0.6-5.fc14

libgtextutils-0.6-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/libgtextutils-0.6-5.el6

libgtextutils-0.6-5.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/libgtextutils-0.6-5.el5

libgtextutils-0.6-5.fc14 has been pushed to the Fedora 14 stable repository.

libgtextutils-0.6-5.el5 has been pushed to the Fedora EPEL 5 stable repository.

libgtextutils-0.6-5.el6 has been pushed to the Fedora EPEL 6 stable repository.