Bug 226483

(!!) MUST: rpmlint output:
**** Review message:
W: tcsh invalid-license distributable

********************
(!!) MUST: Package must meet the Package Naming Guidelines
**** Review message:
%{?dist} tag is not present. Release should be: 14%{?dist}

********************
(!!) MUST: License field in spec must match actual license.
**** Review message:
- License: distributable
  According to http://directory.fsf.org/tcsh.html the license should be BSD

********************
(!!) MUST: The package must successfully compile/build on at least 1 architecture.
**** Review message:
- Package does not compile successfully.
  For me, it is due to the missing -ltermcap option when linking.
  Compiles successfully without the tinfo patch.

********************
(!!) MUST: The spec file MUST handle locales properly.
**** Review message:
- The package does not use the %find_lang macro

********************
(!!) SHOULD: Packager should query upstream for license text file.
**** Review message:
- License file is missing.
********************

Thanks for the review.

> (!!) MUST: Package must meet the Package Naming Guidelines
> **** Review message:
> %{?dist} tag is not present. Release should be: 14%{?dist}
The dist tag is optional, see http://fedoraproject.org/wiki/Packaging/DistTag .

> (!!) MUST: License field in spec must match actual license.
> **** Review message:
> - License: distributable
>   According to http://directory.fsf.org/tcsh.html the license should be BSD
Updated.

> (!!) MUST: The package must successfully compile/build on at least 1 architecture.
> **** Review message:
> - Package does not compile successfully.
>   For me, it is due to the missing -ltermcap option when linking.
>   Compiles successfully without the tinfo patch.
Are you perhaps building on FC-6?  -ltinfo is only in rawhide ncurses, and tcsh
seems to build correctly using mock.

> (!!) MUST: The spec file MUST handle locales properly.
> **** Review message:
> - The package does not use the %find_lang macro
%find_lang works only on gettext message files, but tcsh uses catgets message
files.  The generated tcsh.file does mark the message files with the
corresponding %lang macro.

> (!!) SHOULD: Packager should query upstream for license text file.
> **** Review message:
> - License file is missing.
A patch was sent upstream.

Updated package is tcsh-6.14-15.

Taking over review.

- The patches are not commented. Comments should be added why any specific patch is needed.

- A newer version 6.16 has been released in September.

- Requires(post): grep and Requires(postun): coreutils, grep are a bit silly, aren't these already required by some minimal system rpm?

- Is the autoreconf really necessary?

- Drop the buildroot checks
[ -n "$RPM_BUILD_ROOT" -a "$RPM_BUILD_ROOT" != / ] 
in install and clean phase.

- Consider safening the %post and %postun phases with

%post
if [ ! -f /etc/shells ]; then
	echo "%{_bindir}/tcsh" >> /etc/shells
	echo "%{_bindir}/csh"  >> /etc/shells
else
	grep -q '^%{_bindir}/tcsh$' /etc/shells || \
	echo "%{_bindir}/tcsh" >> /etc/shells
	grep -q '^%{_bindir}/csh$'  /etc/shells || \
	echo "%{_bindir}/csh"  >> /etc/shells
fi

%postun
if [ ! -x %{_bindir}/tcsh ]; then
	grep -v '^%{_bindir}/tcsh$'  /etc/shells | \
	grep -v '^%{_bindir}/csh$' > /etc/shells.rpm &&
	mv /etc/shells.rpm /etc/shells
fi


- Package does not handle locales in the right manner. Installing manually is OK, but after that use %{find_lang} to build the file list.

rpmlint output:
tcsh.x86_64: W: file-not-utf8 /usr/share/doc/tcsh-6.15/Fixes
tcsh.x86_64: W: dangerous-command-in-%postun rm
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

- Convert Fixes file to utf8 with iconv.


MUST: The spec file for the package is legible and macros are used consistently. ~OK
- Some comments could be nice in the install phase, it would make the spec file a lot easier to read and understand.

MUST: The License field in the package spec file must match the actual license. NEEDSFIX
- The license in source code is 3-clause BSD, not BSD with advertising. Change License tag to BSD.
- Contact upstream to clarify this, since CopyRight is 4-clause.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. ~OK
- The file matches but source URL is bad; now the correct url has old/ before the release. Switch to newest release will fix this.

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

MUST: Optflags are used and time stamps preserved. NEEDSFIX
- SMP make is not enabled.
- Use -p flag to install in install phase.

MUST: A package must own all directories that it creates or require the package that owns the directory. OK
- Please change %{_mandir}/*/* to %{_mandir}/man1/*.1, since it's safer this way.

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSFIX
- Include BUGS and WishList in the package. Remember to convert the files to utf8.

SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
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: Packages containing shared library files must call ldconfig. OK
MUST: Files only listed once in %files listings. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. OK
MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. OK
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK
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. OK
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: The package builds in mock. OK

vcrhonek: please rectify.

Hi Jussi,

Thanks for adding me to the CC.

(In reply to comment #4)
> - The patches are not commented. Comments should be added why any specific
> patch is needed.

I think it's good idea with new patches, but I don't want comment them retrospectively.

> 
> - A newer version 6.16 has been released in September.

Rebased.

> 
> - Requires(post): grep and Requires(postun): coreutils, grep are a bit silly,
> aren't these already required by some minimal system rpm?

Probably not (e. g. when you have busybox, then you don't need coreutils). We should rewrite these scripts to RPM Lua maybe...:)

> 
> - Is the autoreconf really necessary?

Yes, it is. Because of using ltinfo instead of ltermcap. We need to refresh configure.

> 
> - Drop the buildroot checks
> [ -n "$RPM_BUILD_ROOT" -a "$RPM_BUILD_ROOT" != / ] 
> in install and clean phase.

Fixed.

> 
> - Consider safening the %post and %postun phases with
> 
> %post
> if [ ! -f /etc/shells ]; then
>  echo "%{_bindir}/tcsh" >> /etc/shells
>  echo "%{_bindir}/csh"  >> /etc/shells
> else
>  grep -q '^%{_bindir}/tcsh$' /etc/shells || \
>  echo "%{_bindir}/tcsh" >> /etc/shells
>  grep -q '^%{_bindir}/csh$'  /etc/shells || \
>  echo "%{_bindir}/csh"  >> /etc/shells
> fi
> 
> %postun
> if [ ! -x %{_bindir}/tcsh ]; then
>  grep -v '^%{_bindir}/tcsh$'  /etc/shells | \
>  grep -v '^%{_bindir}/csh$' > /etc/shells.rpm &&
>  mv /etc/shells.rpm /etc/shells
> fi

Done.

> 
> 
> - Package does not handle locales in the right manner. Installing manually is
> OK, but after that use %{find_lang} to build the file list.  

See comment #2 from Miroslav, %{find_lang} doesn't work here. The file list (tcsh.lang) is assembled in %install, locales manually installed here too and finally packed in %files (taken from the file list).

(In reply to comment #5)
> rpmlint output:
> tcsh.x86_64: W: file-not-utf8 /usr/share/doc/tcsh-6.15/Fixes
> tcsh.x86_64: W: dangerous-command-in-%postun rm
> 3 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> - Convert Fixes file to utf8 with iconv.

Fixed.

> 
> 
> MUST: The spec file for the package is legible and macros are used
> consistently. ~OK
> - Some comments could be nice in the install phase, it would make the spec file
> a lot easier to read and understand.
> 
> MUST: The License field in the package spec file must match the actual license.
> NEEDSFIX
> - The license in source code is 3-clause BSD, not BSD with advertising. Change
> License tag to BSD.
> - Contact upstream to clarify this, since CopyRight is 4-clause.

Fixed (I was probably confused with this Copyrigt file).

> 
> MUST: The sources used to build the package must match the upstream source, as
> provided in the spec URL. ~OK
> - The file matches but source URL is bad; now the correct url has old/ before
> the release. Switch to newest release will fix this.

Fixed (rebase).

> 
> MUST: The package MUST successfully compile and build into binary rpms. OK
> MUST: The spec file MUST handle locales properly. NEEDSFIX

Commented before.

> 
> MUST: Optflags are used and time stamps preserved. NEEDSFIX
> - SMP make is not enabled.
> - Use -p flag to install in install phase.

Fixed.

> 
> MUST: A package must own all directories that it creates or require the package
> that owns the directory. OK
> - Please change %{_mandir}/*/* to %{_mandir}/man1/*.1, since it's safer this
> way.

Fixed.

> 
> MUST: All relevant items are included in %doc. Items in %doc do not affect
> runtime of application. NEEDSFIX
> - Include BUGS and WishList in the package. Remember to convert the files to
> utf8.

Fixed.

> 
> SHOULD: If the package does not include license text(s) as separate files from
> upstream, the packager should query upstream to include it. OK
> 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: Packages containing shared library files must call ldconfig. OK
> MUST: Files only listed once in %files listings. OK
> MUST: Permissions on files must be set properly. OK
> MUST: Clean section exists. OK
> MUST: Large documentation files must go in a -doc subpackage. OK
> MUST: Header files must be in a -devel package. OK
> MUST: Static libraries must be in a -static package. OK
> MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK
> 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. OK
> MUST: No file conflicts with other packages and no general names. OK
> MUST: Buildroot cleaned before install. OK
> SHOULD: %{?dist} tag is used in release. OK
> SHOULD: The package builds in mock. OK  


Changes commited to the devel branch. Please check it and let me know your opinion. Thanks!

- Time stamps are not conserved. 

for i in Fixes WishList; do
    iconv -f iso-8859-1 -t utf-8 < "$i" > "${i}_"
    mv "${i}_" "$i"
done

should be

for i in Fixes WishList; do
    iconv -f iso-8859-1 -t utf-8 "$i" > "${i}_" && \
    touch -r "$i" "${i}_" && \
    mv "${i}_" "$i"
done

- Defattr should be
 (-,root,root,-)
not
 (-,root,root)


Fix these before building the new release. The package has been

APPROVED

Built in tcsh-6.16-1.fc12